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

feat: add minimum supported axoupdater check (library) #1432

Merged
merged 12 commits into from
Oct 7, 2024
Merged

Conversation

mistydemeo
Copy link
Contributor

This adds a check when building Rust software which uses axoupdater as a library. If the user is using a version older than what this version of cargo-dist was for, it emits a warning at buildtime. I tested it on cargo-dist itself by bumping the const to something that doesn't exist, and confirmed it works:

 WARN Your project uses axoupdater as a library. We've detected that you're using 0.7.2, but this version of cargo-dist expects a version of at least 0.10.0 (or greater). Please consider updating to ensure your users don't experience inconsistent update behaviour.

To support this, I added support in axoproject for detecting if axoupdater is present and, if it is, its version is recorded. I briefly considered tracking the full dep tree in the axoproject info but that seemed like overkill.

Fixes #1392.

@mistydemeo mistydemeo requested a review from Gankra September 27, 2024 00:13
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Approved but some changes to make

@mistydemeo
Copy link
Contributor Author

Pushed up a version with some changes.

I realized that since we were already iterating over direct packages in the workspace and not transitive dependencies (and since Gankra's suggestion was to ignore unexpected packages), we could do this a little easier by putting that dependency info on each package rather than on the workspace. Makes it easier to decide if we need to care about that axoupdater version per-package. We don't - currently - know of any projects that actually transitively depend on axoupdater, I suppose.

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

some comments, mostly focused on: if we error on a transitive dep, we need to give the user more info. also makes a suggestion that we link to a docs page on this with the error.

With the first clause in the previous verison, we'd end up reporting that
axoupdater always came from the package itself instead of tracking the
dependency it really came by. Tracking only the subdeps works for both
direct and transitive dependencies.
@mistydemeo
Copy link
Contributor Author

Fixed a bug that caused us to report the wrong transitive dep version. Works fine for both direct and transitive deps now.

Direct:

  × Your project (cargo-dist) uses axoupdater as a library, but the version specified (0.7.2) is older than the
  │ minimum supported version (0.10.0). (The dependency comes via cargo-dist in the dependency tree.)
  help: https://opensource.axo.dev/cargo-dist/book/installers/updater.html#minimum-supported-updater-version-
        checking

Transitive:

  × Your project (deptest) uses axoupdater as a library, but the version specified (0.7.2) is older than the minimum
  │ supported version (0.10.0). (The dependency comes via cargo-dist in the dependency tree.)
  help: https://opensource.axo.dev/cargo-dist/book/installers/updater.html#minimum-supported-updater-version-
        checking

This avoids miette linebreaking the URL, which should make it easier to
copy/paste or cmd+click.
@mistydemeo mistydemeo merged commit 3c46e46 into main Oct 7, 2024
16 checks passed
@mistydemeo mistydemeo deleted the track-deps branch October 7, 2024 19:58
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 "minimum supported axoupdater version" checks
3 participants