Allow true and false as options for strip option
#9153
Conversation
This follows the convention of `lto` and `debug` that allow `true` for the highest level, and `false` for disabled. Signed-off-by: David Calavera <david.calavera@gmail.com>
|
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. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
Verifying options doesn't happen when the file is parsed. This scenario is already covered in another test. See [the profiles test suite](https://github.com/rust-lang/cargo/blob/6e3e14e223f9566773b5d903357cbf35ae528ac6/tests/testsuite/profiles.rs#L549) Signed-off-by: David Calavera <david.calavera@gmail.com>
ehuss
added
S-waiting-on-author
|
thanks for the review @alexcrichton. I'll try to work on a more general solution with less specific validations |
- Accept a string option that's passed to rustc to decide how to handle it. - Remove early validation. Signed-off-by: David Calavera <david.calavera@gmail.com>
The compiler emits an error when it receives an unknown option. Signed-off-by: David Calavera <david.calavera@gmail.com>
|
@bors r+ squash |
|
📌 Commit 666380f has been approved by |
bors
added
S-waiting-on-bors
| Some(StringOrBool::String(ref n)) if matches!(n.as_str(), "off" | "n" | "no") => { | ||
| profile.lto = Lto::Off | ||
| } | ||
| Some(StringOrBool::String(ref n)) if is_off(n.as_str()) => profile.lto = Lto::Off, |
There was a problem hiding this comment.
I'm a little uncomfortable changing this to accept "none" as off, but in the grand scheme of things probably doesn't matter.
There was a problem hiding this comment.
to be honest, it made me uncomfortable too, but then I remembered that I copied/pasted this code in different places in the first iteration, and kinda decided to avoid that for future contributors.
Allow `true` and `false` as options for `strip` option This follows the convention of `lto` and `debug` that allow `true` for the highest level, and `false` for disabled. Signed-off-by: David Calavera <david.calavera@gmail.com>
|
☀️ Test successful - checks-actions |
Update cargo 5 commits in 34170fcd6e0947808a1ac63ac85ffc0da7dace2f..ab64d1393b5b77c66b6534ef5023a1b89ee7bf64 2021-02-04 15:52:52 +0000 to 2021-02-10 00:19:10 +0000 - Allow `true` and `false` as options for `strip` option (rust-lang/cargo#9153) - Change git dependencies to use `HEAD` by default (rust-lang/cargo#9133) - appendix auth gcm link to new repo (which is xplat) (rust-lang/cargo#9152) - Fix warnings of the new non_fmt_panic lint (rust-lang/cargo#9148) - Fix panic with doc collision orphan. (rust-lang/cargo#9142)
|
It looks like bors merged these changes in master already, but the PR is still open. Should this be closed? I've noticed that other PRs get closed and marked as "merged" by bors, I don't understand why it didn't happen with this one. |
|
Oh, strange! GitHub was having some issues yesterday, that could be it. Or it could be the |
This follows the convention of
ltoanddebugthat allowtrueforthe highest level, and
falsefor disabled.Signed-off-by: David Calavera david.calavera@gmail.com