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

feat: Support downloading nightly rustc-dev components #349

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

Arm1stice
Copy link
Contributor

Working with the Rust compiler crates requires the rustc-dev component. This is distributed separately from the standard library. This PR adds a new option to download the rustc-dev component which defaults to False. The component is only distributed with nightly Rust, so the version option must be set to "nightly".

Comment on lines 421 to 429
for target_triple in [ctx.attr.exec_triple]:
BUILD_components.append(_load_rust_stdlib(ctx, target_triple))
if ctx.attr.dev_components:
# No BUILD file is needed because components will match the stdlib
# BUILD file globs
_load_rustc_dev_nightly(ctx, target_triple)

for target_triple in ctx.attr.extra_target_triples:
BUILD_components.append(_load_rust_stdlib(ctx, target_triple))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx.attr.extra_target_triples contains the wasm target. While there is a stdlib package, there are no rustc-dev components shipped for this target. Because of this, I split this into two different loops. This means the if statement in the first loop only has to check if dev_components is True. Another option might be to keep one loop, and have the if statement be:

if ctx.attr.dev_components and target_triple not in ctx.attr.extra_target_triples:

I prefer the two loops personally, but I can also switch to the longer if statement if there is a strong preference for it by the maintainers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After reading the code, I think one loop will make reading the code easier, and adding a comment on why excluding extra_target_triples make sense (e.g. "it contains wasm and the likes, which does not have a dev component")

Copy link
Collaborator

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

LGTM

rust/repositories.bzl Outdated Show resolved Hide resolved
Comment on lines 421 to 429
for target_triple in [ctx.attr.exec_triple]:
BUILD_components.append(_load_rust_stdlib(ctx, target_triple))
if ctx.attr.dev_components:
# No BUILD file is needed because components will match the stdlib
# BUILD file globs
_load_rustc_dev_nightly(ctx, target_triple)

for target_triple in ctx.attr.extra_target_triples:
BUILD_components.append(_load_rust_stdlib(ctx, target_triple))
Copy link
Collaborator

Choose a reason for hiding this comment

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

After reading the code, I think one loop will make reading the code easier, and adding a comment on why excluding extra_target_triples make sense (e.g. "it contains wasm and the likes, which does not have a dev component")

@Arm1stice
Copy link
Contributor Author

New commits address @damienmg's comments

@Arm1stice
Copy link
Contributor Author

Fixed the broken build

@damienmg damienmg merged commit 264fd66 into bazelbuild:master Jun 23, 2020
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.

None yet

4 participants