Ask rust_toolchain for rustc_src in rust_analyzer.bzl

[hlopko]

In this PR we remove hardcoded assumption about where rust-analyzer support code looks for rustc sources, and instead read that information from the rust_toolchain.rustc_src. We also handle the case when rustc sources are actually in the main repository, so there is no need to use the __EXEC_ROOT__/ prefix for its path.

[hlopko]

CC @djmarcin

[illicitonion]

Seems reasonable :)

[hlopko]

@illicitonion only until you'll see I just added rustc srcs back after your PR :))

Now more seriously we'll need some way of including rustc sources for some targets on the CI. I can see how an env var could be used for that, what do you think?

[illicitonion]

@illicitonion only until you'll see I just added rustc srcs back after your PR :))

Now more seriously we'll need some way of including rustc sources for some targets on the CI. I can see how an env var could be used for that, what do you think?

Personally I'm fine with rules_rust always fetching the rustc srcs, as long as it doesn't force consumers of rules_rust to also always fetch them...

[UebelAndre]

Personally I'm fine with rules_rust always fetching the rustc srcs, as long as it doesn't force consumers of rules_rust to also always fetch them...

I would love to only fetch Rust srcs when needed (which is locally). I don't think there's a reason to fetch them in CI (of projects using rules_rust) so would like to have a mechanism for controlling this. I think there's a lot of value in rust_analyzer so I think it's fine to default to downloading the sources and allow for an 'opt-out' flag.

[hlopko]

Personally I'm fine with rules_rust always fetching the rustc srcs, as long as it doesn't force consumers of rules_rust to also always fetch them...

Oh I'm glad to hear that, that simplifies a lot and makes a perfect sense to me.

I would love to only fetch Rust srcs when needed (which is locally). I don't think there's a reason to fetch them in CI (of projects using rules_rust) so would like to have a mechanism for controlling this. I think there's a lot of value in rust_analyzer so I think it's fine to default to downloading the sources and allow for an 'opt-out' flag.

If I understand you correctly you're saying that users of rules_rust should not downloads rustc sources by default, both locally and on the CI, but there should be a way to download sources for people wanting to use rust analyzer (and that there is rarely a need to download rustc sources on their CI). I fully agree with these points.

What this PR does is only downloading rustc sources for local development of rules_rust, and also for CI of rules_rust (because we need test coverage on the CI for the rust analyzer support).

So it seems to me we're in full synergy? :)

[UebelAndre]

What this PR does is only downloading rustc sources for local development of rules_rust, and also for CI of rules_rust (because we need test coverage on the CI for the rust analyzer support).

What about local development of another project which uses rules_rust but is not actually the rules? What does that workflow look like?

[hlopko]

That project has to write its own WORKSPACE file and in it specify which rust toolchains to use (and therefore whether and how to download rustc sources). WORKSPACE files are not used when the repository is an external repo, they are only used when the repository is the main one.

[UebelAndre]

That project has to write its own WORKSPACE file and in it specify which rust toolchains to use (and therefore whether and how to download rustc sources). WORKSPACE files are not used when the repository is an external repo, they are only used when the repository is the main one.

Yeah, so the thing I'm saying is the new include_rust_srcs flag is not a flexible enough solution. I don't know of any use of the rust sources outside of local development. So as I write the WORKSPACE file for my project, I'll set include_rust_srcs = True and forever incur the cost of fetching those. Again, this is fine locally because I plan to use those for rust_analyzer but it's a cost to CI where each build now downloads these sources that are otherwise useless. I should be able to specify in a .bazelrc whether or not I want to set include_rust_srcs = True so local developers can have functional rust_analyzer and CI can efficiently build and test code.

[UebelAndre]

#541 is a good example of what I'm thinking. Where something settable in a .bazelrc file can toggle this behavior.

[hlopko]

Ah I understand now. Yeah, having an environment variable to include sources that you set locally, or a variable to not include sources, that you set on the CI, sound like a standard solution for this problem. It's an issue a bit orthogonal to this PR, let's address it in a separate PR (do you want to give it a try? :).