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

create_universe no longer assumes the name of rust_repository repos #805

Merged
merged 7 commits into from
Jun 29, 2021

Conversation

UebelAndre
Copy link
Collaborator

crate_universe doesn't work if the user defines their own rust repositories with custom names. The coupling between these repository names is nice to avoid re-downloading assets but is ultimately a bad assumption to make. This change makes crate_universe isolated and in the future we can find ways of optimizing this fetch (though, the fetch shouldn't be an issue since we're using Bazel's HTTP downloader which would cache the artifact. The only time spent is in extracting the tar files).

@illicitonion
Copy link
Collaborator

Is it possible to add arguments which take labels pointing at the right artifacts which can be already-loaded using rules_rust's workspace set-up, rather duplicating it all?

@UebelAndre
Copy link
Collaborator Author

Is it possible to add arguments which take labels pointing at the right artifacts which can be already-loaded using rules_rust's workspace set-up, rather duplicating it all?

Possibly? The thing I wanted to avoid is having crate_universe take advantage of internal knowledge of the repository rules or force users to go understand them to be able to write their WORKSPACE files. When you say "which takes labels", do you expect that users would provide maybe a label_keyed_string_dict mapping the cargo+rustc binary targets to a triple? Or that they'd provide a repository name for the toolchain repository? Or perhaps something else I'm not interpreting correctly?

@illicitonion
Copy link
Collaborator

I'm not sure what kind of implementation I'm imagining to be honest... I guess cargo_universe could take in a dict[str, str] of triple to toolchain name (and default this to DEFAULT_TOOLCHAIN_TRIPLES), and we could pass that dict as an additional param to get_host_info to replace the currently hard-coded strings?

@UebelAndre
Copy link
Collaborator Author

I'm not sure what kind of implementation I'm imagining to be honest... I guess cargo_universe could take in a dict[str, str] of triple to toolchain name (and default this to DEFAULT_TOOLCHAIN_TRIPLES), and we could pass that dict as an additional param to get_host_info to replace the currently hard-coded strings?

Would it be acceptable to allow the user to pass a dict of triple -> repository but have that be an optional parameter where the rule would fallback to using it's own binaries when they're not provided? I feel like knowledge about repository rules is more internal knowledge that the rule shouldn't depend on.

@illicitonion
Copy link
Collaborator

I'm not sure what kind of implementation I'm imagining to be honest... I guess cargo_universe could take in a dict[str, str] of triple to toolchain name (and default this to DEFAULT_TOOLCHAIN_TRIPLES), and we could pass that dict as an additional param to get_host_info to replace the currently hard-coded strings?

Would it be acceptable to allow the user to pass a dict of triple -> repository but have that be an optional parameter where the rule would fallback to using it's own binaries when they're not provided?

What does "its own binaries" mean? I'd expect that if you don't specify an explicit value for the arg, it would use whatever rules_rust would use if you just called rust_repositories() in your WORKSPACE...

I guess my answer is: If by "its own binaries" you mean "its own binaries, which happen to be the same ones as rules_rust assuming you're not configuring it specially", then yes?

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Works for me! Thanks

crate_universe/bootstrap.bzl Outdated Show resolved Hide resolved
crate_universe/defs.bzl Outdated Show resolved Hide resolved
crate_universe/private/util.bzl Outdated Show resolved Hide resolved
@UebelAndre UebelAndre merged commit 72c591d into bazelbuild:main Jun 29, 2021
@UebelAndre UebelAndre deleted the crate branch June 29, 2021 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants