Rework and improve the code around package version and package version constraints #450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is mainly done for consistency. There are currently two ways to specify package/dependency versions: - `PackageVersion`: An exact version. - `PackageVersionConstraint`: A "constraint" character followed by a version. Only `=` is currently supported as "constraint" character though, so the version must be an exact match as well. This means that `PackageVersionConstraint` currently only implements the exact same version matching logic as `PackageVersion` but additionally requires the `=` character in front of the version string. This makes that "constraint" character optional so that it doesn't trigger a parsing error when, e.g., a CLI argument lacks it (it was a bit annoying that some subcommands use `PackageVersionConstraint` and therefore required the `=` in front of the version). Signed-off-by: Michael Weiss <[email protected]>
The previous commit used the conservative approach to treat `PackageVersionConstraint` without any constraint as `PackageVersion` (exact version match) but we want to eventually implement real package version requirements/constraints so let's use the semver crate for that as it implements Rust's nice version requirement syntax [0]. This is a bit of a dangerous change as `PackageVersionConstraint` may now, for the first time, result in multiple package versions that match (so we must ensure that all usages in the code are correct and will either pick only the newest version or correctly handle all versions). [0]: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html Signed-off-by: Michael Weiss <[email protected]>
This fixes the documentation/description of the help output. Signed-off-by: Michael Weiss <[email protected]>
Using a `PackageVersionConstraint` can result in multiple versions of the package matching, in which case butido will output the environment variables of all packages/versions but without any indication which variable belongs to which version (optionally with some variable merging logic). We could support this but for now it seems best to explicitly only support a single version. Signed-off-by: Michael Weiss <[email protected]>
The `--all` flag didn't work properly as it expected a value. The old usage (from the error/help output) looks like this: > butido release new [OPTIONS] --to <RELEASE_STORE_NAME> <PKG|--all <all-packages>> <SUBMIT> [VERSION] The position/index of `PKG` and `SUBMIT` are wrong. The `PKG` argument should come after `SUBMIT` but it likely appears before `SUBMIT` in the usage output as it's in a `ArgGroup` with the `--all` flag and flags/options come before arguments with values. I decided to simply swap the indices of `PKG` and `SUBMIT` as it's more important that the usage matches the parsing (-> no unexpected errors) than that it's quicker / more ergonomic to repeat the command with the same submit but a different package. The new usage is now consistent with the parsing: > butido release new [OPTIONS] --to <RELEASE_STORE_NAME> <PKG|--all> <SUBMIT> [VERSION] It isn't ideal though that `SUBMIT` is now between `PKG` and `VERSION`! (But it currently seems to be the only combination that works right...) Signed-off-by: Michael Weiss <[email protected]>
I noticed that the approach from the previous commit sometimes(?) still resulted in some issues and it looks like it's a bad idea to mix options/flags and positional arguments/values in a `ArgGroup`. The `all-packages` argument isn't even used in the code (it's sufficient that `package` is not set) so it seems way batter to simply drop the `--all` flag/option and keep `package_name` optional (I've extended the description a bit to properly document the behaviour). I've also restored the original indices of the positional values/arguments to make the usage more ergonomic again. Signed-off-by: Michael Weiss <[email protected]>
It should be safe to assume that users want to release at least a single artifact if they execute the "release new" subcommand so we should emit an error message and unsuccessful exit code if nothing happened (can be the case when there's a typo in the package name and/or version). Signed-off-by: Michael Weiss <[email protected]>
The code calls `unwrap()` on both arguments and therefore panics if any of them are not set (`None`). This fixes the specification of both arguments so that the usage is correct and that Clap will emit an error instead of butido panicking/crashing. It also makes sense that both of these arguments are required to avoid accidental deletion of multiple versions up to nuking an entire release store (there's a confirmation prompt but accidentally hitting `y` is enough to confirm it). Signed-off-by: Michael Weiss <[email protected]>
I noticed a few mistakes in the help/usage strings and added some additional improvements (hopefully/IMO). Signed-off-by: Michael Weiss <[email protected]>
Due to my previous findings, I decided to check all commands / help outputs and those are the final improvements/tweaks I came up with. Signed-off-by: Michael Weiss <[email protected]>
To make that part consistent with the rest of the output and I also found a typo. Signed-off-by: Michael Weiss <[email protected]>
The old approach seemed unnecessary redundant so I simply switched to giving the visual sections a title instead and added some optional indentation as that looks nicer. It's not perfect but IMO it looks a bit better now and it should certainly be good enough for that command. Signed-off-by: Michael Weiss <[email protected]>
We might want to add an abstraction for those arguments in the future to
make this easier but at least the arguments are now all named
`package_version_constraint` with the argument value name
`VERSION_CONSTRAINT` (e.g., in the help output), and a help text /
description that makes clear that this is a version constraint and not a
single, exact version.
The documentation of the package version arguments for the
`source {verify,url,of}` subcommands didn't match the code
(a `PackageVersionConstraint` is used in the code).
Signed-off-by: Michael Weiss <[email protected]>
The code uses a `PackageVersion` and not a `PackageVersionConstraint`. Signed-off-by: Michael Weiss <[email protected]>
The `dependencies-of` subcommand has an optional `VERSION_CONSTRAINT` argument (s. `src/cli.rs`) but during my checks I noticed that it wasn't used/implemented in the code. This fixes that by implementing it. Signed-off-by: Michael Weiss <[email protected]>
Those don't match the implementation anymore and we already return a more appropriate error context/hint in the `TryFrom` implementation for `PackageVersionConstraint`. Signed-off-by: Michael Weiss <[email protected]>
This seems like a much better and safer default than `semver::On::Caret`, which I chose in fe0df00. It's more intuitive and it's also the best way to support both our strict implementation for `=` (full version matches only) as well as `semver::Op::Exact` (partial version matches are supported). I don't remember why I previously picked `^` (Caret / "compatible" updates) as the default - probably just because it's the default that the `semver` crate uses [0] (since the crate is for Cargo's flavor of SemVer [1]) and I didn't spend much thought on it. [0]: https://github.com/dtolnay/semver/blob/1.0.24/src/parse.rs#L158 [1]: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#caret-requirements Signed-off-by: Michael Weiss <[email protected]>
The `PackageVersionConstraint::matches()` function currently only
returns a boolean which is very bad for error handling as this results
in a butido crash ("bug") instead of a pretty error message of what went
wrong. This change doesn't fix that issue but at least it makes the
method much more reliable/usable/suitable. Our version specification
isn't compatible with `semver::Version` so I decided to implement a
fallback parser so that any `PackageVersion` can be converted into a (at
least somewhat appropriate) `semver::Version`. This might cause issues
in special cases but it should work well enough for any sane version
numbers/strings (in our cases we only ran into this issue with "partial"
version numbers like FFmpeg 7.1 (no patch version) or versions with
non-numeric characters like tmux 3.5a.
Signed-off-by: Michael Weiss <[email protected]>
I later noticed that the previous iteration didn't work as "recursive" capture groups aren't supported (so the nested numbered capture group for the version number(s) in the outer non-capturing group that may match zero or more times doesn't result in more than one capturing group and only the last match will be returned for the capturing group). This new logic fixes that but it's still far from ideal... I also noticed that butido doesn't seem to do any verification of the version numbers using `PackageVersion::parser()` so far - at least in the code branches that I tested so we definitely should change that. Signed-off-by: Michael Weiss <[email protected]>
We currently support two kinds of `PackageVersionConstraint`: 1) Exact version matches (`=$VERSION`): Those are implemented using string comparison (`==`). 2) Exact but partial version matches (`$VERSION`): Those are implemented using the `semver` crate and `semver::Op::Exact` (`=$VERSION`). In the second case the version number/string must be a valid `semver::VersionReq` which is more strict than our butido versions (especially since we have a parser for `PackageVersion` but it isn't used to validate the versions yet). We didn't check this in `PackageVersionConstraint::parser()` before so butido would panic in `PackageVersionConstraint::matches()` instead of returning an appropriate error message if the provided `PackageVersionConstraint` isn't "valid". This fixes that by attempting to parse the version using the `semver` crate in the `parser()` function as well (we need it to parse it as `VersionReq` since `Version` must be a full version (including the minor and patch versions) - I therefore decide to deduplicate `default_constraint` using a private function). Signed-off-by: Michael Weiss <[email protected]>
This is done to make it "compatible"/interchangeable with `PackageVersionConstraint` (we'll probably only allow it temporarily until we properly sort out the versioning and handling of dependency specifications). Signed-off-by: Michael Weiss <[email protected]>
The current logic was never capable of properly handling `PackageVersionConstraint` but this wasn't an issue so far since the only supported constraint was an exact version match using `=`. I decided to temporarily switch this code to just `PackageVersion` before properly implementing this to avoid potential "regressions". (The current code works fine with `PackageVersionConstraint` but the logic doesn't make sense as all matching versions would be build instead of only the newest one - this isn't an issue as long as the `pkg.toml` files aren't changed to use versions that aren't prefixed using `=` but let's avoid this potential issue for now.) Signed-off-by: Michael Weiss <[email protected]>
If there isn't a constraint (only `=` is currently supported) and the version cannot be parsed as a SemVer (`VersionReq`), we'll automatically fall back to strict version matching (as if `=` was provided) in order to better support exotic versions. This is done to retain CLI compatibility with the old behaviour. Signed-off-by: Michael Weiss <[email protected]>
This merge commit is only necessary as there is a single merge conflict in `src/commands/source/download.rs` and I don't want to rebase my long running feature branch that reworks/improves/fixes the logic around `PackageVersion` and `PackageVersionConstraint`. Ideally GitHub could handle this but I currently can only choose between this additional merge commit or merging it manually (bypassing our branch protections to enforce CI). Signed-off-by: Michael Weiss <[email protected]>
christophprokop
approved these changes
Jan 14, 2025
Merged
via the queue into
science-computing:master
with commit Jan 14, 2025
778d37f
11 of 13 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had
PackageVersionConstraintbut it was basically identical toPackageVersionprefixed with a=character as only the constraint=for exact matches was supported.The CLI used
PackageVersionConstraintin a few places (likesource download) which made things worse as this inconsistency resulted in some version parameters requiring=in front of the version and some rejecting it.I extended
PackageVersionConstraintto allow for the constraint to be omitted which then performs partial version matching (so, e.g.,1will match1.2.3and1.42).Other constraints could be added in the future. This is basically part one of two: I focused on the code supporting
PackageVersionConstraints where multiple package versions match. It was basically a lot of testing as the type checking cannot ensure that the logic behaves correctly if multiple versions can match now. I also made quite a few related improvements/fixes that I discovered along the way.I needed to make a few compromises to keep this as backward compatible as possible for now and avoid relaxing the dependency specifications in
pkg.tomlfiles yet so that we can decide on a proper version specification before allowing that (otherwise this could create issues when trying to properly determine the newest version as version numbers can use quite exotic formats atm).