Add support for publish to optionally take the index that can be used #4568

cswindle · Oct 3, 2017

This form part of alternative-registries RFC-2141, it allows crates to optionally specify which registries the crate can be be published to.

@carols10cents, one thing that I am unsure about is if there is a plan for publish to still provide index, or for registry to be provided instead. I thought that your general view was that we should move away from the index file. If we do need to map allowed registries to the index then there will be a small amount of extra work required once #4506 is merged.

@withoutboats, happy for this to be merged into your branch if you want, the main reason I did not base it on your branch was due to tests not working on there yet.

rust-highfive · Oct 3, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (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.

bors · Oct 3, 2017

☔️ The latest upstream changes (presumably #4559) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · Oct 4, 2017

Looks great! I think I'd like to try to throw this in with #4506 though as we'll want to be sure to feature gate the acceptance of a list of strings here I believe (and the feature gate bits were added in that PR)

cswindle · Oct 4, 2017

It looks like #4506 is getting close to merge, so maybe that should be merged, then I will add in feature gating to this PR when I merge the changes in.

alexcrichton · Oct 5, 2017

Ok sure yeah, either way's fine by me

withoutboats · Oct 5, 2017

I think we should probably merge #4506 separately. I have some big picture concerns about how we're handling publishing in general, which we're talking about on #4574, but also these smaller concerns:

I think this list should take registry names (as will be introduced in #4506), not index URLs, as @cswindle mentioned doing in the first comment. In general we're moving away from users dealing with index URLs as much as possible.

There's also the issue with passing registry.token raised in #4574. Basically, I think we should never pass registry.token to any registry tracked through one of those registries objects. I don't have an opinion about how you could declare an alternative token for those registries, either just add another key to the object (alongside index) or something with cargo/credentials, etc.

cswindle · Oct 5, 2017

I am fine switching to registry names, I will sort that out once your stuff is merged.

Regarding not pushing the token to an alternate registry, maybe it would be better just blocking sending the token if --index is provided (as that is the only way to send to an alternate registry) and instead --token must be provided in that situation. That sounds like it would offer protection from tokens being accidentally sent to the wrong server (until there is support for multiple registry tokens).

cswindle · Oct 5, 2017

In the case of providing a registry that we want to use in publish, I actually wonder if we should just make a minor tweak to registry_configuration to pass in the registry (if provided) and look up the "registry.<registry>.token" in the config, otherwise just get "registry.token". That way we can cross off the issue to support multiple registry tokens.

alexcrichton · Oct 5, 2017

@cswindle ah yeah it was my expectation that registry.token is basically synonymous with registry.crates-io.token (but not used for other registries)

cswindle · Oct 5, 2017

@alexcrichton I presume that is not something that would need to be done in an rfc and could just be implemented as part of this pull request (assuming resolution of #4574 is that we want to be able to push to private registries).

alexcrichton · Oct 5, 2017

@cswindle indeed yeah!

bors · Oct 25, 2017

☔️ The latest upstream changes (presumably #4506) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · Oct 27, 2017

Looks great! For now as well, could this be gated on the same alternate registries feature in the manifest?

cswindle · Oct 27, 2017

@alexcrichton, now that the alternate registries has landed I was planning on updating this to be gated on the feature in the manifest. I have been taking a look into adding support for --registry for publish and login and if that is available it actually makes the code to perform the registry lookup easier (assuming that it is ok to mandate that if you want to push to alternate registry you use --registry rather than --index), so I might try and get that in first.

alexcrichton · Oct 30, 2017

@cswindle ok sounds great! I think with a --registry argument we can definitely deprecate --index (which only ever existed for Cargo's own test suite anyway)

cswindle · Oct 30, 2017

See #4680 for the PR to add --registry support.

cswindle · Nov 1, 2017

I have now merged in my changes to add support for this. I am aware of the following two issues from further testing of this:

unable to publish when there are dependencies in another registry
we do not currently set the registry field in the index

I am in the process of sorting these last couple of bits out, so think it would make sense to do so in this pull request, rather than having another to do the last couple of bits.

cswindle · Nov 1, 2017

I believe that this should now allow publishing and can be reviewed.

alexcrichton · Nov 6, 2017

alexcrichton reviewed Nov 6, 2017

View changes

Looking great, thanks!

alexcrichton · Nov 6, 2017

src/cargo/ops/registry.rs

-        bail!("some crates cannot be published.\n\
-               `{}` is marked as unpublishable", pkg.name());
+    if let &Some(ref allowed_registries) = pkg.publish() {
+        if opts.registry.is_none() || !allowed_registries.contains(&opts.registry.clone().unwrap()) {


Perhaps this could be:

if let Some(ref registry) = opts.registry { if !allowed_registries.contains(registry) { bail!(...); } }

This will not work as if there are allowed registries setup, but we have not provided --registry then we also need to block. This covers the case of pushing to crates.io when publish is set.

Er so what I mostly mean here is getting rid of the is_none + clone + unwrap, basically restructuring this to avoid all the function calls (and make it a little easier to read)

I have got rid of the is_none, unwrap etc.

alexcrichton · Nov 6, 2017

src/cargo/core/source/source_id.rs

@@ -44,6 +44,8 @@ enum Kind {
    Path,
    /// represents a remote registry
    Registry,
+    /// represents a remote alternative registry
+    AltRegistry,


I'd ideally like to avoid having a separate source for alternative registries here, would it be possible to just use Registry above and recognize crates.io through an identifier like "crates-io"? Either that or perhaps a method like is_default_registry(&self) -> bool or something like that.

The problem is that in order to distinguish if it is the default registry in unit tests it becomes more of a pain as we can't purely check for if the index matches crates.io index. One change that I made as part of #4697 was to store the name in the source_id, that would then allow just checking if that was present, rather than there being an additional type, would you prefer that approach?

I updated this to use the name as that also fixed the bug that you highlighted.

alexcrichton · Nov 6, 2017

src/cargo/ops/registry.rs

+                    bail!("crates cannot be published to crates.io with dependencies sourced from other\n\
+                           registries either publish `{}` on crates.io or pull it into this repository\n\
+                           and specify it with a path and version\n\
+                           (crate `{}` is pulled from {}", dep.name(), dep.name(), dep.source_id());


While you're at it I think there's a missing ) in this message, mind throwing it in there?

I will make that change.

alexcrichton · Nov 6, 2017

src/cargo/util/toml/mod.rs

+                Some(vecstring.clone())
+            },
+            Some(VecStringOrBool::Bool(false)) => Some(vec![]),
+            _ => None,


Could this explicitly match None and Some(Bool(true)) just to be exhaustive?

alexcrichton · Nov 6, 2017

tests/alt-registry.rs

@@ -65,11 +65,13 @@ fn depend_on_alt_registry() {
    // Don't download a second time
    assert_that(p.cargo("build").masquerade_as_nightly_cargo(),
                execs().with_status(0).with_stderr(&format!("\
+[UPDATING] registry `{reg}`


Hm this looks like a bug? I don't think that an update should happen here for a clean build?

alexcrichton · Nov 6, 2017

tests/alt-registry.rs

@@ -91,7 +93,7 @@ fn depend_on_alt_registry_depends_on_same_registry() {
        .build();

    Package::new("baz", "0.0.1").alternative(true).publish();
-    Package::new("bar", "0.0.1").registry_dep("baz", "0.0.1", registry::alt_registry().as_str()).alternative(true).publish();
+    Package::new("bar", "0.0.1").dep("baz", "0.0.1").alternative(true).publish();


Hm how come this change was required?

This is required as using registry_dep includes the registry in the dependencies, but for when a crate is published to the same registry it should not include the registry in the dependencies (otherwise it makes it a lot more difficult to move the index).

Oh hm, this I thought was an explicit decision where an omitted key implies "crates.io" and an explicit key is required for any other registry. @withoutboats want to confirm though?

@alexcrichton, I did this way due to the following in the RFC:

If a dependency's registry is not specified, Cargo will assume the dependency can be located in the current registry. By specifying the registry of a dependency in the index, cargo will have the information it needs to fetch crate files from the registry indices involved without needing to involve an API server.

However, as this is optional, I will add an additional test to cover both when we do and don't have a registry set in the index.

alexcrichton · Nov 6, 2017

tests/alt-registry.rs

    assert_that(p.cargo("publish").masquerade_as_nightly_cargo()
-                 .arg("--index").arg(registry::alt_registry().to_string()),
-                execs().with_status(101));


Could this perhaps stick around as a separate test? I think we'll want an assertion that you can't publish a crate with dependencies on a separate registry to crates.io

I have added the test back.

cswindle · Nov 6, 2017

@alexcrichton, I have updated the code for most of the review comments, there are a couple where I have not updated, but in those cases I have explained why in the comments.

alexcrichton · Nov 7, 2017

r? @withoutboats

I think you'll want to take a final pass here

withoutboats · Nov 18, 2017

@bors: r+

Sorry to take a while to review. This all looks good to me, no further changes. needed :)

bors · Nov 18, 2017

📌 Commit cb37d33 has been approved by withoutboats

bors · Nov 18, 2017

⌛️ Testing commit cb37d33 with merge 6a1aee0...

bors · Nov 18, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: withoutboats
Pushing 6a1aee0 to master...

Add support for publish to take a registry that should be used.

Loading status checks…

345c03d

rust-highfive assigned matklad Oct 3, 2017

Merge branch 'master' into alt-registry-publish
Conflicts: src/cargo/core/package.rs

Loading status checks…

4f08943

cswindle referenced this pull request Oct 4, 2017
Closed
Tracking issue for RFC 2141: Add support to Cargo for alternative registries #44931
4 of 11 tasks complete

withoutboats added the A-registries label Oct 4, 2017

withoutboats referenced this pull request Oct 4, 2017
Closed
What is the significance of "specification" of index format/API? #4574

cswindle referenced this pull request Oct 23, 2017
Merged
Alternative registries #4506

Merge remote-tracking branch 'upstream/master' into alt-registry-publish
Conflicts: src/cargo/util/toml/mod.rs

Loading status checks…

77ca377

Include registry in dependencies in the index.

ef4a920

Use the correct interface for creating an alternative registry source…
… id.

Loading status checks…

cb01e03

cswindle added some commits Nov 6, 2017

Code review markups.

Loading status checks…

93c0d7c

Sort out bug with updating registry on a clean build.

Loading status checks…

4a302ac

rust-highfive assigned withoutboats and unassigned matklad Nov 7, 2017

Code review markups.

Loading status checks…

cb37d33

bors merged commit cb37d33 into rust-lang:master Nov 18, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

sfackler referenced this pull request Dec 21, 2017
Closed
`path` and `registry` shouldn't be mutually exclusive in dependency configurations #4843

rust-lang/cargo

Join GitHub today

Add support for publish to optionally take the index that can be used #4568

Conversation

cswindle commented Oct 3, 2017

rust-highfive assigned matklad Oct 3, 2017

This comment has been minimized.

rust-highfive commented Oct 3, 2017

This comment has been minimized.

bors commented Oct 3, 2017

cswindle referenced this pull request Oct 4, 2017

withoutboats added the A-registries label Oct 4, 2017

withoutboats referenced this pull request Oct 4, 2017

This comment has been minimized.

alexcrichton commented Oct 4, 2017

This comment has been minimized.

cswindle commented Oct 4, 2017

This comment has been minimized.

alexcrichton commented Oct 5, 2017

This comment has been minimized.

withoutboats commented Oct 5, 2017

This comment has been minimized.

cswindle commented Oct 5, 2017 • edited

This comment has been minimized.

cswindle commented Oct 5, 2017 • edited

This comment has been minimized.

alexcrichton commented Oct 5, 2017

This comment has been minimized.

cswindle commented Oct 5, 2017

This comment has been minimized.

alexcrichton commented Oct 5, 2017

cswindle referenced this pull request Oct 23, 2017

This comment has been minimized.

bors commented Oct 25, 2017

This comment has been minimized.

alexcrichton commented Oct 27, 2017

This comment has been minimized.

cswindle commented Oct 27, 2017

This comment has been minimized.

alexcrichton commented Oct 30, 2017

This comment has been minimized.

cswindle commented Oct 30, 2017

cswindle added some commits Oct 31, 2017

This comment has been minimized.

cswindle commented Nov 1, 2017 • edited

This comment has been minimized.

cswindle commented Nov 1, 2017

alexcrichton reviewed Nov 6, 2017 View changes

alexcrichton left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cswindle added some commits Nov 6, 2017

This comment has been minimized.

cswindle commented Nov 6, 2017

This comment has been minimized.

alexcrichton commented Nov 7, 2017

rust-highfive assigned withoutboats and unassigned matklad Nov 7, 2017

This comment has been minimized.

withoutboats commented Nov 18, 2017

This comment has been minimized.

bors commented Nov 18, 2017

bors added a commit that referenced this pull request Nov 18, 2017

This comment has been minimized.

bors commented Nov 18, 2017

This comment has been minimized.

cswindle commented Oct 5, 2017 •

edited

cswindle commented Oct 5, 2017 •

edited

cswindle commented Nov 1, 2017 •

edited

alexcrichton reviewed Nov 6, 2017

View changes

bors merged commit `cb37d33` into rust-lang:master Nov 18, 2017
3 checks passed