Required dependencies are not features #4364
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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. |
|
As @sfackler pointed out, this is a breaking change. I have no idea what the process is for that with cargo. |
|
This looks great to me, thanks for finding and fixing this! I agree with @sfackler though that we'll likely want to ease this transition through warnings rather than errors to start out with. Right now we unfortunately don't have a great way of doing this during resolution, but I think we'll want to do something that looks like:
Does that sound reasonable? |
|
I can try doing that. However, I am not sure when I will have the time, so this may take a week or so. |
|
I think I mostly got this, but there is one problem: The warning is emitted twice for the second of my testcases ( |
|
@RalfJung ah yeah resolution is currently called twice, but it's fine to just emit the warning once, perhaps just a new boolean to the resolution function? |
|
It seems to be called twice for one test, but only once for another test? Also, how would I even distinguish the two calls? I mean I could use a Is it even guaranteed that the two calls to resolution will use all the same settings and everything, and hence show the same warnings? |
|
This is the one location where resolution happens twice. If the first |
|
Can I assume in general that when |
|
Maybe? Don't know that off the top of my head. One way to find out! |
|
Well, I will see if that's the case of the two test cases I have. That doesn't leave me very confident. But I guess this doesn't actually work -- looking at |
|
All right, I got something that makes the tests pass. However, I had to actually let the second resolution in this function you pointed to print warnings -- the first one would not print anything. |
|
Dang. Seems like getting access to the shell here conflicts with someone else already having taken access. I'll try to refactor this, maybe that helps. |
|
It seems that did it :) |
|
@bors: r+ Thanks so much! |
|
📌 Commit 8a4a291 has been approved by |
Required dependencies are not features Also, while I was at it, I fixed an error message which complained about something not being an optional dependency, when really what mattered was that it was not a dependency at all. I made a bunch of guesses about how things work. These guesses ended up as comments in the commit (so hopefully, the next reader of these files has to guess less). I am not totally certain these comments are all correct, so please yell if not. :) In particular, for resolve_features, I observed that dependencies get compiled even when they are not returned from that function. But judging from how the function used to behave, it actually returns all dependencies, even those that have nothing to do with any features. (Making the name rather misleading, TBH...) Fixes #4363
|
☀️ Test successful - status-appveyor, status-travis |
Changelog:
Version 1.21.0 (2017-10-12)
==========================
Language
--------
- [You can now use static references for literals.][43838]
Example:
```rust
fn main() {
let x: &'static u32 = &0;
}
```
- [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540]
Example:
```rust
my_macro!(Vec<i32>::new); // Always worked
my_macro!(Vec::<i32>::new); // Now works
```
Compiler
--------
- [Upgraded jemalloc to 4.5.0][43911]
- [Enabled unwinding panics on Redox][43917]
- [Now runs LLVM in parallel during translation phase.][43506]
This should reduce peak memory usage.
Libraries
---------
- [Generate builtin impls for `Clone` for all arrays and tuples that
are `T: Clone`][43690]
- [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459]
- [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`,
`From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565]
Stabilized APIs
---------------
[`std::mem::discriminant`]
Cargo
-----
- [You can now call `cargo install` with multiple package names][cargo/4216]
- [Cargo commands inside a virtual workspace will now implicitly
pass `--all`][cargo/4335]
- [Added a `[patch]` section to `Cargo.toml` to handle
prepublication dependencies][cargo/4123] [RFC 1969]
- [`include` & `exclude` fields in `Cargo.toml` now accept gitignore
like patterns][cargo/4270]
- [Added the `--all-targets` option][cargo/4400]
- [Using required dependencies as a feature is now deprecated and emits
a warning][cargo/4364]
Misc
----
- [Cargo docs are moving][43916]
to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo)
- [The rustdoc book is now available][43863]
at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc)
- [Added a preview of RLS has been made available through rustup][44204]
Install with `rustup component add rls-preview`
- [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348]
Previously only showed `std::os::unix`.
Compatibility Notes
-------------------
- [Changes in method matching against higher-ranked types][43880] This may cause
breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880]
- [rustc's JSON error output's byte position start at top of file.][42973]
Was previously relative to the rustc's internal `CodeMap` struct which
required the unstable library `libsyntax` to correctly use.
- [`unused_results` lint no longer ignores booleans][43728]
[42565]: rust-lang/rust#42565
[42973]: rust-lang/rust#42973
[43348]: rust-lang/rust#43348
[43459]: rust-lang/rust#43459
[43506]: rust-lang/rust#43506
[43540]: rust-lang/rust#43540
[43690]: rust-lang/rust#43690
[43728]: rust-lang/rust#43728
[43838]: rust-lang/rust#43838
[43863]: rust-lang/rust#43863
[43880]: rust-lang/rust#43880
[43911]: rust-lang/rust#43911
[43916]: rust-lang/rust#43916
[43917]: rust-lang/rust#43917
[44204]: rust-lang/rust#44204
[cargo/4123]: rust-lang/cargo#4123
[cargo/4216]: rust-lang/cargo#4216
[cargo/4270]: rust-lang/cargo#4270
[cargo/4335]: rust-lang/cargo#4335
[cargo/4364]: rust-lang/cargo#4364
[cargo/4400]: rust-lang/cargo#4400
[RFC 1969]: rust-lang/rfcs#1969
[info/43880]: rust-lang/rust#44224 (comment)
[`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
Also, while I was at it, I fixed an error message which complained about something not being an optional dependency, when really what mattered was that it was not a dependency at all.
I made a bunch of guesses about how things work. These guesses ended up as comments in the commit (so hopefully, the next reader of these files has to guess less). I am not totally certain these comments are all correct, so please yell if not. :)
In particular, for resolve_features, I observed that dependencies get compiled even when they are not returned from that function. But judging from how the function used to behave, it actually returns all dependencies, even those that have nothing to do with any features. (Making the name rather misleading, TBH...)
Fixes #4363