rustbuild changes to build LLVM #32599
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| } | ||
|
|
||
| #[cfg(windows)] | ||
| pub fn build_path(path: &str) -> PathBuf { |
There was a problem hiding this comment.
Hm this seems very suspicious about how this would be called, how are MSYS paths leaking into the configuration here? Are the makefiles encoding MSYS paths?
Also, this can probably be done in Rust like sanitize_sh rather than forking out to cygpath, right?
There was a problem hiding this comment.
The configure script produces MSYS path, so we need to convert them when importing config.mk.
I'm not sure what the logic is to convert to Windows paths, so I'd rather avoid writing that.
There was a problem hiding this comment.
The logic should be pretty simple, right? /a/foo translates to A:\foo?
There was a problem hiding this comment.
That's only the Windows paths in msys form. There are also things like /home and even /h/path where /h as a directory and not a drive.
There was a problem hiding this comment.
Right so this can cause problems, but this happens on every startup of the build system, so it would be best to spawn fewer processes and just catch up on edge cases over time. The vast majority of these paths will just be /c/foo or /other that we need to fix (which are all trivial to fix)
There was a problem hiding this comment.
Or actually, even better, we should change the configure script to not emit MSYS paths, but rather use cygpath in there.
There was a problem hiding this comment.
I don't want to touch that script though.
There was a problem hiding this comment.
I don't think we can stomach all of these process spawns on every invocation of the build system, so this needs to be handled in some method.
| @@ -180,6 +180,7 @@ def parse_nightly_dates(self): | |||
|
|
|||
| def build_bootstrap(self): | |||
| env = os.environ.copy() | |||
| env["CARGO_HOME"] = os.path.join(self.build_dir, "cargo") | |||
There was a problem hiding this comment.
How come this was added? In theory this should pick up the same CARGO_HOME that it inherits?
There was a problem hiding this comment.
This was added so rustbuild doesn't write to directories outside the build directory and the rust source.
There was a problem hiding this comment.
Let's leave this out for now as it seems unrelated to this change, and this can always be set externally or we can add a configuration option for it.
| @@ -78,6 +79,24 @@ pub fn check(build: &mut Build) { | |||
| panic!("the iOS target is only supported on OSX"); | |||
| } | |||
|
|
|||
| if cfg!(windows) { | |||
There was a problem hiding this comment.
Could you add a comment to this explaining that this is just ensuring that we can run binaries like llvm-config and the compiler by ensuring that the dynamically linked libraries are in PATH?
There was a problem hiding this comment.
This also seems relevant for all platforms (not just Windows), so perhaps this could create a variable like ldpath to handle the Unix/OSX cases?
There was a problem hiding this comment.
This isn't needed on Linux atleast, since it links using full paths and not just the filename,
There was a problem hiding this comment.
Isn't this needed for running llvm-config at all? Or running the compiler if it's dynamically linked? I may be misunderstanding the purpose for this though?
There was a problem hiding this comment.
It is not need for llvm-config on Windows, since it's in the directory with the DLLs. It is needed for executables which are not in the LLVM's binary directory which link dynamically to LLVM (like rustc).
There was a problem hiding this comment.
Right, but that's not a Windows specific limitation, right? If rustc ever dynamically links to LLVM (on any platform) we need the library directory to be in the dynamic library path?
It looks like the difference is just that the dynamic library directory on Windows happens to be the same as the binaries directory?
There was a problem hiding this comment.
It is Windows specific, as you link with filenames on Windows and not paths.
There was a problem hiding this comment.
Sorry I think I'm not being clear enough. So if you're liking with an external LLVM that is itself dynamically linked, then the only hope we have of working correctly is to ensure that the directory with that LLVM dynamic library is in the runtime linker lookup path. On Windows this just happens to be PATH but on Linux/OSX this id LD_LIBRARY_PATH and DYLD_LIBRARY_PATH.
Which is to say, if we link to a dynamically linked LLVM on any platform, we need to run this logic to ensure that the relevant directory is in the relevant environment variable.
|
☔ The latest upstream changes (presumably #32751) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing due to inactivity, but feel free to resubmit with a rebase! |
This fixes issues with --llvm-root in msys2.