Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement renaming dependencies in the manifest #4953
Conversation
rust-highfive
assigned
matklad
Jan 17, 2018
This comment has been minimized.
This comment has been minimized.
rust-highfive
commented
Jan 17, 2018
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
This may have already been discussed elsewhere, but I think it would be a bit less confusing to do something like this: [dependencies]
foo = "0.1"
bar = { version = "0.1", registry = "custom", name = "foo" }
baz = { git = "https://github.com/foo/bar", name = "foo" }It avoids the extra array, and makes it so that the set of names you're importing (modulo crate/lib naming differences) are all in the same place. |
This comment has been minimized.
This comment has been minimized.
|
Heh that's actually the first syntax I thought of as well, although it was changed due to rust-lang/rfcs#2126 (comment), namely:
For example today you actually specify package names as the key in That being said I also just realized something that can be troubling. How, for example, do we enable features for these dependencies? For example: [dependencies]
foo = [
{ version = "0.1" },
{ version = "0.1", registry = "custom", as = "bar" },
{ git = 'https://github.com/foo/bar', as = "baz" },
]
[features]
some-feature = ["foo/another-feature"] # what version of `foo`?This I think is much clearer with what @sfackler is thinking: [dependencies]
foo = "0.1"
bar = { version = "0.1", registry = "custom", name = "foo" }
baz = { git = "https://github.com/foo/bar", name = "foo" }
[features]
some-feature = ["foo/another-feature"] # aha, the crates.io version!That may tip my own opinion towards the latter! |
This comment has been minimized.
This comment has been minimized.
|
Oh yeah, good point. It's too bad you're allowed to set the library name separately from the crate name :( |
matklad
reviewed
Jan 19, 2018
| @@ -125,6 +125,9 @@ features! { | |||
|
|
|||
| // Downloading packages from alternative registry indexes. | |||
| [unstable] alternative_registries: bool, | |||
|
|
|||
| // Downloading packages from alternative registry indexes. | |||
This comment has been minimized.
This comment has been minimized.
| .filter_map(|d| d.rename()) | ||
| .next(); | ||
|
|
||
| v.push(name.unwrap_or(&dep.target.crate_name())); |
This comment has been minimized.
This comment has been minimized.
matklad
Jan 19, 2018
Member
Could we make this .unwrap_or an utility method of something? Like fn crate_name(&self) -> String, which deals with as and, probably, with translating - to _ as well.
| bar = [ | ||
| { version = "0.1.0" }, | ||
| { version = "0.2.0", as = "baz" }, | ||
| ] |
This comment has been minimized.
This comment has been minimized.
matklad
Jan 19, 2018
Member
Hm the syntax does not look too natural to me. Is the following a valid TOML?
[dependencies]
bar = "0.1.0"
bar = { version = "0.2.0", as = "baz"}
This comment has been minimized.
This comment has been minimized.
alexcrichton
Jan 19, 2018
Author
Member
Unfortunately TOML-the-spec doesn't allow for something like that b/c it's counted as a redefinition of an existing key :(
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
force-pushed the
alexcrichton:rename-via-as
branch
from
0addcd2
to
6e90f36
Jan 22, 2018
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
force-pushed the
alexcrichton:rename-via-as
branch
from
6e90f36
to
102ee8a
Jan 25, 2018
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
referenced this pull request
Feb 8, 2018
Closed
Allow renaming crates in the dependencies section #1311
This comment has been minimized.
This comment has been minimized.
|
If we wanted to make it clear where we're specifying the package name vs the crate name, perhaps we could do something like... [dependencies]
foo = "0.1"
bar = { version = "0.1", registry = "custom", package = "foo" }
baz = { git = "https://github.com/foo/bar", package = "foo" }
[features]
some-feature = ["foo/another-feature"] # aha, the crates.io version! |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton Great work on this. I think this feature is very much due. Funnily enough, I was just going to suggest the key It's also worth noting that this PR helps with rust-lang/rust#44660. Finally, maybe add a "fixes #1311" to your original PR message? |
alexcrichton
force-pushed the
alexcrichton:rename-via-as
branch
from
102ee8a
to
3d7dcdf
Feb 12, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton Yeah, that's fair enough. Anyway, the new semantics are with the alias on the LHS of the |
This comment has been minimized.
This comment has been minimized.
|
@alexreg, correct! |
This comment has been minimized.
This comment has been minimized.
|
Super. Hopefully @matklad can review soon and then we can get this merged. |
matklad
reviewed
Feb 12, 2018
|
Here are a couple of tests I think we could add
|
| @@ -27,6 +27,7 @@ struct Inner { | |||
| specified_req: bool, | |||
| kind: Kind, | |||
| only_match_name: bool, | |||
| rename: Option<String>, | |||
This comment has been minimized.
This comment has been minimized.
matklad
Feb 12, 2018
Member
I'd call it package, to be consistent with the package in the surface syntax, and to make it more clear in which direction renaming goes (I.e, in theory you can have a crate foo which is renamed to pacakge baz, or, the other way around, a package baz which is renamed to crate foo).
This comment has been minimized.
This comment has been minimized.
alexcrichton
Feb 12, 2018
Author
Member
Oh this is slightly different through b/c it's not actually the package, but rather what the dependency is renaming the crate to
This comment has been minimized.
This comment has been minimized.
alexreg
Feb 13, 2018
Contributor
Yeah, even though there's a one-to-one correspondence with the package attribute, rename probably makes more sense in code (perhaps with a comment?)... that's my thought, anyway.
alexcrichton
force-pushed the
alexcrichton:rename-via-as
branch
from
3d7dcdf
to
79942fe
Feb 12, 2018
This comment has been minimized.
This comment has been minimized.
|
@matklad sure yeah, although for specifying one crate twice we're already pretty bad about that today unfortunately. For example something like: [dependencies]
foo = "0.1"
[target.'cfg(unix)'.dependencies]
foo = "0.1"is accepted and works, so I figure we can probably just let rustc sort it out if it's specified multiple times. |
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
Good work with this @alexcrichton! Thanks. 👍🏻 |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Feb 19, 2018
This comment has been minimized.
This comment has been minimized.
|
|
alexcrichton
deleted the
alexcrichton:rename-via-as
branch
Mar 1, 2018
This comment has been minimized.
This comment has been minimized.
peterjoel
commented
Mar 16, 2018
|
@matklad wrote:
Probably should be a warning - and maybe not even that. Generated code shouldn't have to worry about this edge case. But two references to the same exact create should only result in the crate being included once, just with two aliases. |
scottmcm
referenced this pull request
Apr 16, 2018
Merged
RFC: Reserve `try` for `try { .. }` block expressions #2388
alexcrichton
referenced this pull request
Apr 27, 2018
Closed
Tracking issue for renaming crate dependencies #5422
astraw
referenced this pull request
Apr 29, 2018
Merged
remove use of futures and tokio from within client #1
jonhoo
referenced this pull request
May 16, 2018
Closed
Use correct name when dependency has been renamed #133
dekellum
referenced this pull request
Jun 1, 2018
Open
cargo dependency rename breaks cargo-tree #38
This comment has been minimized.
This comment has been minimized.
aaronsky
commented
Jul 21, 2018
•
|
With this merged, does this mean that the item under rust-lang/rust#44931 should be updated? Apologies if this is the wrong place to ask about this |
This comment has been minimized.
This comment has been minimized.
hannobraun
referenced this pull request
Oct 11, 2018
Open
[RFC] Managing breaking changes to existing traits #100
dtolnay
referenced this pull request
Oct 25, 2018
Open
Expose dependency renames so that enabled features make sense #1539
This comment has been minimized.
This comment has been minimized.
aldanor
commented
Nov 15, 2018
•
|
Here's a curious (but not so improbable) edge case with proc macros. Wondering how should this be handled? (or am I misunderstanding something)
Now, if we use the derive proc macro in crate ^^^^^^ Could not find `foo` in `{{root}}`What would be a proper way to refer to it then?.. |
aldanor
unassigned
matklad
Nov 15, 2018
This comment has been minimized.
This comment has been minimized.
aldanor
commented
Nov 15, 2018
•
|
Here's a concrete example, using
cargo-features = ["rename-dependency"]
[package]
name = "rename-test"
version = "0.1.0"
edition = "2018"
[dependencies]
foo = { package = "num-traits", version = "0.2" }
num-derive = "0.2"
use num_derive::{FromPrimitive, ToPrimitive};
#[derive(FromPrimitive, ToPrimitive)]
enum Color { Red, Blue, Green }... which results in...
|
This comment has been minimized.
This comment has been minimized.
|
@aldanor that's a general problem with hygiene and procedural macros, and most procedural macros only work if the corresponding library crate is imported with one precise name (and not renamed). There's no way, for example, to get importing the |
This comment has been minimized.
This comment has been minimized.
aldanor
commented
Nov 16, 2018
•
|
@alexcrichton Yea, I understand (and this is quite sad because most derive macros depend on some other crate; maybe there should be some crate-resolving syntax like With the example above though, if you revert back to 2015-edition way and remove renaming from the manifest, it does work with [dependencies]
num-traits = "0.2"
num-derive = "0.2"and extern crate num_traits as foo; // <--
#[macro_use] extern crate num_derive;
#[derive(FromPrimitive, ToPrimitive)]
enum Color { Red, Blue, Green }(because of Does it mean this particular case is a rename-dependency regression then? |
This comment has been minimized.
This comment has been minimized.
|
I don't think this is really a regression, you can always remove renaming in the manifest and have |
This comment has been minimized.
This comment has been minimized.
There is an issue open for this at rust-lang/rust#54363. |
alexcrichton commentedJan 17, 2018
•
edited
This commit implements a new unstable feature for manifests which allows
renaming a crate when depending on it. For example you can do:
Here three crates will be imported but they'll be made available to the Rust
source code via the names
foo(crates.io),bar(the custom registry), andbaz(the git dependency). The package name, however, will befoofor allof them. In other words the git repository here would be searched for a crate
called
foo. For example:The intention here is to enable a few use cases:
extern crate foo as barsyntactically in Rust sourceCurrently I don't think we're ready to stabilize this so it's just a nightly
feature, but I'm hoping we can continue to iterate on it!
cc #1311