-
Notifications
You must be signed in to change notification settings - Fork 13.3k
avoid rustc_llvm rebuilds when LD_LIBRARY_PATH changes #104375
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
Conversation
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.
This makes sense to me since REAL_LIBRARY_PATH_VAR is still tracked. I'd prefer for Mark to take a look before I approve though.
I believe this is still necessary -- the CI LLVM should apply to your local build, but without this you might pull in the beta LLVM (which is a different LLVM). And, in any case, the CI LLVM should not have changed anything since we aren't always using it. @bors r=jyn514,Mark-Simulacrum |
I am confused, you said we should not do this change and then approved it?
|
…514,Mark-Simulacrum avoid rustc_llvm rebuilds when LD_LIBRARY_PATH changes Currently the fairly slow rustc_llvm build script gets rerun every time LD_LIBRARY_PATH changes. So if one has an IDE setup where LD_LIBRARY_PATH inside the IDE differs from the one on the host, rebuilds happen all the time. With LLVM being downloaded from CI, this REAL_LIBRARY_PATH stuff does not seem needed any more, at least not on my system. But I also don't really understand what happens here. Does a patch like this make sense? r? `@jyn514` `@Mark-Simulacrum` Cc `@bjorn3`
cargo.env("REAL_LIBRARY_PATH_VAR", &util::dylib_path_var()); | ||
if let Some(e) = env::var_os(util::dylib_path_var()) { | ||
cargo.env("REAL_LIBRARY_PATH", e); | ||
if !self.config.llvm_from_ci { |
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.
Oh, I missed this part of the diff.
@bors r- |
I suspect like with other IDE specific differences it's possible that our answer has to involve changing how your IDE invokes x.py (e.g. setting the ld library path to your system one just before). I think the alternative is isolating the changes to the env variable - we should be able to figure out a specific set of things that need to be in the dynamic library path, I suspect, and reduce to that. It's probably pretty annoying to do that though. |
I'll see if I can produce an example that breaks with this, so we can bounce off of that. |
I know my IDE setup is pretty special, so if this doesn't work that's fine and I will add some local hacks. I just thought maybe there is a nice solution here since CI LLVM is less complicated than the case where LLVM is built locally. But if it doesn't work, then I'll find another way. |
Locally built and CI llvm should be the same for linking purposes, I think, at least loosely speaking. |
Currently the fairly slow rustc_llvm build script gets rerun every time LD_LIBRARY_PATH changes. So if one has an IDE setup where LD_LIBRARY_PATH inside the IDE differs from the one on the host, rebuilds happen all the time.
With LLVM being downloaded from CI, this REAL_LIBRARY_PATH stuff does not seem needed any more, at least not on my system. But I also don't really understand what happens here. Does a patch like this make sense?
r? @jyn514 @Mark-Simulacrum
Cc @bjorn3