Uh oh!
There was an error while loading. Please reload this page.
Add an option to run rustbuild on low priority on Windows and Unix - #42069
Conversation
This is a resurrection of rust-lang#40776, combining their Windows setup with an additional setup on Unix to set the program group's niceness to +10 (low-but-not-lowest priority) when the `low_priority` option is on.
rust-highfive
commented
May 17, 2017
(rust_highfive has picked a reviewer for you, use r? to override) |
porglezomp
commented
May 17, 2017
nice |
aidanhs
commented
May 18, 2017
Thanks for the PR @QuietMisdreavus, we'll make sure @alexcrichton or another reviewer gets to this soon. |
alexcrichton
left a comment
There was a problem hiding this comment.
Looks great to me, thanks @QuietMisdreavus!
| //apparently glibc defines their own enum for this parameter, in a different type | ||
| #[cfg(not(any(target_env = "musl", target_env = "musleabi", target_env = "musleabihf", | ||
| target_os = "emscripten", target_arch = "mips", target_arch = "mipsel")))] | ||
| const PRIO_PGRP: libc::c_uint = libc::PRIO_PGRP as libc::c_uint; |
There was a problem hiding this comment.
It looks like this is defined as
pubconstPRIO_PGRP:::c_int = 1;for all platforms, is the #[cfg] here necessary?
There was a problem hiding this comment.
The re-declaration is necessary because of how setpriority is defined for glibc platforms:
pubfnsetpriority(which:::__priority_which_t,who:::id_t,prio:::c_int) -> ::c_int;Where __priority_which_t is at the top of the file:
pubtype__priority_which_t = ::c_uint;Without this re-declaration, I get a type error when calling setpriority on x86_64-unknown-linux-gnu.
| #openssl-static = false | ||
| # Run the build with low priority | ||
| #low_priority = false |
There was a problem hiding this comment.
In TOML configuration we tend to prefer dashes, could this perhaps be low-priority?
| # known-good version of OpenSSL, compile it, and link it to Cargo. | ||
| #openssl-static = false | ||
| # Run the build with low priority |
There was a problem hiding this comment.
Could you expand this to explain a bit about what this means? Referencing nice here for Unix and "low priority" job object for Windows would probably be sufficient.
alexcrichton
commented
May 18, 2017
@bors: r+ |
bors
commented
May 18, 2017
📌 Commit 4f88106 has been approved by |
…hton Add an option to run rustbuild on low priority on Windows and Unix This is a resurrection of rust-lang#40776, combining their Windows setup with an additional setup on Unix to set the program group's *nice*ness to +10 (low-but-not-lowest priority, mirroring the priority in the Windows setup) when the `low_priority` option is on.
Mark-Simulacrum
commented
May 18, 2017
@bors r- This failed in the rollup: |
Failed on macOS. I totally missed the condition that sends libc into the if #[cfg(any(target_os = "linux",
target_os = "android",
target_os = "emscripten",
target_os = "fuchsia"))]{mod notbsd;pubuseself::notbsd::*;}Gonna make this cfg even more awkward, sadly. (EDIT: For those curious, libc has a |
QuietMisdreavus
commented
May 19, 2017
Looking back at this, it feels like it would be cleaner to patch libc to make |
Mark-Simulacrum
commented
May 19, 2017
Patching libc in this way would be backwards incompatible, though, right? I don't know libc all that well unfortunately (both the C library and the crate) so I can't really comment here. cc @rust-lang/libs, probably. |
sfackler
commented
May 19, 2017
Should be solvable with just |
QuietMisdreavus
commented
May 19, 2017
...huh. I've never seen |
Mark-Simulacrum
commented
May 19, 2017
@bors r=alexchrichton |
bors
commented
May 19, 2017
📌 Commit dd0855d has been approved by |
bors
commented
May 19, 2017
⌛ Testing commit dd0855d with merge 3c94c57... |
bors
commented
May 19, 2017
💔 Test failed - status-appveyor |
bors
commented
May 20, 2017
⌛ Testing commit dd0855d with merge b011276... |
bors
commented
May 20, 2017
💔 Test failed - status-appveyor |
Mark-Simulacrum
commented
May 20, 2017
Another failure (hopefully spurious) to build rustc. @bors retry |
bors
commented
May 20, 2017
⌛ Testing commit dd0855d with merge e848729... |
bors
commented
May 20, 2017
💔 Test failed - status-appveyor |
Mark-Simulacrum
commented
May 20, 2017
Hmm, that's the second time it's failed with rustc just not compiling. I think I've seen this on another PR though so presumably not related? Very concerning though. I'll give it another chance. @bors retry |
bors
commented
May 20, 2017
⌛ Testing commit dd0855d with merge 01951a6... |
Add an option to run rustbuild on low priority on Windows and Unix This is a resurrection of #40776, combining their Windows setup with an additional setup on Unix to set the program group's *nice*ness to +10 (low-but-not-lowest priority, mirroring the priority in the Windows setup) when the `low_priority` option is on.
bors
commented
May 20, 2017
☀️ Test successful - status-appveyor, status-travis |
This is a resurrection of #40776, combining their Windows setup with an additional setup on Unix to set the program group's niceness to +10 (low-but-not-lowest priority, mirroring the priority in the Windows setup) when the
low_priorityoption is on.