add validation for string "true"/"false" in lto profile #10676

djmcgill · 2022-05-17T18:25:55Z

What does this PR try to resolve?

Adds a special-cased error message for when lto is set to the string "true"/"false" which is surprisingly (I was surprised anyway) not allowed and the error message is ambiguous. The new error message makes it clear what values are accepted.
Fixes #10572

How should we test and review this PR?

Uh I've not actually tested yet that's the WIP part. But put

[profile.dev]
lto="false"

in your TOML and run cargo build, check that you get the new error message and that it makes sense and is helpful.

Additional information

It's worth noting that as per rust-lang/rust#97051 this doesn't fix the real problem here IMO which is that rust's opt_parse_bool cli parsing doesn't accept true/false which certainly seems an ad-hoc historical choice to me on first glance but also it's a much bigger change to change those semantics than this error message.

rust-highfive · 2022-05-17T18:25:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

Muscraft · 2022-05-17T19:39:28Z

src/cargo/util/toml/mod.rs

+                    bail!(
+                        "`lto` setting of string `\"{}\"` is not a valid setting, \
+                        must be a boolean (`true`/`false`) or a string (`\"yes\"`/ \
+                        `\"no\"`/`\"on\"`/`\"off\"`/`\"thin\"`/`\"fat\"`, etc) or omitted.",


While this does match the output of rustc if you put lto = "true" it doesn't match the current cargo lto docs. I think it might be a good idea to either update the current cargo docs to match rustc or to only include the things that the current cargo lto docs do. I would lean towards only keeping what cargo currently does as it differs greatly from the rustc docs. I would hold off on implementing this until a maintainer chimes in but I think it would be a mistake to not have the error message here match the documentation about lto.

Thanks for the quick review, will do

Also it would be great to include the profile name, so the message might be like

`lto` setting of string `{arg}` for `{name}` profile is not valid. \ Must be a boolean or a string ("fat", "thin", "off").

src/cargo/util/toml/mod.rs

djmcgill · 2022-05-17T20:31:29Z

src/cargo/util/toml/mod.rs

+            if let StringOrBool::String(arg) = lto {
+                if arg == "true" || arg == "false" {


Try as I might I could not get Deref pattern matching to work here. I think because StringOrBool doesn't implement it, so we can't use that mechanism on its contents, only match it against String or &String etc? And e.g. ref "true" to force it isn't a valid pattern I discovered, that works only on bound variables not on literal patterns.

weihanglo

Thank you for the PR! Could you add a unit test for the change? One similar to this is great:

cargo/tests/testsuite/profiles.rs

Lines 326 to 354 in d6cdde5

    
           #[cargo_test] 
        
           fn profile_panic_test_bench() { 
        
               let p = project() 
        
                   .file( 
        
                       "Cargo.toml", 
        
                       r#" 
        
                           [package] 
        
                           name = "foo" 
        
                           version = "0.0.1" 
        
                           [profile.test] 
        
                           panic = "abort" 
        
                           [profile.bench] 
        
                           panic = "abort" 
        
                       "#, 
        
                   ) 
        
                   .file("src/lib.rs", "") 
        
                   .build(); 
        
               p.cargo("build") 
        
                   .with_stderr_contains( 
        
                       "\ 
        
           [WARNING] `panic` setting is ignored for `bench` profile 
        
           [WARNING] `panic` setting is ignored for `test` profile 
        
           ", 
        
                   ) 
        
                   .run(); 
        
           }

src/cargo/util/toml/mod.rs

weihanglo · 2022-05-18T13:55:57Z

src/cargo/util/toml/mod.rs

+                    bail!(
+                        "`lto` setting of string `\"{}\"` is not a valid setting, \
+                        must be a boolean (`true`/`false`) or a string (`\"yes\"`/ \
+                        `\"no\"`/`\"on\"`/`\"off\"`/`\"thin\"`/`\"fat\"`, etc) or omitted.",


Also it would be great to include the profile name, so the message might be like

`lto` setting of string `{arg}` for `{name}` profile is not valid. \ Must be a boolean or a string ("fat", "thin", "off").

djmcgill · 2022-06-05T11:51:58Z

Made the changes thanks, output currently looks like:

$ ./target/debug/cargo build
error: failed to parse manifest at `/mnt/c/Users/admin/Documents/code/cargo/Cargo.toml`

Caused by:
  `lto` setting of string `"false"` for `dev` profile is not a valid setting, must be a boolean (`true`/`false`) or a string (`"thin"`/`"fat"`/`"off"`) or omitted.

weihanglo · 2022-06-05T13:56:23Z

Looks nice! Could you add a unit test as I mentioned before? #10676 (review)

tests/testsuite/profiles.rs

djmcgill · 2022-06-05T18:26:14Z

Done, everything passed now.

weihanglo · 2022-06-05T22:27:16Z

Thank you!
The commit logs could be more meaningful. Could you tidy them up or squash them?

djmcgill · 2022-06-05T22:31:06Z

Done, too used to autosquash ha

weihanglo · 2022-06-05T23:02:41Z

Thanks!

@bors r+

bors · 2022-06-05T23:02:43Z

📋 Looks like this PR is still in progress, ignoring approval.

Hint: Remove WIP from this PR's title when it is ready for review.

weihanglo · 2022-06-05T23:03:16Z

@bors r+

bors · 2022-06-05T23:03:17Z

📌 Commit 0daf70d has been approved by weihanglo

bors · 2022-06-05T23:03:23Z

⌛ Testing commit 0daf70d with merge a9efb06...

bors · 2022-06-05T23:45:02Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing a9efb06 to master...

Update cargo 7 commits in 38472bc19f2f76e245eba54a6e97ee6821b3c1db..85e457e158db216a2938d51bc3b617a5a7fe6015 2022-05-31 02:03:24 +0000 to 2022-06-07 21:57:52 +0000 - Make -Z http-registry use index.crates.io when accessing crates-io (rust-lang/cargo#10725) - Respect submodule update=none strategy in .gitmodules (rust-lang/cargo#10717) - Expose rust-version through env var (rust-lang/cargo#10713) - add validation for string "true"/"false" in lto profile (rust-lang/cargo#10676) - Enhance documentation of testing (rust-lang/cargo#10726) - Clear disk space on CI. (rust-lang/cargo#10724) - Enforce to use tar v0.4.38 (rust-lang/cargo#10720)

rust-highfive assigned ehuss May 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2022

Muscraft reviewed May 17, 2022

View reviewed changes

djmcgill commented May 17, 2022

View reviewed changes

weihanglo reviewed May 18, 2022

View reviewed changes

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2022

weihanglo reviewed Jun 5, 2022

View reviewed changes

tests/testsuite/profiles.rs Outdated Show resolved Hide resolved

Special case the string true/false error message for LTO profile arg

0daf70d

djmcgill force-pushed the origin/master branch from 31106c6 to 0daf70d Compare June 5, 2022 22:30

weihanglo changed the title ~~WIP: add validation for string "true"/"false" in lto profile~~ add validation for string "true"/"false" in lto profile Jun 5, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jun 5, 2022

bors merged commit a9efb06 into rust-lang:master Jun 5, 2022

djmcgill deleted the origin/master branch June 5, 2022 23:50

ehuss mentioned this pull request Jun 8, 2022

Update cargo rust-lang/rust#97893

Merged

ehuss added this to the 1.63.0 milestone Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add validation for string "true"/"false" in lto profile #10676

add validation for string "true"/"false" in lto profile #10676

djmcgill commented May 17, 2022

rust-highfive commented May 17, 2022

Muscraft May 17, 2022

djmcgill May 17, 2022

weihanglo May 18, 2022

djmcgill May 17, 2022 •

edited

Loading

weihanglo left a comment

weihanglo May 18, 2022

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

bors commented Jun 5, 2022

weihanglo commented Jun 5, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

		if let StringOrBool::String(arg) = lto {
		if arg == "true" \|\| arg == "false" {

	#[cargo_test]
	fn profile_panic_test_bench() {
	let p = project()
	.file(
	"Cargo.toml",
	r#"
	[package]
	name = "foo"
	version = "0.0.1"

	[profile.test]
	panic = "abort"

	[profile.bench]
	panic = "abort"
	"#,
	)
	.file("src/lib.rs", "")
	.build();

	p.cargo("build")
	.with_stderr_contains(
	"\
	[WARNING] `panic` setting is ignored for `bench` profile
	[WARNING] `panic` setting is ignored for `test` profile
	",
	)
	.run();
	}

add validation for string "true"/"false" in lto profile #10676

add validation for string "true"/"false" in lto profile #10676

Conversation

djmcgill commented May 17, 2022

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented May 17, 2022

Muscraft May 17, 2022

Choose a reason for hiding this comment

djmcgill May 17, 2022

Choose a reason for hiding this comment

weihanglo May 18, 2022

Choose a reason for hiding this comment

djmcgill May 17, 2022 • edited Loading

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo May 18, 2022

Choose a reason for hiding this comment

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

djmcgill commented Jun 5, 2022

weihanglo commented Jun 5, 2022

bors commented Jun 5, 2022

weihanglo commented Jun 5, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

bors commented Jun 5, 2022

djmcgill May 17, 2022 •

edited

Loading