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

Projects
None yet
4 participants
@pickfire
Copy link
Contributor

commented Oct 9, 2018

Same as --path in 'rustup override unset'

@pickfire pickfire force-pushed the pickfire:toolchain-set-path branch 2 times, most recently from 72bb48a to 5f8930e Oct 9, 2018

@kinnison

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

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

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

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 pickfire force-pushed the pickfire:toolchain-set-path branch from 5f8930e to 8944f27 Feb 2, 2019

@pickfire

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

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

@pickfire

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

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

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Mar 17, 2019

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 was marked as outdated.

Copy link
Member

commented Mar 17, 2019

Rebasing over file moves should be trivial.

@dwijnand dwijnand force-pushed the pickfire:toolchain-set-path branch from 8944f27 to 92c9bc7 Mar 17, 2019

@pickfire

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Mar 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

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

Add --path option to 'rustup override set'
Same as --path in 'rustup override unset'

@pickfire pickfire force-pushed the pickfire:toolchain-set-path branch from 92c9bc7 to 1e6e08b Apr 15, 2019

@pickfire

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

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

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@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).

@kinnison
Copy link
Collaborator

left a comment

Other than the test issue, this LGTM.

Show resolved Hide resolved tests/cli-rustup.rs Outdated

@kinnison kinnison merged commit 5d1db84 into rust-lang:master Apr 15, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@pickfire pickfire deleted the pickfire:toolchain-set-path branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.