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

Support set auto-self-update mode #2763

Merged
merged 1 commit into from
May 27, 2021

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented May 10, 2021

close #2576

  1. The PR supports three different modes of auto self update: enable/disable/check-only.
  • When enabled, commands such as install or update will update rustup itself.
  • When disable, commands such as install or update will not update rustup itself. Only self udpate will update rustup itself.
  • When check-only, commands such as install or update will not update rustup itself but will check for updates. Only self udpate will update rustup itself.
  1. This PR removes a large number of "--no-self-update" args in tests(setup will turn off auto self update by default.)
  2. Add tests for check-only and enable.
  3. add docs

@Rustin170506 Rustin170506 marked this pull request as draft May 10, 2021 06:24
@Rustin170506

This comment has been minimized.

@Rustin170506 Rustin170506 force-pushed the rustin-patch-slef-update branch 2 times, most recently from ae19fc2 to f46b487 Compare May 12, 2021 07:50
@Rustin170506 Rustin170506 marked this pull request as ready for review May 12, 2021 07:51
@Rustin170506 Rustin170506 changed the title Support self-update modes Support set auto-self-update modes May 12, 2021
@Rustin170506 Rustin170506 changed the title Support set auto-self-update modes Support set auto-self-update mode May 12, 2021
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
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.

There are a number of whitespace changes in this PR. I don't mind them too much, but in future it's best if this kind of reformatting is only done in independent commits/PRs.

I have made a number of wording change suggestions. You do not have to accept them all, but in general I think they're likely to improve matters.

doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
src/notifications.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

There are a number of whitespace changes in this PR. I don't mind them too much, but in future it's best if this kind of reformatting is only done in independent commits/PRs.

Got it.

@Rustin170506
Copy link
Member Author

Rustin170506 commented May 16, 2021

@kinnison Could you please take another look? All comments addressed. Thanks!

@Rustin170506
Copy link
Member Author

Conflicts have been resolved.

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.

A couple of doc tweaks and a question which might be "no need" or might lead to a slight code change - otherwise I think this is nearly ready to merge.

doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

Commits squashed.

doc/src/basics.md Outdated Show resolved Hide resolved
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.

Assuming CI goes green, I think this is okay. @rbtcollins ?

doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
src/cli/self_update.rs Show resolved Hide resolved
@Rustin170506 Rustin170506 force-pushed the rustin-patch-slef-update branch 2 times, most recently from 6f481f2 to a558f65 Compare May 24, 2021 01:10
src/cli/self_update.rs Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 force-pushed the rustin-patch-slef-update branch 2 times, most recently from d5b8ed7 to 2c7134f Compare May 25, 2021 07:33
@Rustin170506 Rustin170506 marked this pull request as draft May 25, 2021 07:52
@Rustin170506 Rustin170506 force-pushed the rustin-patch-slef-update branch 2 times, most recently from 80f9e55 to 31fd3c6 Compare May 25, 2021 08:15
@Rustin170506 Rustin170506 marked this pull request as ready for review May 25, 2021 08:16
Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

looking quite good

doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
doc/src/basics.md Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/self_update.rs Outdated Show resolved Hide resolved
src/settings.rs Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

Rustin170506 commented May 25, 2021

@rbtcollins All comments addressed. Thanks for your review!

@Rustin170506
Copy link
Member Author

Rustin170506 commented May 26, 2021

@rbtcollins @kinnison Can we merge it? Are there any other issues?

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 suggestion, otherwise I'm good with this. If @rbtcollins is happy then we can merge IMO.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
@Rustin170506 Rustin170506 force-pushed the rustin-patch-slef-update branch 2 times, most recently from b74cfea to d85d0be Compare May 27, 2021 13:36
@Rustin170506
Copy link
Member Author

It seems the docker image is broken.

Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

Minor nits - I'll fix these I think

Running `rustup update` also checks for updates to `rustup` itself and automatically
installs the latest version. To manually update `rustup` only,
without updating installed toolchains, type `rustup self update`:
If your `rustup` was build with the `no-self-update` feature, it will not be able to update itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be past tense

'was built with'

let arg0 = arg0
.as_ref()
.and_then(|a| a.to_str())
.ok_or(CLIError::NoExeFilePath)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have NoExeName : I don't think is different enough to bother with a new error (and as the error is never caught and never repeated, we can just use anyhow!(...)

test: clean up "--no-self-update" args
test: add check-only and enable tests
docs: update docs and add comment
@rbtcollins rbtcollins merged commit 494b307 into rust-lang:master May 27, 2021
@Rustin170506 Rustin170506 deleted the rustin-patch-slef-update branch May 27, 2021 23:30
@Rustin170506
Copy link
Member Author

@rbtcollins @kinnison Thanks again for your review!

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.

Setting to only check for a self-update during rustup update
3 participants