Skip to content

Commit

Permalink
Auto merge of #10676 - djmcgill:origin/master, r=weihanglo
Browse files Browse the repository at this point in the history
add validation for string "true"/"false" in lto profile

### 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?

 <!-- Demonstrate how you test this change and guide reviewers through your PR.
With a smooth review process, a pull request usually gets reviewed quicker.

If you don't know how to write and run your tests, please read the guide:
https://doc.crates.io/contrib/tests -->

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](https://github.com/rust-lang/rust/blob/491f619f564a4ff9ae4cc837e27bb919d04c31be/compiler/rustc_session/src/options.rs#L456) 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.
  • Loading branch information
bors committed Jun 5, 2022
2 parents cce487e + 0daf70d commit a9efb06
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
10 changes: 10 additions & 0 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,16 @@ impl TomlProfile {
}
}

if let Some(StringOrBool::String(arg)) = &self.lto {
if arg == "true" || arg == "false" {
bail!(
"`lto` setting of string `\"{arg}\"` for `{name}` profile is not \
a valid setting, must be a boolean (`true`/`false`) or a string \
(`\"thin\"`/`\"fat\"`/`\"off\"`) or omitted.",
);
}
}

Ok(())
}

Expand Down
31 changes: 31 additions & 0 deletions tests/testsuite/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,37 @@ fn profile_in_virtual_manifest_works() {
.run();
}

#[cargo_test]
fn profile_lto_string_bool_dev() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
[profile.dev]
lto = "true"
"#,
)
.file("src/lib.rs", "")
.build();

p.cargo("build")
.with_status(101)
.with_stderr(
"\
error: failed to parse manifest at `[ROOT]/foo/Cargo.toml`
Caused by:
`lto` setting of string `\"true\"` for `dev` profile is not a valid setting, \
must be a boolean (`true`/`false`) or a string (`\"thin\"`/`\"fat\"`/`\"off\"`) or omitted.
",
)
.run();
}

#[cargo_test]
fn profile_panic_test_bench() {
let p = project()
Expand Down

0 comments on commit a9efb06

Please sign in to comment.