Use response files for rustc #7762
Conversation
This would workaround windows command line length limits when using lots and lots of features. rust-lang#7759
It turns out none of the following work:
rustdoc @...
cargo @...
cargo fix @...
And build_base_args gets used on more than just rustc commands.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
Thanks for the PR! Can this use a similar strategy to rustc's approach to invoking the linker though? That would entail actually executing a process and then catching specific errors to fall back to a response file, rather than using a response file unconditionally. |
|
The end result being response files only get used when the command line length limits are actually exceeded? Sure! Introducing rarely exercised edge cases like that makes me a bit nervous, but it'd fix the rustc 1.41 dependency - and if it's already being done for linking, then it sounds like I'm in good company ;) |
|
Yeah the general idea in rustc is to try to use the battle-tested "just call the linker first" idiom first and only if necessary fall back to using response files. In theory it's a tad bit faster too since there's no file to create, write, or clean up. The main goal though is robustness where the CLI approach is tried-and-true. |
Since we'll only resort to response files when we've already failing, it no longer makes sense to configure this per call site in a potentially fiddly manner. As such, this also reverts many of my earlier changes.
src/cargo/util/process_builder.rs
Outdated
| let exit = command.status().chain_err(|| { | ||
| process_error(&format!("could not execute process {}", self), None, None) | ||
| })?; | ||
| let exit = match self.build_command().status() { |
There was a problem hiding this comment.
Could this dance perhaps be folded into an internal spawn() method? That way this wouldn't have to be duplicated across exec, exec_with_output, and exec_with_streaming
There was a problem hiding this comment.
Additionally, the pattern of try-then-if-failed-try-again didn't actually work out for rustc on Windows so I think we'll want to tweak the logic to roughly match what rustc does, unconditionally using a response file for "very likely to fail" requests on Windows.
There was a problem hiding this comment.
Could this dance perhaps be folded into an internal spawn() method? That way this wouldn't have to be duplicated across exec, exec_with_output, and exec_with_streaming
f3ff04b folds what can be folded, but it's actually a net increase to the amount of code. Each of those fns uses slightly different entry points (status, output, spawn) and slightly different piping as a result.
The new status/output fns only have a single call site each, but at least they make the error chaining easier...? I'd love to have a better alternative to spawn_with's callback, but Stdio doesn't seem to be clonable...
Additionally, the pattern of try-then-if-failed-try-again didn't actually work out for rustc on Windows so I think we'll want to tweak the logic to roughly match what rustc does, unconditionally using a response file for "very likely to fail" requests on Windows.
It's been working in my testing, but I suppose the concern is a RUSTC_WRAPPER.cmd ?
src/cargo/util/process_builder.rs
Outdated
| } | ||
| // cmd.exe can handle up to 8k work of args, this leaves some headroom if using a .cmd rustc wrapper. | ||
| let mut cmd_remaining: usize = 1024 * 6; | ||
| let mut response_file = tempfile::NamedTempFile::new()?; |
There was a problem hiding this comment.
Could this place the temporary file in the target folder so we don't spray too many temporary files throughout /tmp if ctrl-c is used?
There was a problem hiding this comment.
(this may require moving this logic of try-with-response-file to somewhere that's more rustc-specific rather than in general for all processes spawned by Cargo)
| command.current_dir(cwd); | ||
| } | ||
| // cmd.exe can handle up to 8k work of args, this leaves some headroom if using a .cmd rustc wrapper. | ||
| let mut cmd_remaining: usize = 1024 * 6; |
There was a problem hiding this comment.
I think it's fine to avoid this heuristic and simply pass everything through a response file once we've failed once.
There was a problem hiding this comment.
A RUSTC_WRAPPER is likely to fail to understand response files. This:
some_rustc_wrapper path/to/rustc.exe --flag --flag --flag ... @remaining_argsIs likely to work, while this:
some_rustc_wrapper @all_args_including_path_to_rustc_exeWill almost certainly break. Perhaps that's fine - or even preferred? Loudly breaking instead of silently failing to recognize/modify flags found in @remaining_args ?
There was a problem hiding this comment.
Er yes that's a good point, could the arguments to rustc be handled a bit differently in that case so only rustc's arguments get into the @-file?
This may be a bit too low-level of a location to put the insertion of the @-file
| let mut arg = OsString::from("@"); | ||
| arg.push(response_file.path()); | ||
| command.arg(arg); | ||
| for (k, v) in &self.env { |
There was a problem hiding this comment.
This is likely duplicated (as well as the jobserver bit below) with a different method in this struct, can those be deduplicated?
|
☔ The latest upstream changes (presumably #7776) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Ping @MaulingMonkey do you think you'll be able to look into some of the remaining comments, and adding some tests? |
ehuss
added
S-waiting-on-author
|
Closing due to inactivity. If you are interested in reviving this, feel free to open a new PR with the concerns addressed. |
Requires rust 1.41 which is not yet stable. If that makes this pull request too early, feel free to close this PR for now, and I can reopen/rebase/squash/??? when 1.41 lands.
Partially implements #7759 , allowing you to
cargo build --features "big"on a crate where "big" implies thousands of other individual features, which would otherwise exceed command line length limits on windows when cargo passes them individually to rustc. I say "partially implements" as several other commands such as rustdoc and cargo itself don't yet recognize response files either -cargo doc --features "big"avoids using response files, and will thus still fail when expanding to thousands of features. Perhaps the issue here can be closed anyways if this is merged, since that's less of an issue in cargo and more of an issue in those other commands?