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

Version check improvements #32

Merged
merged 3 commits into from
Nov 26, 2023

Conversation

a-wai
Copy link
Contributor

@a-wai a-wai commented Nov 2, 2023

This PR addresses 2 distinct "issues":

  • it fixes Version checks are too strict in 0.5.1 #31 by adding a check on a likely-compatible version in case the exact check fails
  • it implements "outdated" reporting and adds a new "compatible" status (for crates not fully up-to-date, but likely compatible according to semver) -- reporting the Debian package version in both cases so it's more obvious/useful to the user

Those are split in 2 separate commits in case you only want to cherry-pick the first part.

When calling `search_generic()`, the `version` parameter is taken from
the output of `cargo metadata`, which always return the latest version
available on crates.io. Due to ba11751 this implies the crate will be
considered missing from Debian unless the packaged version is the latest
one.

This commit adds a fallback version check in case the initial check
fails, by trimming the version to a supposedly compatible semver
version.

Fixes kpcyrd#31
Package search only returns a boolean depending on whether a package
with a correct version exists in Debian (or NEW). This isn't sufficient
to handle the cases where the package exists, but is so outdated that
the version in Debian is likely incompatible with the crate we're
inspecting.

Moreover, this doesn't differentiate between exact version matches and
semver'd matches (e.g. when the Debian version is older but has the same
major -- and, if needed, minor -- as the version found on crates.io). In
order to deal with the latter case, this change introduces the notion of
"compatible" version: it represents crates already packaged in Debian
but not on the latest version, although it *should* still be compatible
with the requirements.

The "compatible" and "outdated" information are then propagated, as well
as the Debian package version, through a new `db::PkgInfo` structure
used as the return type for the `db::search*` functions. That way, this
additional information can easily be processed by other modules and
ultimately be reported to the user.
@alexanderkjall
Copy link
Collaborator

Thanks a lot for your contribution :)

I know that there isn't very good test coverage today, but how do you feel about creating a test that showcases this behavior? It feels like this can be subtle enough to easily break by mistake in the future.

@kpcyrd
Copy link
Owner

kpcyrd commented Nov 6, 2023

I haven't had time to look into this yet (the last few days both the Reproducible Builds Summit and the Arch Linux Summit kept me quite busy), but I also noticed the current cargo debstatus behavior breaks my workflow and I'm all for having this fixed! :)

@a-wai
Copy link
Contributor Author

a-wai commented Nov 6, 2023

Thanks a lot for your contribution :)

I know that there isn't very good test coverage today, but how do you feel about creating a test that showcases this behavior? It feels like this can be subtle enough to easily break by mistake in the future.

Hmm, I came up with the following test:

    #[test]
    fn check_version_reqs() {
        let mut db = Connection::new().unwrap();
        // Debian bullseye has rust-serde v1.0.106 and shouldn't be updated anymore
        let query = "SELECT version::text FROM sources WHERE source in ($1, $2) AND release='bullseye';";
        let info = db.search_generic(query, "serde", &Version::parse("1.0.100").unwrap()).unwrap();
        assert_eq!(info.status, PkgStatus::Found);
        assert_eq!(info.version, "1.0.106");
        let info = db.search_generic(query, "serde", &Version::parse("1.0.150").unwrap()).unwrap();
        assert_eq!(info.status, PkgStatus::Compatible);
        let info = db.search_generic(query, "serde", &Version::parse("2.0.0").unwrap()).unwrap();
        assert_eq!(info.status, PkgStatus::Outdated);
        let info = db.search_generic(query, "notacrate", &Version::parse("1.0.0").unwrap()).unwrap();
        assert_eq!(info.status, PkgStatus::NotFound);
    }

The thing, it requires network access, so it can work for upstream (this repo), but would have to be patched out for the package to build on Debian's buildd.

@kpcyrd
Copy link
Owner

kpcyrd commented Nov 6, 2023

Something I've done in cases like this is:

    #[test]
    #[ignored]
    fn check_version_reqs() {
        // ...
    }

Then run both cargo test and cargo test -- --ignored in CI, while Debian would only run cargo test.

@a-wai
Copy link
Contributor Author

a-wai commented Nov 6, 2023

@kpcyrd Thanks for the hint! I just pushed the test as a new commit based on that

This new test searches for `serde` in Debian Bullseye, which is
`oldstable` now and thus won't be updated anymore. It requests various
versions to ensure the crate is reported as "found", "outdated" or
"compatible" appropriately. It also searches for the non-existing
`notacrate` to ensure it properly reports missing packages.

Finally, this test (which requires network access) is marked as ignored
so it isn't run by default, avoiding issues with Debian's build process.
The workflow has been updated to ensure this test is run in CI.
@kpcyrd
Copy link
Owner

kpcyrd commented Nov 6, 2023

Thanks, you now also need to edit .github/workflows/rust.yml to include cargo test -- --ignored :)

    steps:
    - uses: actions/checkout@v2
    - name: Build
      run: cargo build --verbose
    - name: Run tests
      run: cargo test --verbose
    - name: Run tests (online)
      run: cargo test --verbose -- --ignored
    - name: Run debstatus on itself
      run: cargo run -- debstatus

@a-wai
Copy link
Contributor Author

a-wai commented Nov 6, 2023

Done already, using --include-ignored so all tests run in one pass ;)

(unless you prefer to keep those as 2 separate steps?)

@kpcyrd
Copy link
Owner

kpcyrd commented Nov 6, 2023

Doing them both in once is fine, I wasn't aware there's an option for that :)

@jamessan
Copy link
Contributor

Thanks for improving the version checking! I had noted in #29 (comment) that there were some side-effects, and hadn't gotten time to work on addressing those.

@alexanderkjall alexanderkjall merged commit 7c0f41c into kpcyrd:main Nov 26, 2023
3 checks passed
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.

Version checks are too strict in 0.5.1
4 participants