Skip to content
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

Try to add links to the index #1248

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Try to add links to the index #1248

merged 1 commit into from
Feb 27, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented Jan 28, 2018

This is a companion to rust-lang/cargo#4978
It is adding the links attribute to the index git. I am working on windows so cannot test locally, I.E. this may involve some sparing with the CI to get this green. Advice and help are welcome!

@carols10cents
Copy link
Member

Ok, seems like we should wait to see how rust-lang/cargo#4978 shakes out before merging this in.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Feb 24, 2018

rust-lang/cargo#4978 was just r+ed. so I think this is the next step, well except testing that adding links to the index will not break old versions of cargo.

@Eh2406
Copy link
Contributor Author

Eh2406 commented Feb 26, 2018

Ci is green, what's the next step? Dose this need a test, if so where?

@carols10cents
Copy link
Member

carols10cents commented Feb 26, 2018

Tests would be good, around where crate publishing is currently tested.

@alexcrichton what are the implications of merging and deploying this change?

@Eh2406
Copy link
Contributor Author

Eh2406 commented Feb 26, 2018

@carols10cents Thanks for your help on IRC. What is the best way to test this? I assumed I would publish a crate with a links and check that the index has the links. But, I am not seeing anything that tests the contents of the index to work off of. (Testing is not my strongest suit.)

A little bit of background, rust-lang/cargo#4978 added 2 things to cargo:

  1. If the record in the index has a links attribute then the resolver will make sure that each link only appears one time. This will reduce the occurrence of lock files that don't work do to duplicated links attributes.
  2. When publishing a crate provide the links attribute. So that crates.io can put it in the index.

This pr puts them together. If cargo provides a links attribute then it adds it to the index.

I have tested that adding a links attribute to the index does not brake old versions of cargo.

Anything else I can answer?

@alexcrichton
Copy link
Member

@carols10cents we should be all good for deploying this! I don't believe there should be any gotchas in terms of Cargo support to result from this.

@carols10cents
Copy link
Member

But, I am not seeing anything that tests the contents of the index to work off of.

Yeah, that's what I was afraid of. We should add some eventually, but given that this code is mostly serde logic, I'm fine with merging this for now and working on adding those tests for the links attribute and everything else eventually.

we should be all good for deploying this!

Ok! merging and will deploy sometime today!

bors: r+

bors-voyager bot added a commit that referenced this pull request Feb 27, 2018
1248: Try to add links to the index r=carols10cents

This is a companion to rust-lang/cargo#4978
It is adding the links attribute to the index git. I am working on windows so cannot test locally, I.E. this may involve some sparing with the CI to get this green. Advice and help are welcome!
@bors-voyager
Copy link
Contributor

bors-voyager bot commented Feb 27, 2018

Build succeeded

@bors-voyager bors-voyager bot merged commit 621ca98 into rust-lang:master Feb 27, 2018
@carols10cents
Copy link
Member

Deployed!

@Eh2406
Copy link
Contributor Author

Eh2406 commented Feb 27, 2018

Thanks for all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants