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

Add --path option to 'rustup override set' #1524

Merged
merged 2 commits into from
Apr 15, 2019

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Oct 9, 2018

Same as --path in 'rustup override unset'

@pickfire pickfire force-pushed the toolchain-set-path branch 2 times, most recently from 72bb48a to 5f8930e Compare October 9, 2018 16:39
@kinnison
Copy link
Contributor

kinnison commented Jan 6, 2019

Hi @pickfire

Are you still interested in persuing this change? If so, you need to rebase it against the current master (We've had a bit of a reformat of code so it's conflicting). Also it'd be nice if you could add a test to cover the new --path functionality.

@pickfire
Copy link
Contributor Author

pickfire commented Jan 7, 2019

@kinnison I am interested but I thought rustup.rs might not accept the changes since it's kinda not accepting changes last time IIRC.

If the patch could be accepted (I could fix the stuff), I am still interested in this but probably need some time since I've been a bit busy lately.

@kinnison
Copy link
Contributor

kinnison commented Jan 7, 2019

I think it's fair to say that merges will not be fast into this repo because the maintainers are busy elsewhere a lot of the time, but I at least am interested in sorting improvements out, and @nrc is able to look in and merge stuff if it's clean and clear :-D I guess step one would be to rebase onto current master and organise a test or two. Once that's done, I'm prepared to help get it mergeable if @nrc wants anything else doing first.

@bors
Copy link
Contributor

bors commented Jan 8, 2019

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

@dwijnand
Copy link
Member

Same as --path in 'rustup override unset'

If this is fixing feature parity then I'd expect it to be accepted. I agree it would be good for this to come with a test (as well as needing another rebase.. 😕)

@pickfire
Copy link
Contributor Author

pickfire commented Feb 2, 2019

I have rebased against master but not sure what to test though, the current unset does not have tests for path as well.

@kinnison
Copy link
Contributor

kinnison commented Feb 3, 2019

Thanks for the rebase, and I'm glad to see the green tick. I'll give some thought to what testing might be appropriate, given you say there's also a gap in the override unset testing too.

@bors
Copy link
Contributor

bors commented Mar 15, 2019

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

@pickfire
Copy link
Contributor Author

@kinnison What do I do now? Resolve the merge conflict again?

@kinnison
Copy link
Contributor

I've been unable to give any time to thinking about this particular PR, which is sad. Re-reading it I'd say that we agreed on what was missing (tests) but not on what those tests would look like or who would be responsible for them. If you lack time to speculatively address these issues then please just let the PR sit for now.

It's possible someone else will come along who wants to work on Rustup and has the time to work it through. A rebase ought not to be too painful (though Dale has rearranged the codebase now so it's not going to be entirely trivial), but the tests remain the sticking point.

@dwijnand

This comment has been minimized.

@pickfire
Copy link
Contributor Author

@dwijnand Thanks a lot for helping out.

@kinnison I have no issues rebasing, I am just not sure what tests to write for this.

@kinnison
Copy link
Contributor

Regarding tests, the main thing is that we should verify that rustup override {un,}set --path ... works correctly. This probably means creating a test which sets up a directory with an override, and then verifies that it works, and then unsets it and verifies the override is gone.

However, if you're really stuck with how to do that, just hold the rebased branch for now until someone can help you with the tests. Sadly I don't have enough time right now to provide an example.

@bors
Copy link
Contributor

bors commented Apr 14, 2019

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

Same as --path in 'rustup override unset'
@pickfire
Copy link
Contributor Author

I added test for both set and unset only for --path.

@kinnison
Copy link
Contributor

Hi @pickfire

Thanks for updating the branch, but the Windows test (Appveyor) failed. From a glance it looks like you maybe hard-coded an architecture name in your test? I will try and review the code later to help you out if you can't spot the bug.

D.

@pickfire
Copy link
Contributor Author

@kinnison Could you please help me to check what to change? I am not so rich to buy a windows laptop, I don't have one to test it out (as well I am not very familiar with windows).

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.

Other than the test issue, this LGTM.

tests/cli-rustup.rs Outdated Show resolved Hide resolved
@kinnison kinnison merged commit 5d1db84 into rust-lang:master Apr 15, 2019
@pickfire pickfire deleted the toolchain-set-path branch April 16, 2019 16:06
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.

4 participants