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.

Sign up for GitHub

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

Jump to bottom

rustup check #1980

Merged
merged 2 commits into from
Sep 16, 2019
Merged

rustup check #1980

merged 2 commits into from
Sep 16, 2019

Conversation

chunk0tronic
Copy link
Contributor

Hi, I was interested in being able to check for updates before running an update, and found that there was already a ticket open for this: #1249. I have had a go at implementing this feature in this PR.

The new command is rustup check and the output looks like this:
stable-x86_64-unknown-linux-gnu - Up to date : 1.37.0 (eae3437df 2019-08-13)
beta-x86_64-unknown-linux-gnu - Up to date : 1.38.0-beta.3 (72bfc3753 2019-08-27)
nightly-x86_64-unknown-linux-gnu - Update available : 1.39.0-nightly (fba38ac27 2019-08-31) -> 1.39.0-nightly (dfd43f0fd 2019-09-01)

The implementation gets the versions from the current local manifest and the downloaded manifest from the dist server. I added some test cases to the cli-rustup tests, I had to tweak the construction of the version string in the mock manifests to get this to work properly though.

I would appreciate any feedback you can give, some things to highlight:

  • I made some assumptions that since the ticket was open and I also wanted the feature, then you guys would too... so I hope you think it is still a useful feature!
  • I am not familiar with the history of the codebase, so I am not sure if the approach I have taken is a 'good' one
  • This only works if there is a V2 manifest, I'm not sure what a V1 manifest is TBH
  • I am quite new to rust, so I might have missed some idiomatic rust ways to do things
  • I wasn't sure whether I needed to add a handle_epipe around the check_updates function call and add a new ErrorKind, do you think this is necessary?

Thanks in advance for your comments.

Preview:
rustup_check

@kinnison
Copy link
Contributor

kinnison commented Sep 2, 2019

@chunk0tronic Thank you for your contribution to Rustup. We are indeed interested in seeing this work. It leads into some thoughts I was having regarding how we clobber channel data before we determine if we could update, but I'll worry about that another time -- this sounds like a useful bit of progress.

I'm not in a position to do a full review right now since I'm about to go to bed, but I wanted to pop on and say thank you and to say that this is on my radar and I'll try and get to review it soon. If you've not heard from me in a few days please do prod me. You can find me on the Rust lang discord, or on various IRC networks, always as 'Kinnison', or you can poke me via this PR :D

kinnison
kinnison suggested changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is very good. It's close to mergeable with a few small tweaks (See review comments).

In addition though, we prefer to not merge fixup commits if we can avoid them, so please can you ensure you rebase and clean up your commit history. It's nice that you provided a change commit and a test change commit; just fold the fixups and formatting into those.

tests/cli-rustup.rs Outdated Show resolved Hide resolved
tests/mock/clitools.rs Outdated Show resolved Hide resolved
@chunk0tronic
Copy link
Contributor Author

Thanks for your comments! I have rebased on upstream, made your requested updates and squashed the commits so there is still one for implementation and one for tests.
Please have another look, I am happy to make more changes if required. I'm quite excited, this is my first open source contribution and I have found it very satisfying to contribute.

@chunk0tronic
Copy link
Contributor Author

Just noticed my tests commit message needs updating, I'll tweak it and push again.

@bors
Copy link
Contributor

bors commented Sep 11, 2019

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

kinnison
kinnison reviewed Sep 12, 2019
View reviewed changes
tests/cli-exact.rs Outdated Show resolved Hide resolved
kinnison
kinnison suggested changes Sep 12, 2019
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny wart and one query, then 👍 I think.

tests/cli-rustup.rs Outdated Show resolved Hide resolved
@chunk0tronic
Copy link
Contributor Author

Ok, sliced off the wart and made a simplification based off your query, hope the explanation for how it is now makes sense. The windows test build failed for some reason, is this a real problem?

kinnison
kinnison approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kinnison
Copy link
Contributor

The Windows failure is a travis bogon.

@kinnison kinnison merged commit 9c264a1 into rust-lang:master Sep 16, 2019
@chunk0tronic
Copy link
Contributor Author

Yeah! Thanks @kinnison!

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

@kinnison kinnison kinnison approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chunk0tronic @kinnison @bors