-
Notifications
You must be signed in to change notification settings - Fork 406
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
rust_toolchain
now generates a Rust sysroot
#1119
Conversation
e5cfc0d
to
bfb4270
Compare
Adding @krasimirgg who is doing something similar on our end :) |
We'd like to do some more extensive internal testing of this PR. |
We hit a tricky issue with this PR internally. This PR causes some unstable clients that use
Compiling something like this in linux that refers to the Now, suppose that instead Now this PR seems to trigger this pre-existing issue in remote caching builds in certain configurations where the @hlopko had an idea that may help us out: In this PR we always generate symlinks for sysroot files. Instead, could this PR be enchanted so that if the inputs are already in a directory tree that resembles sysroot, we use them directly without creating symlinks? Alternatively as a bit of a workaround, wdyt about adding a new attribute, say |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We hit a tricky issue with this PR internally, caused by a misfortunate combination of factors. This PR causes some unstable clients that use extern crate rustc_driver
to fail to build under a configuration with remote caching. An example is Kythe: https://osscs.corp.google.com/kythe/kythe/+/master:kythe/rust/extractor/src/lib.rs;l=16?q=rustc_driver&ss=kythe%2Fkythe:kythe%2Frust%2F,
but I've got a smaller example here.
So "-L" "/usr/local/google/home/krasimir/.cache/bazel/_bazel_krasimir/c736e0c317c9884eaf6f814e37ef83da/external/rust_linux_x86_64/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-lrustc_driver-cbd31b920a2b2b82"
I don't think I like the forked behavior. I'd rather the toolchains work the same way all the time and not need to do any work to detect a directory structure. Creating symlinks should be very fast but alternatively a directory could be created where files are copied into place if symlinks are a concern. WDYT? |
Sounds roughly along the lines of the symlink pattern approach from my previous comment:
Are you thinking about something like this, or something where we always copy everything instead of using symlinks ever? I personally don't have a sense if copying all the toolchain artifacts (some of which are quite large files) like that may become prohibitively expensive in some situations; @hlopko may have an opinion on this. |
I'm thinking that we would always copy the same files into the sysroot. My expectation is that instead of having an extra attribute for determining what to copy (or symlink), that would be determined by the attributes themselves. Meaning if you don't want something in your sysroot, don't include it in the toolchain (behavior may vary with specific attributes but this would generally be the rule). |
@krasimirgg Also, thank you so much for the quick replies! Much appreciated 🙏 |
The subtlety is in the distinction between the [what to copy] and [what to symlink] cases. If we go with the approach to copy everything coming from all attributes, we don't have the If we go the approach to use symlinks (for some of the stuff coming from the attributes), we would still want |
I guess, it doesn't matter to me if symlinks or copies are used, I don't want the behavior to be configurable outside of what is passed to what attribute (e.g. rust_stdlib_filegroup). Though, I'm confused at how a situation like this occurs. I won't be able to test on linux till later today but I'm surprised the solution to a problem like this is not "include the missing file in the sysroot" which I'd expect to be done by expanding the globs here. edit:
Expanding on this a bit, I feel there's already lots of work needed for setting up external crates and thing the configuration tax of using Rust in Bazel is already at capacity. If possible, keeping the toolchain behavior as consistent as possible for users would be better for everyone over all IMO. |
Update: the The current version of this PR looks good (using symlinks for everything, no need to copy anything, no need for a pattern-knob workaround) IMO. |
Oh, I actually did not test the removal of |
The last commit fixes the issue for me but does rely on the |
@krasimirgg do you know why the issues with |
Using the --sysroot flag for rustdoc sounds good to me (just using it with rustc seems problematic).
I don't understand the mechanism that causes the issues with --sysroot while using the rustc_driver in our remote build configurations. I'll double check, but I believe the [equivalent of the] glob patterns we use are correct. |
@hlopko @krasimirgg If there are no outstanding concerns, could I get a final review from each of you? 🙏 😅 |
There was a problem hiding this 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. I just noticed a small nit.
Please wait for @hlopko's review/ approval.
Co-authored-by: Krasimir Georgiev <krasimir@google.com>
@krasimirgg Will do.
Separate from the reviews, I'd still love to learn what you find here. Should I make a new issue for this so it's easier to track? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor things, in general looks good to me, thank you!!
Thank you! :) |
Currently,
rust_toolchain
does not support the use of toolchain components that are not already a part of a valid sysroot structure. This means that if I have arustc
artifact and arust-std
(Rust standard library) artifact, I cannot use them if they were not downloaded into the same repository rule and follow a specific directory structure. This makes toolchain management hard and leads to unnecessary data being downloaded if users want to have toolchains for multiple target platforms as before any 1 toolchain could be used, allrust-std
artifacts (or any other target specific assets) would all need to be downloaded regardless of whether or not that target platform is being built for. This PR aims to solve for allowing users to use individual Rust static assets without needing to do additional maintenance to maintain asset bundles specific to all targets or waste time fetching components that will not be used.