feat(hints): support minimum optimization levels - #17368
Conversation
b532a3f to
2b19c48
Compare
There was a problem hiding this comment.
🔢 Self-check (PR reviewed by myself and ready for feedback)
-
Code compiles successfully
-
Unit tests added
-
No AI-generated elegant nonsense in PR.
-
Comments added where necessary
-
PR title and description updated
-
Documentation updated
-
PR size is reasonable
| [package] | ||
| name = "cargo-util-schemas" | ||
| version = "0.14.3" | ||
| version = "0.15.0" |
There was a problem hiding this comment.
Because we added a new field to a public struct without marking it as non_exhaustive, I bumped the major version.
| }, | ||
| "TomlDebugInfo": { | ||
| "type": ["string", "integer", "boolean"], | ||
| "type": [ |
There was a problem hiding this comment.
This one was also generated automatically.
| &self, | ||
| pkg_id: PackageId, | ||
| pkg_hints: Option<&Hints>, | ||
| hint_min_opt_level: bool, |
There was a problem hiding this comment.
Not sure if this is the best way to determine whether users want to enable this hint.
| via `profile`, which takes precedence, and which can only be specified in the | ||
| top-level crate being built. | ||
|
|
||
| ## Package `min-opt-level` hint |
There was a problem hiding this comment.
Can you have a subsection that is written as-if it was the end-user documentation?
| The hint accepts the numeric optimization levels 0, 1, 2, and 3. Other values | ||
| produce a warning and are ignored. If the selected [profile](profiles.md) has a |
There was a problem hiding this comment.
Why do they produce a warning instead of error?
There was a problem hiding this comment.
If we proceed with errors, does that mean that introducing a new optimization level in the future, which is accepted by the new Cargo, would cause an error with older versions of Cargo? Would this be considered a backward compatibility issue? But I guess introducing a new optimization level might be impossible, especially a numeric optimization level?
| let pkg_hint_min_opt_level = match parse_min_opt_level_hint(hints.min_opt_level.as_ref()) { | ||
| Ok(level) => level, | ||
| Err(MinOptLevelHintError::OutOfRange(level)) => { | ||
| unit_capped_warn(&format!( | ||
| "ignoring unsupported value ({level}) for 'hints.min-opt-level', which only supports integers from 0 to 3" | ||
| ))?; | ||
| None | ||
| } | ||
| Err(MinOptLevelHintError::WrongType(value_type)) => { | ||
| unit_capped_warn(&format!( | ||
| "ignoring unsupported value type ({value_type}) for 'hints.min-opt-level', which expects an integer" | ||
| ))?; | ||
| None | ||
| } | ||
| }; | ||
| if matches!(pkg_hint_min_opt_level, Some(1..=3)) && !bcx.gctx.cli_unstable().hint_min_opt_level | ||
| { | ||
| unit_capped_warn( | ||
| "ignoring 'hints.min-opt-level', pass `-Zhint-min-opt-level` to enable it", | ||
| )?; | ||
| } |
There was a problem hiding this comment.
Why is this check placed here?
There was a problem hiding this comment.
I simply followed the mostly-unused placement and tried to group them together. However, after reconsidering, this may not make sense because this case differs from it: this hint does not directly change the cmd here. The per-compilation-unit check also does not apply to the min-opt-level hint. I will try to find the right place to perform this check.
| let mut profile = maker.get_profile( | ||
| Some(pkg_id), | ||
| is_member, | ||
| unit_for.is_for_host(), | ||
| min_opt_level, | ||
| ); |
There was a problem hiding this comment.
Why do we pass it into get_profile, rather than overriding it here?
There was a problem hiding this comment.
Because the hint takes precedence within ProfileMaker::get_profile, applying it afterward would override explicit settings such as:
[profile.dev.package.foo]
opt-level = 0 | with_opt_level(&mut cargo, "dep", "0"); | ||
| let mut cargo = p.cargo("check -v -Zhint-min-opt-level"); | ||
| cargo.masquerade_as_nightly_cargo(&["hint-min-opt-level"]); | ||
| with_opt_level(&mut cargo, "dep", "2"); |
There was a problem hiding this comment.
If you move -Zhint-min-opt-level into a separate .arg() along with masquerade_as_nightly_cargo, we have one line to delete and don't need to worry accidentally deleting part of the important test command.
I just did this in freshness_checksum.rs and am tempted to roll it out further.
| ("positive", "positive", "2", "2"), | ||
| ("wrong-type", "wrong_type", r#""s""#, "0"), | ||
| ("out-of-range", "out_of_range", "4", "0"), | ||
| ] { |
There was a problem hiding this comment.
Would make the feature commit cleaner to have expected_opt_level in the previous commit
| } | ||
|
|
||
| #[cargo_test] | ||
| fn min_opt_level_without_feature_gate() { |
There was a problem hiding this comment.
Why are tests being added in this commit?
There was a problem hiding this comment.
As I thought, testing a nonexistent -Z flag doesn’t make sense. However, I can move it to the previous commit.
This comment has been minimized.
This comment has been minimized.
14ab0ce to
415812c
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
415812c to
aaf6533
Compare
aaf6533 to
2c86de0
Compare
Record the current ignored-hint behavior across the RFC precedence, scope, validation, and boundary cases. Signed-off-by: 0xPoe <poe.liu@pm.me>
RFC 3924 lets a package request a numeric optimization floor without overriding an application's profile choices. Apply the hint after profile and built-in host defaults but before explicit package and build overrides, while leaving the unordered size levels unchanged. Signed-off-by: 0xPoe <poe.liu@pm.me>
Document the unstable gate, accepted range, profile precedence, non-recursive scope, and the narrow cases where packages should request optimization. Include an explicit debugging override so top-level users retain control. Signed-off-by: 0xPoe <poe.liu@pm.me>
2c86de0 to
18f313e
Compare
What does this PR try to resolve?
ref #17334
This PR would allow Rust library to provide a simple hint about the minimum opt-level to build them with via the
hint.min-opt-levelsetting in the manifest.How to test and review this PR?
Check the unit tests and review it commit by commit.
r?@ghost