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

Always replace metadata when replacing package #5564

Merged
merged 1 commit into from
May 25, 2018

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented May 24, 2018

Fixes #4582

I'm having problem writing test for it.
The test should install binary, make commit and reinstall binary, this part is done.
To know if it was done properly we need to compare git revision of HEAD and installed binary and that's where the problems begin...

@rust-highfive
Copy link

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

@alexcrichton
Copy link
Member

Perhaps the manifest file could be manually read here to assert that the updated revision is reflected there?

@mati865
Copy link
Contributor Author

mati865 commented May 24, 2018

I'm not familiar with cargo code base. I came here because of nabijaczleweli/cargo-update#55 to be honest.

Could you point me where should I start searching?

@alexcrichton
Copy link
Member

Ah no worries! In the tests I believe you can use paths::home and std::fs to read the crate manifest and basically just assert that the revision you expected is located within the manifest

@mati865
Copy link
Contributor Author

mati865 commented May 24, 2018

I thought about cargo function parsing .crates.toml but yeah you solution should be simpler.

@mati865
Copy link
Contributor Author

mati865 commented May 25, 2018

Updated test does fail for non patched code and succeeds with fix applied.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented May 25, 2018

📌 Commit 059107e has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 25, 2018

⌛ Testing commit 059107e with merge 4c8b8ac...

bors added a commit that referenced this pull request May 25, 2018
Always replace metadata when replacing package

Fixes #4582

I'm having problem writing test for it.
The test should install binary, make commit and reinstall binary, this part is done.
To know if it was done properly we need to compare git revision of HEAD and installed binary and that's where the problems begin...
@bors
Copy link
Contributor

bors commented May 25, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 4c8b8ac to master...

@bors bors merged commit 059107e into rust-lang:master May 25, 2018
@mati865
Copy link
Contributor Author

mati865 commented May 25, 2018

Thanks 👍
When it would be appropriative to update cargo submodule for rust again?
It's just a small QoL improvement for cargo-update users but last update was 9 days ago anyway.
I can open PR if you wish.

@mati865 mati865 deleted the metadata_fix branch May 25, 2018 16:31
@alexcrichton
Copy link
Member

Certainly! A PR any time is fine, it's done on an as-needed basis

@mati865
Copy link
Contributor Author

mati865 commented May 25, 2018

Opened rust-lang/rust#51062

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

Successfully merging this pull request may close these issues.

Cargo install -f --git doesn't update crate metadata
5 participants