Error early if there are spaces in the source directory path, rather than waiting for LLVM to fail

[ckaran]

The build system fails if there are spaces in the path leading to the rust repository. This is a quick fix that detects if there are any spaces in the path leading to the rust repository, and if there are, quits with a message warning the user about the problem.

cc #56650

Signed-off-by: Cem Karan [email protected]

r?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

I feel like globally preventing this in x.py isn't the right way to go; at minimum it should live inside rustbuild itself.

Emitting the hard error as part of the compile::Rustc step would seem reasonable to me -- we don't know that this code is going to run until then, and if you're working on std it seems ok to not lint for you.

[ckaran]

@Mark-Simulacrum I don't know enough about the compiler to really do much more than I have with this PR. If you can point me at the right files to modify, I can take a stab at it, but unfortunately, given my current knowledge level of the internals of the compiler, this is about the best I can do.

The one real advantage to putting it in x.py is that it's really 'in your face'. That way, once the rest of the build system is really fixed, finding and removing this hacky 'fix' will be easy.

[Mark-Simulacrum]

I would expect us to put the check around here. This is all build system modification, not the compiler itself.

[ckaran]

I poked around that code a little bit; are you sure that's the best place to put this check? The only thing that I see here to check over is cargo.env, which I'd do by iterating over all of the output of get_envs(). Unfortunately, the docs for it explicitly state that:

It does not include environment variables that will be inherited by the child process.

which I interpret as meaning that if we don't modify the environment variable, then it will be passed without change to the child process.

Moreover, are we guaranteed that all uses of the path to the current working directory funnel through this chunk of code? I know that everything is funneling through x.py, so if I catch it there, I've caught it everywhere, but I don't know if everything funnels through the cargo variable that goes through that code.

[jyn514]

Moreover, are we guaranteed that all uses of the path to the current working directory funnel through this chunk of code? I know that everything is funneling through x.py, so if I catch it there, I've caught it everywhere, but I don't know if everything funnels through the cargo variable that goes through that code

Right, that's the point - not all invocations of x.py with spaces fail currently, so we shouldn't fail more often after this PR than we did before.

I know that everything is funneling through `x.py

That's changing soon.

[Mark-Simulacrum]

Checking the working directory https://doc.rust-lang.org/nightly/std/env/fn.current_dir.html I think would suffice there.

[ckaran]

Moreover, are we guaranteed that all uses of the path to the current working directory funnel through this chunk of code? I know that everything is funneling through x.py, so if I catch it there, I've caught it everywhere, but I don't know if everything funnels through the cargo variable that goes through that code

Right, that's the point - not all invocations of x.py with spaces fail currently, so we shouldn't fail more often after this PR than we did before.

I know that everything is funneling through `x.py

That's changing soon.

Oooooffff... OK, that's sufficiently bad that the current PR will be useless very quickly. I'm going to close this PR, and if I have more time in the future, I'll dig into where @Mark-Simulacrum suggested within the build system. Unless that's going away soon too?

[ckaran]

Checking the working directory https://doc.rust-lang.org/nightly/std/env/fn.current_dir.html I think would suffice there.

Will this work if someone starts the build from a different directory? E.g., someone is in the parent directory and wants to start the build with <parent_dir>/x.py? I'm thinking about someone trying to build a dockerfile or do something else funky where they don't change directories to the root of the build before starting it.

[jyn514]

We do support building outside of the source directory, yes. Not sure why Mark suggested using the working directory - the thing that makes this go wrong is when llvm-config prints paths with spaces, and the paths it prints are only affected by the build directory and not the source directory.

I guess that's an argument to use builder.out instead of builder.src ... It would be nice if you could run a build where the source dir, build dir, and working dir are all separate and all have spaces to see which breaks first.

[Mark-Simulacrum]

95% of users (or more) are going to build in the working directory; using the build directory would probably be better though. In general I think the point here is that we're just adding a heuristic-based error more than trying to eliminate some bug directly, so if we don't catch all cases that seems ok.

[ckaran]

Yeah, I think that what @Mark-Simulacrum said is probably the right way of thinking about this issue. Not a 100% solution, just a quick fix that deals with a large chunk of the user base's problems.

Honestly, unless everyone is willing to invest the time in completely overhauling the build system all the way down through LLVM, I don't think we can do any better than a bunch of heuristics. And just to be clear, I for one do not want to open the can of worms of trying to rewrite the entire build system from scratch. It is absolutely not worth the time or trouble to do so!

[jyn514]

euclio had a patch with a proper fix to LLVM: #56650 (comment)

it would be nice to rebase that and get it merged at least to https://github.com/rust-lang/llvm-project, even if it's not upstreamed.

[ckaran]

Looks like euclio had a lot of changes that would need to be freshened up. I don't have time to look into it at the moment, unfortunately, so unless someone else is willing to take it on it's probably not going to get fixed.