Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added version comparison and tests #147

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cjrolo
Copy link
Collaborator

@cjrolo cjrolo commented Jan 9, 2025

Closes #146

This PR addresses issue #146 in the following way:

  1. Adds Version information in the CompressorHeader (Extracted from Cargo.toml)
  2. On Decoding checks the version of the file against the version of the compressor.
    2.1 Doesn't allow decompression of higher version files by lower version compressor (Should it?)
  3. Add tests for the compression/decompression
  4. Minor version bump to start tagging the files where the header was introduced.

@cjrolo cjrolo requested review from rukai and worryg0d January 9, 2025 12:28
Copy link
Collaborator

@worryg0d worryg0d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I left some nits regarding some code duplications and little optimization.

2.1 Doesn't allow decompression of higher version files by lower version compressor (Should it?)

I think so. The frame format may be changed in the future, so parsing them with an outdated compressor version may lead to undefined behavior.

Comment on lines 40 to 49
let current_version = env!("CARGO_PKG_VERSION").to_string();
trace!("Versions: c:{} h:{}", current_version, header.version);
match compare(current_version.clone(), header.version.clone()) {
Ok(Cmp::Lt) => panic!(
"Can't decompress! File is version ({}) is higher than compressor version ({})!",
header.version, current_version
),
Ok(Cmp::Eq | Cmp::Gt) => debug!("File version: {}", header.version),
_ => panic!("Wrong version number!"),
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Both decode and borrow_decode contain the same code for verifying the version field. Making a func for this looks reasonable to me.

fn verify_header_versions(header: &CompressorHeader) {
    let current_version = env!("CARGO_PKG_VERSION").to_string();
    trace!("Versions: c:{} h:{}", current_version, header.version);
    match compare(&current_version, &header.version) {
        Ok(Cmp::Lt) => panic!(
            "Can't decompress! File is version ({}) is higher than compressor version ({})!",
            header.version, current_version
        ),
        Ok(Cmp::Eq | Cmp::Gt) => debug!("File version: {}", header.version),
        _ => panic!("Wrong version number!"),
    }
}

atsc/src/header.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add versioning to compressed files.
2 participants