Warn about and ignore invalid crate-types #1914
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (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. 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. |
|
Thanks! In the interest of backwards-compatibility, however, I think that we'll probably just want to warn on situations like this for now. It's likely that some manifests have already been published to crates.io with invalid crate-types listed and I wouldn't want to break those. Could you switch this over to just a warning instead? Perhaps a few months from now we can go back and turn it into a hard error. |
|
Alright. Should the invalid crate types just be discarded?
|
|
Yeah it's fine to just drop them aside, but a warning should be issued for each one that's dropped. |
|
Alright. Should I propagate a reference to |
|
There's a few other places in the TOML parsing where warnings are added to a manifest (mainly because we don't want to warn about upstream manifests, only local ones), so following that pattern should do the trick! |
|
Alright. The manifest doesn't exist when this parsing code runs, unfortunately (we're still in the middle of creating it). I'll have to store the warnings for when it does. |
|
This should be ready to go, unless you have any further suggestions. |
| .set_doctest(toml.doctest.unwrap_or(t2.doctested())) | ||
| .set_benched(toml.bench.unwrap_or(t2.benched())) | ||
| .set_harness(toml.harness.unwrap_or(t2.harness())) | ||
| .set_for_host(toml.plugin.unwrap_or(t2.for_host())); |
There was a problem hiding this comment.
It looks like there weren't any substantial changes here, so could this stay the same?
There was a problem hiding this comment.
Yes; this change is, strictly speaking, unrelated; just eliminates the call to clone(). I'll revert it.
|
Would you prefer I make a new commit fixing your review questions, or squash everything down into a new one (and force-push over the above ones)? |
|
Feel free to go ahead and squash + force-push! |
|
The above commit was rebased; it includes all the changes you wanted! |
thirtythreeforty
changed the title
| if l.plugin == Some(true) {LibKind::Dylib} | ||
| else {LibKind::Lib} | ||
| ] | ||
| }; |
There was a problem hiding this comment.
Ah could you format this as:
let foo = match bar {
Some(foo) => {
foo.iter().filter_map(|a| {
// ...
}).collect()
}
None => {
vec![...]
}
};There was a problem hiding this comment.
Oh I understand what you meant now. Done. Needed a re-test anyway because Travis is having trouble.
This fixes a problem with manifest parsing that I detailed [here](http://stackoverflow.com/a/32055245/1830736). Namely, requesting an invalid `crate-type`, as in the following: ``` [lib] crate-type = ["bad_type"] ``` did not cause an error to be printed, but rather the bad list of types was silently converted to `["rlib"]`. This was confusing to the user. Cargo now properly rejects the above snippet.
|
☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64 |
|
Glad to help, thanks for your patience! |
|
Created an issue for turning the warning into a hard error: #2692 |
This fixes a problem with manifest parsing that I detailed here. Namely, requesting an invalid
crate-type, as in the following:did not cause an error to be printed, but rather the bad list of types was silently converted to
["rlib"]. This was confusing to the user.Cargo now properly rejects the above snippet.