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

Support a "default" option for build.jobs #11816

Closed
do-not-use-parker-timmerman opened this issue Mar 9, 2023 · 20 comments · Fixed by #12222
Closed

Support a "default" option for build.jobs #11816

do-not-use-parker-timmerman opened this issue Mar 9, 2023 · 20 comments · Fixed by #12222
Assignees
Labels
A-jobserver Area: jobserver, concurrency, parallelism C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` disposition-merge FCP with intent to merge E-easy Experience: Easy finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce

Comments

@do-not-use-parker-timmerman
Copy link

do-not-use-parker-timmerman commented Mar 9, 2023

Problem

There is no way to "unset"/reset to "default, the number of build jobs specified to Cargo.

Consider your team has a .cargo/config.toml located in the root of your repository. You want to set the following:

[build]
jobs = -1

So that on all developer's machines, one core will be left available when running cargo for your computer to do other tasks.

But your CI infrastructure also runs cargo, and you want CI to use all possible cores to makes builds as fast as possible. You can set the CARGO_BUILD_JOBS environment variable, but via the env variable (or --jobs cli option) but there is no way to override the number of jobs back to "default"/num cpus.

Proposed Solution

A solution is to define a new value that represents the default/num_cpus state. Two proposals would be:

  1. Reinterpret 0 jobs as meaning "default"
    a. Today specifying 0 as the number of jobs returns an error, so changing this shouldn't(?) break any flows
  2. Add a new supported value like "d", "default", or "num_cpus" that when specified we use the default value

Personally I'd prefer option 1 as it seems like the easiest, but I'm not sure if there are any flows that rely on cargo returning an error.

Notes

No response

@do-not-use-parker-timmerman do-not-use-parker-timmerman added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Mar 9, 2023
@weihanglo weihanglo added the A-jobserver Area: jobserver, concurrency, parallelism label Mar 9, 2023
@weihanglo
Copy link
Member

weihanglo commented Mar 10, 2023

Some previous works:

I am okay with interpreting 0 as default (all possible parallelism based on environment and jobserver). I wonder what others' opinions are.

By the way, .cargo/config.toml is considered as a set of settings for environment. If a config is not intended to be shared, probably just remove it from the file.

@weihanglo weihanglo added S-needs-team-input Status: Needs input from team on whether/how to proceed. E-easy Experience: Easy labels May 14, 2023
@charmitro
Copy link
Contributor

Is this still relevant? If yes, I’d like to give it a try.

@weihanglo
Copy link
Member

Thank you for asking! Personally, I am happy with the idea, but the team has yet discussed it. Once it is generally accepted you will see it labeled as S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review .

BTW, I'd also like to learn prior arts from other tools. How does make handle --jobs 0? What about other build tools like bazel, buck or else? It would be helpful if someone can collect the data.

@charmitro
Copy link
Contributor

BTW, I'd also like to learn prior arts from other tools. How does make handle --jobs 0? What about other build tools like bazel, buck or else? It would be helpful if someone can collect the data.

Ack.
I can try to collect data from other tools, especially the famous ones.

@charmitro
Copy link
Contributor

I collected information from make, bazel & cmake.

In all three, we observe the same logic, i.e. they require a positive integer argument.

For reference,

  • make: the '-j' option requires a positive integer argument
  • Basel: ERROR: While parsing option --jobs=0: Value '(0)' must be at least 1.
  • cmake: The value requires a positive integer argument.

@ehuss
Copy link
Contributor

ehuss commented May 27, 2023

I have a slight concern that 0 might be somewhat ambiguous. For example, ninja treats it as "infinity".

Some other data:

  • msbuild: maxCpuCount without a value is number of processors.
  • buck2: 0 is number of cpus.
  • buck: has 5 different options, like thread_core_ratio_reserved_cores
  • meson: Less than 1 means to leave the decision up to the build tool (which is often number of cpus).
  • gradle: Looks like org.gradle.workers.max does not support it.
  • go: -p must be a positive integer
  • swift: -j must be a positive integer

Would there be any objections to using an english string to represent the "number of cpus" or some other meaning? Using 0 probably isn't so much of a leap from our use of negative numbers (and has some precedence), but a short string might be a little clearer.

@weihanglo
Copy link
Member

Second it. A simple English word is probably a clearer choice. This also enables some future extensions like simple math expressions (see #9221 (comment)).

I propose the short word to be CPUS. However, I see Bazel also has a --local_cpu_resources which chooses HOST_CPUS and accepts expressions. Not sure if Cargo will support remote build or distributed build in the future, but HOST_CPUS does sound reasonable in terms of future compatibility.

@weihanglo
Copy link
Member

Let's put it this way. We could choose CPUS. The semantics is “all available parallelism in the system”, equivalent to std::thread::available_parallelism.

If we do have distributed build support in the future, I don't think we will need a breaking change. CPUS can still mean “all available parallelism in the system”. Whether the system refers to host or the entire distributed system is something we can define later.

@charmitro
Copy link
Contributor

I also agree, having something like CPUS or ALL that is interpreted into std::thread::available_parallelism can make this configuration variable look meaningful.

@weihanglo weihanglo added the T-cargo Team: Cargo label May 28, 2023
@weihanglo
Copy link
Member

I'd like to accept this feature request. Let's do an FCP.

@rfcbot FCP merge

Currently there is no way to unset a config value, and this proposes using a simple English word to unset previously set build.job back to the default: use all available parallelism on the system.

Without this feature, it is impossible to unset build.job unless you edit the config file where it is defined.

For bike shedding the short word, we can decide later. The implementation side can start earlier once this feature is accepted.

@weihanglo
Copy link
Member

all CAPS doesn't work?

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented May 28, 2023

Team member @weihanglo has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels May 28, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented May 30, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels May 30, 2023
@epage
Copy link
Contributor

epage commented May 30, 2023

I lean towards "default"

0 seems reasonable with us having -1 but that can be added later

@weihanglo
Copy link
Member

The Cargo team discussed this in the meeting today. We choose default as the magic word to unset to the default parallelism. @charmitro if you are still interested in helping with this, feel free to send a pull request :)

@charmitro
Copy link
Contributor

The Cargo team discussed this in the meeting today. We choose default as the magic word to unset to the default parallelism. @charmitro if you are still interested in helping with this, feel free to send a pull request :)

Still interested, thanks for the update!

@rustbot claim

@charmitro
Copy link
Contributor

charmitro commented Jun 2, 2023

Should this be implemented only on both CLI(--jobs) and config([build]) or only on the CLI?

@weihanglo
Copy link
Member

I think the answer is yes. The two values are merged here. You might like to change i32 to an enum here.

@ParkMyCar
Copy link

Wooo! Glad to see this making progress, thanks @charmitro for putting up a PR! :)

Side note: I opened this ticket originally with a different GitHub account, but have since merged everything into this one. To dissuade folks from tagging the old one, I renamed it, hence the 'do-not-use-parker-timmerman', sorry for any confusion!

@charmitro
Copy link
Contributor

charmitro commented Jun 2, 2023 via email

@bors bors closed this as completed in ab0e1f7 Jun 3, 2023
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-jobserver Area: jobserver, concurrency, parallelism C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` disposition-merge FCP with intent to merge E-easy Experience: Easy finished-final-comment-period FCP complete S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants