-
Notifications
You must be signed in to change notification settings - Fork 460
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
Don't override NUM_JOBS if already set #3258
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just had one request
"OPT_LEVEL": compilation_mode_opt_level, | ||
"RUSTC": toolchain.rustc.path, | ||
"TARGET": toolchain.target_flag_value, | ||
# OUT_DIR is set by the runner itself, rather than on the action. | ||
}) | ||
|
||
# Allow the user to override the number of jobs for the build script. | ||
if "NUM_JOBS" not in env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is any value larger than 1 then it would be good to have an associated resource set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what that means, what's an "associated resource"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically a change similar to #3112
which uses ctx.actions.run.resource_set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll find the big ugly file rust/private/rustc_resource_set.bzl
and it's use in rust/private/rustc.bzl
resource_set = get_rustc_resource_set(toolchain)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@UebelAndre I can't call get_rustc_resource_set as it stands without refactoring it to take an int instead of a toolchain, do you want me to do that, or duplicate rust/private/rustc_resource_set.bzl
with an int variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just add a new function. get_cargo_build_script_resource_set
or something that does what you need. When I wrote that all I was thinking about was Rustc
actions but I don't think we should duplicate these functions. So I'd just make a new function for the use here.
"OPT_LEVEL": compilation_mode_opt_level, | ||
"RUSTC": toolchain.rustc.path, | ||
"TARGET": toolchain.target_flag_value, | ||
# OUT_DIR is set by the runner itself, rather than on the action. | ||
}) | ||
|
||
# Allow the user to override the number of jobs for the build script. | ||
if "NUM_JOBS" not in env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just add a new function. get_cargo_build_script_resource_set
or something that does what you need. When I wrote that all I was thinking about was Rustc
actions but I don't think we should duplicate these functions. So I'd just make a new function for the use here.
Building large src crates (e.g. proj_sys) take a long time because NUM_JOBS was always 1. This allows the user to override (e.g. with --actiona_env) the value of NUM_JOBS