Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for negative --jobs parameter, counting backwards from max CPUs. #9221

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,14 @@ impl BuildConfig {
/// * `target.$target.libfoo.metadata`
pub fn new(
config: &Config,
jobs: Option<u32>,
jobs: Option<i32>,
requested_targets: &[String],
mode: CompileMode,
) -> CargoResult<BuildConfig> {
let cfg = config.build_config()?;
let requested_kinds = CompileKind::from_requested_targets(config, requested_targets)?;
if jobs == Some(0) {
anyhow::bail!("jobs must be at least 1")
anyhow::bail!("jobs must not be zero")
}
if jobs.is_some() && config.jobserver_from_env().is_some() {
config.shell().warn(
Expand All @@ -66,7 +66,11 @@ impl BuildConfig {
its environment, ignoring the `-j` parameter",
)?;
}
let jobs = jobs.or(cfg.jobs).unwrap_or(::num_cpus::get() as u32);
let jobs = match jobs.or(cfg.jobs) {
None => ::num_cpus::get() as u32,
Some(j) if j < 0 => (::num_cpus::get() as i32 + j).max(1) as u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd personally prefer that this return an error if the result is less than one rather than clamping to one

Copy link
Contributor Author

@orlp orlp Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcrichton I think it's better if not, suppose that a build script contains -j -2 but is then run on a dual-core machine, now your build breaks. Also, -j 1000 doesn't give an error either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm this is something I'm worried about though in that it's sort of hidden behavior. Using -j100 when you have 2 cpus just means "spawn up to 100 things at the same time" which is always possible, you're basically asking the kernel to do tons of scheduling for you. If the request ends up being "spawn at most -N" tasks I'm not sure Cargo can reasonbly handle that and I would prefer we be conservative and return an error.

If in the future, though, the script case is common enough and the error is just getting in everyone's way we could change it to clamp to 1

Some(j) => j as u32,
};

Ok(BuildConfig {
requested_kinds,
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/core/compiler/context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
let jobserver = match bcx.config.jobserver_from_env() {
Some(c) => c.clone(),
None => {
let client = Client::new(bcx.build_config.jobs as usize)
.chain_err(|| "failed to create jobserver")?;
let client =
Client::new(bcx.jobs() as usize).chain_err(|| "failed to create jobserver")?;
client.acquire_raw()?;
client
}
Expand Down Expand Up @@ -558,7 +558,7 @@ impl<'a, 'cfg> Context<'a, 'cfg> {
}

pub fn new_jobserver(&mut self) -> CargoResult<Client> {
let tokens = self.bcx.build_config.jobs as usize;
let tokens = self.bcx.jobs() as usize;
let client = Client::new(tokens).chain_err(|| "failed to create jobserver")?;

// Drain the client fully
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/timings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ impl<'cfg> Timings<'cfg> {
self.total_dirty,
self.total_fresh + self.total_dirty,
max_concurrency,
bcx.build_config.jobs,
bcx.jobs(),
num_cpus::get(),
self.start_str,
total_time,
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub struct PackageOpts<'cfg> {
pub check_metadata: bool,
pub allow_dirty: bool,
pub verify: bool,
pub jobs: Option<u32>,
pub jobs: Option<i32>,
pub targets: Vec<String>,
pub features: Vec<String>,
pub all_features: bool,
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub struct PublishOpts<'cfg> {
pub index: Option<String>,
pub verify: bool,
pub allow_dirty: bool,
pub jobs: Option<u32>,
pub jobs: Option<i32>,
pub targets: Vec<String>,
pub dry_run: bool,
pub registry: Option<String>,
Expand Down
17 changes: 14 additions & 3 deletions src/cargo/util/command_prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ pub trait AppExt: Sized {
self._arg(
opt("jobs", "Number of parallel jobs, defaults to # of CPUs")
.short("j")
.value_name("N"),
.value_name("N")
.allow_hyphen_values(true),
)
}

Expand Down Expand Up @@ -287,6 +288,16 @@ pub trait ArgMatchesExt {
Ok(arg)
}

fn value_of_i32(&self, name: &str) -> CargoResult<Option<i32>> {
let arg = match self._value_of(name) {
None => None,
Some(arg) => Some(arg.parse::<i32>().map_err(|_| {
clap::Error::value_validation_auto(format!("could not parse `{}` as a number", arg))
})?),
};
Ok(arg)
}

/// Returns value of the `name` command-line argument as an absolute path
fn value_of_path(&self, name: &str, config: &Config) -> Option<PathBuf> {
self._value_of(name).map(|path| config.cwd().join(path))
Expand Down Expand Up @@ -320,8 +331,8 @@ pub trait ArgMatchesExt {
Ok(ws)
}

fn jobs(&self) -> CargoResult<Option<u32>> {
self.value_of_u32("jobs")
fn jobs(&self) -> CargoResult<Option<i32>> {
self.value_of_i32("jobs")
}

fn targets(&self) -> Vec<String> {
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/util/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1874,7 +1874,7 @@ pub struct CargoBuildConfig {
pub target_dir: Option<ConfigRelativePath>,
pub incremental: Option<bool>,
pub target: Option<ConfigRelativePath>,
pub jobs: Option<u32>,
pub jobs: Option<i32>,
pub rustflags: Option<StringList>,
pub rustdocflags: Option<StringList>,
pub rustc_wrapper: Option<PathBuf>,
Expand Down
26 changes: 0 additions & 26 deletions tests/testsuite/bad_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,32 +138,6 @@ Caused by:
.run();
}

#[cargo_test]
fn bad_cargo_config_jobs() {
let p = project()
.file("src/lib.rs", "")
.file(
".cargo/config",
r#"
[build]
jobs = -1
"#,
)
.build();
p.cargo("build -v")
.with_status(101)
.with_stderr(
"\
[ERROR] error in [..].cargo/config: \
could not load config key `build.jobs`

Caused by:
invalid value: integer `-1`, expected u32
",
)
.run();
}

#[cargo_test]
fn invalid_global_config() {
let p = project()
Expand Down
20 changes: 15 additions & 5 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4719,18 +4719,28 @@ fn good_cargo_config_jobs() {
p.cargo("build -v").run();
}

#[cargo_test]
fn good_jobs() {
let p = project()
.file("Cargo.toml", &basic_bin_manifest("foo"))
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("build --jobs 1").run();

p.cargo("build --jobs -1").run();
}

#[cargo_test]
fn invalid_jobs() {
let p = project()
.file("Cargo.toml", &basic_bin_manifest("foo"))
.file("src/foo.rs", &main_file(r#""i am foo""#, &[]))
.build();

p.cargo("build --jobs -1")
.with_status(1)
.with_stderr_contains(
"error: Found argument '-1' which wasn't expected, or isn't valid in this context",
)
p.cargo("build --jobs 0")
.with_status(101)
.with_stderr_contains("error: jobs must not be zero")
.run();

p.cargo("build --jobs over9000")
Expand Down