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

Fix bindings version warning and upgrade command. #187

Merged
merged 2 commits into from Nov 11, 2023

Conversation

peterhuene
Copy link
Member

@peterhuene peterhuene commented Nov 8, 2023

This PR fixes the warning emitted when a mismatched bindings version crate is detected such that it no longer warns when the bindings crate version is compatible, but not the same version (i.e. 0.4.1 when cargo-component is 0.4.0).

Likewise, the upgrade command no longer attempts to downgrade the version of the bindings crate when it would otherwise be compatible.

The upgrade tests no longer attempt the install, which require a long time to just verify that a cargo install cargo-component would succeed, which isn't terribly useful.

The warning and upgrade command now both detect when cargo-component is outdated and instruct the user appropriately.

@jeffparsons
Copy link
Contributor

The upgrade tests no longer attempt the install, which require a long time to just verify that a cargo install cargo-component would succeed, which isn't terribly useful.

I thought the #[cfg(not(test))] achieved this? With the new approach of always passing in --no-install from the tests, I think that means delegation to a new process isn't tested anywhere.

src/commands/upgrade.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@jeffparsons
Copy link
Contributor

(Sorry if my comments here don't quite make sense; I'm posting from mobile in the middle of the night. Maybe I shouldn't do that...)

This commit fixes the warning emitted when a mismatched bindings version crate
is detected such that it no longer warns when the bindings crate version is
compatible, but not the same version (i.e. `0.4.1` when cargo-component is
`0.4.0`).

Likewise, the `upgrade` command no longer attempts to downgrade the version of
the bindings crate when it would otherwise be compatible.

The upgrade tests no longer attempt the install, which require a long time to
just verify that a `cargo install cargo-component` would succeed, which isn't
terribly useful.
* Remove outdated comment in upgrade command.
* Properly handle when the bindings crate is newer than cargo-component:
  * Emit a warning instructing the upgrade of cargo-component.
  * Skip updating `Cargo.toml` for `cargo component upgrade`.
Copy link
Contributor

@jeffparsons jeffparsons left a comment

Choose a reason for hiding this comment

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

This looks good to me! 🎉

I'm not a BA member or anything, though; I don't know if there are any rules for reviews that apply to this repo?

@peterhuene
Copy link
Member Author

As you are a previous contributor, I'm happy to rely on your review, given it is related to the feature you added previously.

I appreciate the review!

@peterhuene peterhuene merged commit 8cfa63f into bytecodealliance:main Nov 11, 2023
6 checks passed
@peterhuene peterhuene deleted the fix-warning branch November 11, 2023 03:38
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.

None yet

2 participants