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(rust_plugin): fix dependency validation logic #903

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Nov 18, 2024

  • Have you signed the CLA?

This pull request fixes the dependency validation logic regarding the "rust-deps" situation. If rustup is installed, then cargo and rustc will return an error code of 1 instead of 127 if no default toolchain is installed (because cargo and rustc will be shims generated by rustup).

Two more tests are added to verify this new validation logic.

Should fix #890.

craft_parts/plugins/rust_plugin.py Show resolved Hide resolved
tests/unit/plugins/test_rust_plugin.py Outdated Show resolved Hide resolved
tests/unit/plugins/test_rust_plugin.py Outdated Show resolved Hide resolved
... if "rust-deps" is in the "after" property (i.e. the user wants to
install the Rust toolchain themselves)
Now the logic goes like this:
* If channel is "none": validates if cargo and rustc are installed
* If channel is other values: validates if rustup is installed
* If "after" contains "rust-deps": validates if cargo and rustc are
  installed
* If "after" contains "rust-deps" and also channel is not "none": throws
  error as this is not a supported configuration
Copy link
Contributor

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@liushuyu
Copy link
Contributor Author

Do I need to rebase the changes for this pull request to merge cleanly?

@mr-cal
Copy link
Contributor

mr-cal commented Nov 27, 2024

Do I need to rebase the changes for this pull request to merge cleanly?

No need, I can merge and land this.

@mr-cal mr-cal merged commit dc79714 into canonical:main Nov 27, 2024
13 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.

Rust plugin always installs the rustup snap, even when trying to use local cargo/rustc
3 participants