New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make crates-io registry URL optional in config; ignore all changes to source.crates-io #3089

Merged
merged 1 commit into from Sep 27, 2016

Conversation

Projects
None yet
4 participants
@carols10cents
Copy link
Member

carols10cents commented Sep 13, 2016

Hi! When I was working on the instructions for source replacement in this crates.io PR, I found that when I'm replacing source.crates-io, I still have to specify some value for registry, or else I get this:

error: no source URL specified for `source.crates-io`, need either `registry` or `local-registry` defined

This seems weird and annoying to me: cargo definitely knows the registry URL for crates-io, and I'm trying to replace it anyway.

So the first commit in this PR makes it optional, so that you don't have to specify a registry url for crates-io: it uses SourceId::crates_io, like it would if we didn't have any source configs at all.

The second commit in this PR might go too far, and/or might break existing uses of cargo, I'm not sure. In my opinion, source.crates-io should only be able to be replaced and never changed directly-- crates-io should always be crates-io, and I should be able to assume that in any project. So the second commit ignores all modifications to source.crates-io's registry, local-registry, and directory, and warns that they're being ignored.

I tried to search github to see if anyone was using these keys with source.crates-io, but since github's search ignores . (ARE YOU LISTENING GITHUB? I WOULD LIKE TO SEARCH WITH PUNCTUATION PLEASE), there's a lot of false positives to wade through. I didn't see anything in the first few pages though.

I'm happy to make whatever modifications to this!

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Sep 13, 2016

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 13, 2016

Thanks for the PR! This looks good to me, but perhaps we can hold off on enabling the warnings just yet? Now that support for custom registries is on beta (I think) we may want to wait for this to hit stable, update cargo vendor and cargo local-registry tools, and then turn on the warnings?

@carols10cents carols10cents force-pushed the carols10cents:crates-io-registry-url branch from 010c275 to fcfbfae Sep 18, 2016

@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Sep 18, 2016

Thanks for the PR! This looks good to me, but perhaps we can hold off on enabling the warnings just yet? Now that support for custom registries is on beta (I think) we may want to wait for this to hit stable, update cargo vendor and cargo local-registry tools, and then turn on the warnings?

Sounds good! I've removed the 2nd commit but I've got it on a branch for later resubmission.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 25, 2016

Hm this'll still cause existing configurations to start failing though, right? That is, they'll start interpreting as having two sources configured which is an error?

Put another way, the test changes shouldn't be required here, just enough to make warnings go away.

Should not have to specify crates-io registry URL to replace-with
Since cargo knows crates-io's registry URL and, anyway, you're trying to
say that you don't want to use crates-io.

@carols10cents carols10cents force-pushed the carols10cents:crates-io-registry-url branch from fcfbfae to 2490f3f Sep 26, 2016

@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Sep 26, 2016

Hm this'll still cause existing configurations to start failing though, right? That is, they'll start interpreting as having two sources configured which is an error?

Put another way, the test changes shouldn't be required here, just enough to make warnings go away.

I don't think this change will break existing configurations-- Cargo's current behavior is to silently ignore changes to crates.io's registry URL (but it requires you to specify a registry URL value, which is the problem I'm trying to solve because I shouldn't have to specify something that gets ignored anyway).

The test changes are not required. I've backed them out and the tests still pass, and instead I've added a new test to show that crates.io's registry URL being missing when replacing the source is no longer an error, which is the only behavior I'm intending this commit to change. This PR should have nothing to do with warnings anymore.

Am I missing something?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Sep 27, 2016

Oh oops! I read this condition:

if name == "crates-io" && srcs.is_empty()

and missed the second clause of src.is_empty(). If you always pushed that'd be an error w/ today, but with that there's no error at all! Sorry for the runaround!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 27, 2016

📌 Commit 2490f3f has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 27, 2016

⌛️ Testing commit 2490f3f with merge 26dcd3e...

bors added a commit that referenced this pull request Sep 27, 2016

Auto merge of #3089 - carols10cents:crates-io-registry-url, r=alexcri…
…chton

Make crates-io registry URL optional in config; ignore all changes to source.crates-io

Hi! When I was working on the instructions for source replacement [in this crates.io PR](rust-lang/crates.io#440), I found that when I'm replacing `source.crates-io`, [I still have to specify some value for `registry`](https://github.com/rust-lang/crates.io/pull/440/files#diff-04c6e90faac2675aa89e2176d2eec7d8R177), or else I get this:

```
error: no source URL specified for `source.crates-io`, need either `registry` or `local-registry` defined
```

This seems weird and annoying to me: cargo definitely knows the registry URL for crates-io, and I'm trying to replace it anyway.

So the first commit in this PR makes it optional, so that you don't have to specify a registry url for crates-io: it uses `SourceId::crates_io`, like it would if we didn't have any source configs at all.

~~The second commit in this PR might go too far, and/or might break existing uses of cargo, I'm not sure. In my opinion, `source.crates-io` should only be able to be replaced and never changed directly-- crates-io should always be crates-io, and I should be able to assume that in any project. So the second commit ignores all modifications to `source.crates-io`'s `registry`, `local-registry`, and `directory`, and warns that they're being ignored.~~

~~I tried to search github to see if anyone was using these keys with `source.crates-io`, but since github's search ignores `.` (ARE YOU LISTENING GITHUB? I WOULD LIKE TO SEARCH WITH PUNCTUATION PLEASE), there's a lot of false positives to wade through. I didn't see anything in the first few pages though.~~

I'm happy to make whatever modifications to this!
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 27, 2016

@bors bors merged commit 2490f3f into rust-lang:master Sep 27, 2016

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
@carols10cents

This comment has been minimized.

Copy link
Member

carols10cents commented Sep 27, 2016

Hooray!!!! Thank you :) <3 <3 <3

@carols10cents carols10cents deleted the carols10cents:crates-io-registry-url branch Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment