rustc: Lower link args to @-files on Windows more
#47507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexcrichton
added
the
beta-nominated
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I'm also tagging this as beta-nominated since incremental is in beta now |
src/librustc_trans/back/command.rs
Outdated
| // bytes we'll typically cause them to blow as well. | ||
| // | ||
| // Basically as a result just perform an inflated estimate of what our | ||
| // command line will look like and test if it's > 8192. If all else |
There was a problem hiding this comment.
This seems to be false? The code below compares with 6*1024, which is 6,144. Though it's already fairly clear what we're trying to say.
There was a problem hiding this comment.
Oh that's actually intentional in the sense that we are sort of checking against 8192, I just randomly chose 6k instead to assume some percentage of slop when calculating the length of the command line (I've updated the comment though to hopefully reflect this!)
kennytm
added
the
S-waiting-on-review
|
Thank you, Alex! r=me with the inconsistency that @Mark-Simulacrum mentioned ironed out. |
alexcrichton
force-pushed
the
rerun-bat-scripts
branch
from
d09be7a to
9e50cc8
Compare
|
@bors: r=michaelwoerister |
|
📌 Commit 9e50cc8 has been approved by |
alexcrichton
added
the
T-compiler
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Jan 18, 2018
Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO --HG-- extra : rebase_source : 1739a7570b2e7fe40ead3b301ea20c2fe79f0431
nikomatsakis
added
the
beta-accepted
|
@bors: p=1 (this'll get backported to beta once merged) |
kennytm
added
S-waiting-on-bors
|
⌛ Testing commit 9e50cc8b73d8df9fae1645299a8bf1251e9493d4 with merge c75f68ca414269a3de08c9963657fc2b37a078b1... |
|
💔 Test failed - status-appveyor |
|
⌛ Testing commit 9e50cc8b73d8df9fae1645299a8bf1251e9493d4 with merge 0b574b519c158f160bcc9078c3fc693a7bee22ec... |
|
💔 Test failed - status-appveyor |
alexcrichton
removed
the
beta-nominated
on second thought, not spurious |
alexcrichton
added
the
beta-nominated
alexcrichton
force-pushed
the
rerun-bat-scripts
branch
from
9e50cc8 to
db2c548
Compare
|
@bors: r=michaelwoerister |
|
⌛ Testing commit b3b376878c9614c81227c1ce42a94bf919c2a7d6 with merge 662b582567dd6c29ad3d3801fed747bf27d81b85... |
|
💔 Test failed - status-appveyor |
alexcrichton
force-pushed
the
rerun-bat-scripts
branch
from
b3b3768 to
3ca320c
Compare
|
@bors: r=michaelwoerister |
|
📌 Commit 3ca320c has been approved by |
bors
added a commit
that referenced
this pull request
Jan 21, 2018
…ster rustc: Lower link args to `@`-files on Windows more When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix #46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886 Closes #46999
|
💔 Test failed - status-appveyor |
|
☔ The latest upstream changes (presumably #45684) made this pull request unmergeable. Please resolve the merge conflicts. |
When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix rust-lang#46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886 Closes rust-lang#46999
alexcrichton
force-pushed
the
rerun-bat-scripts
branch
from
3ca320c to
66366f9
Compare
|
I've rebased but this isn't ready for merging, I'm investigating that last failure. |
|
@bors: r=michaelwoerister |
|
📌 Commit 66366f9 has been approved by |
bors
added a commit
that referenced
this pull request
Jan 22, 2018
…ster rustc: Lower link args to `@`-files on Windows more When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix #46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886 Closes #46999
|
☀️ Test successful - status-appveyor, status-travis |
Merged
alexcrichton
removed
the
beta-nominated
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Oct 2, 2019
Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO UltraBlame original commit: 261725e65af9b5d98ff593db3db08632bf019454
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this pull request
Oct 2, 2019
Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO UltraBlame original commit: 261725e65af9b5d98ff593db3db08632bf019454
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Oct 2, 2019
Work around excessive command-line length issues by disabling incremental rust compilation, which is enabled by default outside `cargo --release` starting with Rust 1.24. Incremental rust builds shouldn't help much in automation, where sccache provides the only continuity between build environments. In the meantime, they add a lot of object files to the link line. See rust-lang/rust#47507 about addressing the underlying issue upstream. MozReview-Commit-ID: LRwUj3fhiaO UltraBlame original commit: 261725e65af9b5d98ff593db3db08632bf019454
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When spawning a linker rustc has historically been known to blow OS limits for
the command line being too large, notably on Windows. This is especially true of
incremental compilation where there can be dozens of object files per
compilation. The compiler currently has logic for detecting a failure to spawn
and instead passing arguments via a file instead, but this failure detection
only triggers if a process actually fails to spawn.
Unfortunately on Windows we've got something else to worry about which is
cmd.exe. The compiler may be running a linker throughcmd.exewherecmd.exehas a limit of 8192 on the command line vs 32k onCreateProcess.Moreso rustc actually succeeds in spawning
cmd.exetoday, it's just that afterit's running
cmd.exefails to spawn its child, which rustc doesn't currentlydetect.
Consequently this commit updates the logic for the spawning the linker on
Windows to instead have a heuristic to see if we need to pass arguments via a
file. This heuristic is an overly pessimistic and "inaccurate" calculation which
just calls
lenon a bunch ofOsStringinstances (wherelenis notprecisely the length in u16 elements). This number, when exceeding the 6k
threshold, will force rustc to always pass arguments through a file.
This strategy should avoid us trying to parse the output on Windows of the
linker to see if it successfully spawned yet failed to actually sub-spawn the
linker. We may just be passing arguments through files a little more commonly
now...
The motivation for this commit was a recent bug in Gecko 1 when beta testing,
notably when incremental compilation was enabled it blew out the limit on
cmd.exe. This commit will also fix #46999 as well though as emscripten uses abat script as well (and we're blowing the limit there).
Closes #46999