Elaborate registry names to index URLs when publishing #4957
Conversation
|
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
sfackler
force-pushed
the
registry-elaboration
branch
from
be2473d to
e7c4344
Compare
There was a problem hiding this comment.
cc @withoutboats and @carols10cents
Wanted to make y'all aware of this as well. This is enabling a feature which IIRC was explicitly disallowed in the original RFC (specifying the URL of an index in Cargo.toml). That being said, though, I don't think this is violating the spirit of the RFC in that we'd still basically only ever teach users about registry = "foo" (named registries) and maybe only mention registry-index = "..." in an appendix or something like that.
If the motivation isn't clear here though as to why we're doing that, I'd also be happy to clarify!
| @@ -181,6 +181,7 @@ impl<'de> de::Deserialize<'de> for TomlDependency { | |||
| pub struct DetailedTomlDependency { | |||
| version: Option<String>, | |||
| registry: Option<String>, | |||
| registry_index: Option<String>, | |||
There was a problem hiding this comment.
Could this use #[serde(rename)] to use registry-index instead?
| details.registry.as_ref(), | ||
| details.registry_index.as_ref(), | ||
| ) { | ||
| (Some(_), _, Some(_), _) | |
There was a problem hiding this comment.
One day we will make this better. It is not this day.
|
Updated |
| @@ -178,25 +178,26 @@ impl<'de> de::Deserialize<'de> for TomlDependency { | |||
| } | |||
|
|
|||
| #[derive(Deserialize, Serialize, Clone, Debug, Default)] | |||
| #[serde(rename_all = "kebab-case")] | |||
There was a problem hiding this comment.
Ah unfortuantely I don't think we can do this as it breaks manifests that use default_features = false, for example.
When we first moved to serde we ran into that (as the old rustc-serialize toml-rs accepted both) and that's why there's two entries below :(
There was a problem hiding this comment.
The #[serde(rename = "default_features")] down on default_features2 will handle that: https://play.rust-lang.org/?gist=db44b7c463a8b4ac43c0b67e2b36e5d0&version=stable
|
This LGTM, but others from @rust-lang/cargo will likely wish to weigh in |
|
☔ The latest upstream changes (presumably #4844) made this pull request unmergeable. Please resolve the merge conflicts. |
sfackler
force-pushed
the
registry-elaboration
branch
from
44d2a3c to
7c7bc23
Compare
|
One reason we didn't want to support this is to disallow credentials (such as https://username:password@index-url.com) from getting into the Cargo.toml. We could just always explicitly ignore credentials specified here so that doing that doesn't work..... |
|
Should we forbid credentials in index URLs entirely? |
|
Forbidding them would also take care of potential issues if some people use |
|
I think so long as we have some way of specifying credentials in |
|
Cool, so are there any blockers on this PR, then? |
This avoids introducing a dependency on the publisher's name for the registry. Closes rust-lang#4880
sfackler
force-pushed
the
registry-elaboration
branch
from
7c7bc23 to
77ccd0e
Compare
|
Updated to forbid credentials in registry URLs. |
|
@bors: r+ |
|
📌 Commit 77ccd0e has been approved by |
Elaborate registry names to index URLs when publishing This avoids introducing a dependency on the publisher's name for the registry. Closes #4880
|
☀️ Test successful - status-appveyor, status-travis |
This avoids introducing a dependency on the publisher's name for the
registry.
Closes #4880