Skip to content

rustc_src is optional #703

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

Merged
merged 2 commits into from
May 11, 2021
Merged

Conversation

illicitonion
Copy link
Collaborator

This is a tar containing a huge number of files (hundreds of thousands)
which can take double-digit seconds to extract.

It's only used for rust analyzer support, so hide this behind a flag
people can opt in to if they want.

@google-cla google-cla bot added the cla: yes label Apr 22, 2021
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Perhaps rust_analyzer should have it's own toolchain which goes and fetches the rustc sources? That might make configuring it a bit clunky but might be warranted and provide a good way to separate the rust_analyzer functionality from core builds. Maybe there's a path forward where rust analyzer is enabled by a bazelrc flag and we'd always define the toolchains in repositories.bzl knowing they'd otherwise never be used without the aspect enabled.

I do feel the amount of time spent fetching dependencies is quite painful though so this is a good direction.

@illicitonion illicitonion force-pushed the optional-rustc-src branch 2 times, most recently from 22ca12a to 93e8cd9 Compare May 6, 2021 15:48
@illicitonion
Copy link
Collaborator Author

I finally got around to addressing these comments - PTAL!

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

What is the user story for having this work with rust-analyzer? I think if we expect users to set some flag to True in their workspace that we should also update documentation for that here at the very least.

@illicitonion
Copy link
Collaborator Author

I added a note to the rust_analyzer docs to tell users they need to set this.

@UebelAndre
Copy link
Collaborator

I was actually thinking something should be added to docs/rust_analyzer.vm. If I'm going to setup rust_analyzer in my existing rules_rust project, I may not see the updated note about rust_repositories. If you add some documentation there I'm happy with this PR 😄

@illicitonion
Copy link
Collaborator Author

Sigh, I edited exactly what you wanted, but I edited the .md file rather than the .vm file so it got reverted by the re-generation. I am looking forward to not putting generated docs in the repo.

Updated the right file this time :)

This is a tar containing a huge number of files (hundreds of thousands)
which can take double-digit seconds to extract.

It's only used for rust analyzer support, so hide this behind a flag
people can opt in to if they want.
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@UebelAndre
Copy link
Collaborator

UebelAndre commented May 11, 2021

Having thought about this a bit more. Could this also be gated by an action_env or something a user could add to their .bazelrc files? Basically, I'm never going to set include_rustc_src = False if I use rust_analyzer in development or I'm gonna awkwardly have a modified WORKSPACE file or something. Maybe --repo_env would work here?

Or perhaps do something for the inverse where a .bazelrc is set for CI to disable fetching these?

@illicitonion
Copy link
Collaborator Author

Having thought about this a bit more. Could this also be gated by an action_env or something a user could add to their .bazelrc files? Basically, I'm never going to set include_rustc_src = False if I use rust_analyzer in development or I'm gonna awkwardly have a modified WORKSPACE file or something. Maybe --repo_env would work here?

Or perhaps do something for the inverse where a .bazelrc is set for CI to disable fetching these?

I put together an alternate patch where a repo env determines whether to do the fetching, and we set the repo env, but it has the down-side that the repository rule is re-run whenever you switch between rust analyzer and non-rust-analyzer bazel usage, which is a pretty bad experience.

I guess a reasonable alternative would be a WORKSPACE file opt-in (as this PR does), and then an env var to specify "No really, I'm never going to want to use rust analyzer" that you could set on CI... But that's starting to feel like a somewhat confusing state-space to need to consider, as a user...

I think @hlopko was talking about looking into why we unconditionally re-fetch toolchain stuff quite so often, which would maybe make my alternate patch more viable.

In the mean time, I'm going to merge this (as discussed offline), we can re-visit when we have a better plan :)

@illicitonion illicitonion merged commit 613f947 into bazelbuild:main May 11, 2021
@illicitonion illicitonion deleted the optional-rustc-src branch May 11, 2021 11:41
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.

3 participants