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.

Sign up for GitHub

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

Jump to bottom

Replace Targets and lists of files in rust_toolchain with depsets #1109

Merged
merged 7 commits into from
Jan 31, 2022
Merged

Replace Targets and lists of files in rust_toolchain with depsets #1109

merged 7 commits into from
Jan 31, 2022

Conversation

UebelAndre
Copy link
Collaborator

This should simplify the API of rust_toolchain and help avoid confusion for users defining their own toolchains and rules.

@UebelAndre UebelAndre marked this pull request as ready for review January 27, 2022 04:48
@UebelAndre
Copy link
Collaborator Author

Not sure if this is good toolchain hygiene or not to try to reduce the toolchain values to files and depsets. It feels like it but happy to learn better practices.

Also, not sure if this should have an incompatibility flag. Happy to add that as well if the change makes sense.

@UebelAndre UebelAndre changed the title Replace lists of files and Targets in rust_toolchain with depsets Replace Targets and lists of files in rust_toolchain with depsets Jan 27, 2022
hlopko
hlopko reviewed Jan 27, 2022
View reviewed changes
rust/private/rustc.bzl Show resolved Hide resolved
rust/private/rustc.bzl Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
rust/toolchain.bzl Outdated Show resolved Hide resolved
@hlopko
Copy link
Member

hlopko commented Jan 27, 2022

In general, why people end up using Target vs a depset of files is that there is the label, and that there are the providers. I think only having a depset of files in the RustToolchainInfo is a good idea and it will simplify how rules use the toolchain (it will force us to extract all the necessary information from providers and expose only simple depset). So I think I'm in favor of this change. Thank you!

@UebelAndre
Copy link
Collaborator Author

@hlopko I have the changes ready, just want to double check they won't introduce a regression

@UebelAndre UebelAndre mentioned this pull request Jan 29, 2022
Merged
@UebelAndre
Copy link
Collaborator Author

@hlopko I opened #1115 just because I'm not sure how the changes related to dirname are gonna pan out. It should make it easier to revert if there's an issue.

hlopko
hlopko reviewed Jan 31, 2022
View reviewed changes
rust/toolchain.bzl Outdated Show resolved Hide resolved
hlopko
hlopko reviewed Jan 31, 2022
View reviewed changes
rust/private/rustc.bzl Outdated Show resolved Hide resolved
hlopko
hlopko approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@hlopko hlopko left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we need an incompatible flag here, because the actual API of the Rust ToolchainInfo is only used by the implementation of rustc_compile_action. Thanks for this change! Only nits remaining.

@UebelAndre UebelAndre merged commit d5ab414 into bazelbuild:main Jan 31, 2022
cfredric pushed a commit to cfredric/rules_rust that referenced this pull request Feb 16, 2022
@UebelAndre @cfredric
Replace Targets and lists of files in rust_toolchain with depsets (b…
f9ef71c 
…azelbuild#1109)

* Replace lists of files and Targets in `rust_toolchain` with depsets

* Use `find_sysroot` helper.

* Simply use `rust_std` files directly

* Revert "Use `find_sysroot` helper."

This reverts commit 914d159.

* files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlopko hlopko hlopko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@UebelAndre @hlopko