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

Spawn cargo add in the new command #209

Merged
merged 1 commit into from
Jan 26, 2024

Conversation

tomasol
Copy link
Contributor

@tomasol tomasol commented Jan 25, 2024

Fixes #204
cargo component add now spawns cargo add cargo-component-bindings . This has a few implications:

  • It makes the command a bit slower as it needs to fetch the metadata from the network
  • It uses the latest version by default.
  • When in a workspace, and cargo-component-bindings is already defined as a workspace dependency, it uses the workspace version.

cargo component upgrade was not changed. I believe this command does too much and should be reconsidered.

Tests did not have to be changed, but checking a specific cargo-component-bindings version in tests might be problematic.

@tomasol tomasol marked this pull request as ready for review January 25, 2024 09:07
@peterhuene
Copy link
Member

Hi @tomasol. Thanks again for tackling this issue!

If we merge #212, the bindings crate has been removed entirely; however, it adds a dependency on wit-bindgen to pull in the runtime functions (disables the macro generation code, so it only has one additional transitive dependency).

So I think we should keep this change but update it for adding the dependency on wit-bindgen with --no-default-features and --features realloc.

That way we can remove the hardcoded version added by #212 and automatically pull it from a workspace if it's already there.

@peterhuene peterhuene force-pushed the spawn-cargo-add branch 2 times, most recently from 07ed8c9 to 1064284 Compare January 25, 2024 23:20
@peterhuene
Copy link
Member

I've pushed up the proposed change and fixed a bug where we weren't checking the returned status for a non-successful exit (the workspace related tests should have failed as it wasn't adding the dependency because the workspace was setup before the subprojects were created; just had to fix the ordering in the tests).

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

@tomasol thanks again for the fix!

@peterhuene peterhuene merged commit 39b37d0 into bytecodealliance:main Jan 26, 2024
6 checks passed
@tomasol
Copy link
Contributor Author

tomasol commented Jan 26, 2024

@peterhuene Thanks for improving and merging the PR!

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.

Consider using cargo add in the new command
2 participants