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

#10125: Support multiple build targets in .cargo/config.toml #10685

Conversation

mryall-mawson
Copy link
Contributor

@mryall-mawson mryall-mawson commented Jul 14, 2023

changelog: support multiple build.target values in .cargo/config.toml

This is a fix for #10125, where the cargo metadata execution fails when multiple build targets are specified in .cargo/config.toml. Multiple build targets in config.toml were stabilized in Rust 1.64.0, as part of rust-lang/cargo#8176.

As described in my comment on the issue, I have changed buildTarget: String? into buildTargets: List<String> in the CargoConfig, as returned by Cargo.getConfig(), and used in various places which calls that function. Instead of being null, buildTargets is an empty list if build.target is not specified, which is still correctly handled (defaulted to the host triple) by the calling code.

As a result, Cargo.fetchMetadata() now passes --filter-platform multiple times to the cargo metadata call, rather than an empty string, which will correctly filter the dependencies for multiple target platforms, fixing the implementation originally added in #7185.

Tests have been added and updated to exercise this functionality.

This is my first time working on this project, so please feel free to adjust whatever is needed to take this forward.

@mryall-mawson
Copy link
Contributor Author

The most recent commit updates the tests to only run on rust 1.64.0 or higher, which is when multiple targets were stabilized.

I couldn't any mention of the rust versions supported by the plugin, but there is one build with 1.53.0, which I'm taking to be the limit for compatibility.

This change is compatible with versions back to 1.53.0; it's just the tests which require the recent version for the sample config to be valid.

@mryall-mawson
Copy link
Contributor Author

@vlad20012 @Undin would either of you be able to help with review on this one? Looks like you have worked before in the Cargo code I changed for this fix.

It would be great to get any feedback while this work is fresh. I'm happy to implement any changes needed to get this merged. Tests were added, builds are passing. Thanks.

@vlad20012 vlad20012 self-assigned this Jul 20, 2023
@mryall-mawson
Copy link
Contributor Author

Review feedback implemented and pushed on the branch. Waiting for builds to pass.

…toml

- add tests for parsing multiple targets
- change `buildTarget: String?` to `buildTargets: List<String>` in Config object and related function calls
- add test for multiple custom build targets (JSON files)
- use empty/singleton list as relevant defaults instead of null/host triple
@vlad20012 vlad20012 force-pushed the mryall/10125-support-multiple-build-targets branch from f05f30a to cbb9542 Compare July 30, 2023 14:43
@vlad20012 vlad20012 added the fix Pull requests that fix some bug(s) label Jul 30, 2023
@vlad20012
Copy link
Member

Thanks!

bors r+

@intellij-rust-bot intellij-rust-bot added this to In Progress in To test Jul 30, 2023
@bors
Copy link
Contributor

bors bot commented Jul 30, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 45d1382 into intellij-rust:master Jul 30, 2023
25 of 44 checks passed
To test automation moved this from In Progress to Test Jul 30, 2023
@github-actions github-actions bot added this to the v200 milestone Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull requests that fix some bug(s)
Projects
To test
  
Test
Development

Successfully merging this pull request may close these issues.

None yet

2 participants