Add an entry to the installed programs on windows #2670

kellda · 2021-02-17T08:48:10Z

kellda · 2021-02-17T08:55:58Z

I have a few questions about how this should be implemented:

Where should the functions I added be called ?
Should they be gated on some option (like for modifying the path)
What should be displayed in the list of installed programs ? (e.g. "rustup" or "The Rust Programming Language")
Should the registry key be a fixed string (easier to find in the registry) or a randomly generated GUID (easier to have multiple installations of rustup) ?

kinnison · 2021-02-20T11:00:53Z

Taking your questions in order:

These functions likely will end up called from cli::self_update::install and cli::self_update::windows::complete_windows_uninstall though these are just guesses right now
I feel like adding to the windows registry uninstall set should probably just be done, but one big problem is that if people installed without modifying the path then they might just choose to delete the folders. Perhaps only adding to the registry if PATH was modified makes sense.
I would suggest rustup - Rust toolchain manager or similar
My gut feeling is that in general there'll only ever be one installation of rustup on a Windows system. HOWEVER we do support more than one, and nominally people could have several. I'd suggest, if possible, that we work on the basis of a GUID derived from the combo of CARGO_HOME and RUSTUP_HOME though I don't know how that'd go, and if we did that, then the text for 3 would need to be augmented with those values so that the user knew which they were uninstalling.

Ideally we'd get someone who actually uses Windows seriously, rather than treats it like a toy OS (me), to weigh in with some understanding of how people expect this kind of thing to present itself. Otherwise we're just going to be speculating as to what might be best, without being informed.

Thank you for your work thus-far.

kellda · 2021-02-24T10:58:34Z

About 1. and 2., I think it makes sense to call these functions in cli::self_update::[un]install, just after do_[add_to/remove_from]_path.

sounds good.

About 4., I was mainly thinking about testing rustup while developing it and in the test suite. On Windows most programs just uninstall any previous version before installing themselves. I'm for adding uninstall information only if it doesn't already contain valid ones and only remove it if it points to the installation we're removing.

I think rustup uninstall RUSTUP_HOME regardless of where is the called installation. Is this correct ?

kinnison · 2021-03-11T19:54:04Z

Sorry it has taken so long to get back to you on this one. I'm struggling to cope with the fact that Windows is making us conflate global and local configuration to some extent. rustup can be installed multiple times, and I think each uninstall entry will have to handle the specific RUSTUP_HOME and CARGO_HOME it's related to - however the installations could feasibly overlap (it'd be broken to do so, but it's at least slightly possible to some extent) - I'm beginning to wonder if I was right to ask about coping with uninstalling multiple variants. Perhaps we should go back to your much simpler original idea of a single entry?

kellda · 2021-03-11T20:06:08Z

I also think that it is not really needed nor useful to have multiple entries in the registry when there is multiple installations. IMO there should be one "main" installation in the registry, the one that was installed first and that's called when RUSTUP_HOME is not set/overridden.

kinnison · 2021-03-11T20:38:33Z

So only set the uninstall in the registry if RUSTUP_HOME and CARGO_HOME are not overridden in the environment? That sounds plausible.

kellda · 2021-03-11T20:48:04Z

I didn't implemented it that way, but I can do it so.

What is currently implemented is:

Check if there is already an entry in the registry
If there isn't, add uninstall information
If the entry points to a valid location, don't modify the registry
If the entry is invalid, override it with info of this installation

And on uninstall, only remove the registry's entry if it points to the currently running executable.

Is it better to check the state of env variables or that of the registry ?

kinnison · 2021-03-11T21:06:43Z

I'm afraid I'm not very good at reasoning out what a Windows user would expect here. @rbtcollins Do you happen to have any insight which might help?

src/cli/self_update.rs

rbtcollins · 2021-03-11T21:38:34Z

src/cli/self_update/windows.rs

+    use std::path::PathBuf;
+
+    let key = RegKey::predef(HKEY_CURRENT_USER)
+        .create_subkey(r"Software\Microsoft\Windows\CurrentVersion\Uninstall\rustup")


This should be Rustup, not rustup, as the project name is capitalised.

You're talking about the registry path, right ? What about the text displayed in the list of installed programs (rustup - Rust toolchain manager or Rustup - Rust toolchain manager)

By the way, I can't see any place where the project name is capitalised.

rustup.rs:

rustup is an installer for...

rustup is an official Rust project.

README.md:

rustup: the Rust toolchain installer

Rustup please. We discussed this on discord prior to my saying this in the review; I realise the prior art in the repo may be inconsistent.

src/cli/self_update/windows.rs

rbtcollins · 2021-03-11T21:46:15Z

src/cli/self_update/windows.rs

+    let mut path = utils::cargo_home()?;
+    path.push("bin\rustup.exe");
+    let mut uninstall_cmd = path.into_os_string();
+    uninstall_cmd.push(" self uninstall");


This doesn't seem to correctly quote the path if the home directory contains spaces. I'm not sure if we can use a REG_MULTI_SZ here? That would be better.

REG_MULTI_SZ only use the first string as if it was a REG_SZ, so it doesn't help here. Quoting the path should work.

src/cli/self_update/windows.rs

rbtcollins · 2021-03-11T21:49:43Z

src/cli/self_update/windows.rs

+
+    let root = RegKey::predef(HKEY_CURRENT_USER);
+
+    let key = match root.open_subkey(r"Software\Microsoft\Windows\CurrentVersion\Uninstall\rustup")


lets factor this out into a const or static

rbtcollins · 2021-03-11T21:50:00Z

src/cli/self_update/windows.rs

+    {
+        Ok(key) => key,
+        Err(ref e) if e.kind() == io::ErrorKind::NotFound => return Ok(()),
+        Err(e) => return Err(e).chain_err(|| ErrorKind::PermissionDenied),


error folding...

rbtcollins · 2021-03-11T21:53:58Z

src/cli/self_update/windows.rs

+        Err(e) => return Err(e).chain_err(|| ErrorKind::PermissionDenied),
+    };
+
+    // Don't remove uninstall information from another installation


I think we should remove this: if someone has moved their homedir around for whatever reason, this logic will make it impossible to remove the entry from their programs list - and thats super annoying.

OTOH removing an entry thats not for a parallel install done using CARGO_HOME + RUSTUP_HOME variables, is somewhat harmless, since the user can still use rustup, and still uninstall directly, as long as we document that.

rbtcollins · 2021-03-11T21:59:14Z

@kinnison review done. rustup cannot be installed multiple times today. Well, not meaningfully. You just think it can :P.

The PATH behaviour really means that rustup is only installed once: the first entry on the path wins, and anything else loses. Some Windows programs both add themselves to the registry PATH and offer a batch script to add themselves to the process environment block PATH by spawning a command shell, setting the variables and dropping the user into it, thus allowing the user to work around the PATH first-wins issue.

So someone setting custom CARGO_HOME and RUSTUP_HOME - both on Windows and elsewhere - really is taking responsibility for managing the definition of installed, and I think our best bet is just to ignore that.

rbtcollins

Once you've done this change please rebase - we don't want the incremental changes that lead the PR, just the clear set of changes.

rbtcollins · 2021-03-12T13:31:15Z

src/cli/self_update.rs

@@ -344,6 +344,7 @@ pub fn install(

        if !opts.no_modify_path {
            do_add_to_path()?;


I think the add to programs should be done first, before adding to path.

rbtcollins · 2021-03-12T13:33:31Z

src/cli/self_update/windows.rs

+        .chain_err(|| ErrorKind::PermissionDenied)?
+        .0;
+
+    // Don't overwrite registry if Rustup is already installed


I'm not certain this is right, but for now lets leave it. I think it might be more that we want to determine default RUSTUP_HOME being overridden etc, and otherwise we should be writing the current correct value into the registry each time.

What would be the "current correct value"? As someone who overrides RUSTUP_HOME and use temporary installs of rustup in parallel of the standard one, I wouldn't want those temporary installs to replace the standard one in the registry.

A later install wont override a previous one, as long as the previous install is still installed.
However when uninstalling, the registry key will be unconditionally removed, and then recreated on the next install. See #2670 (comment)

By the way, any rustup self uninstall will unistall rustup in the current RUSTUP_HOME, regardless of where the executable you're running is.

kellda force-pushed the uninstall branch from 41c5f1b to c493f76 Compare February 24, 2021 17:01

rbtcollins requested changes Mar 11, 2021

View reviewed changes

kellda force-pushed the uninstall branch 3 times, most recently from b2f0285 to 3363086 Compare March 12, 2021 12:34

kellda requested a review from rbtcollins March 12, 2021 13:11

rbtcollins requested changes Mar 12, 2021

View reviewed changes

Add an entry to the installed programs on windows

ebbe961

kellda force-pushed the uninstall branch from 3363086 to ebbe961 Compare March 12, 2021 13:54

kellda marked this pull request as ready for review March 12, 2021 13:55

rbtcollins merged commit 6fccc01 into rust-lang:master Mar 12, 2021

kellda deleted the uninstall branch March 12, 2021 15:02

workingjubilee mentioned this pull request Apr 12, 2021

docs, comments: Editing and style cleanup #2715

Merged

pietroalbini mentioned this pull request Apr 15, 2021

Announce Rustup 1.24.0 rust-lang/blog.rust-lang.org#816

Merged

kellda mentioned this pull request Nov 3, 2022

Outdated information about rustup rust-lang/rust-forge#659

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an entry to the installed programs on windows #2670

Add an entry to the installed programs on windows #2670

kellda commented Feb 17, 2021

kellda commented Feb 17, 2021

kinnison commented Feb 20, 2021

kellda commented Feb 24, 2021

kinnison commented Mar 11, 2021

kellda commented Mar 11, 2021

kinnison commented Mar 11, 2021

kellda commented Mar 11, 2021

kinnison commented Mar 11, 2021

rbtcollins Mar 11, 2021

kellda Mar 12, 2021

kellda Mar 12, 2021

rbtcollins Mar 12, 2021

rbtcollins Mar 11, 2021

kellda Mar 12, 2021

rbtcollins Mar 11, 2021

rbtcollins Mar 11, 2021

rbtcollins Mar 11, 2021

rbtcollins commented Mar 11, 2021

rbtcollins left a comment

rbtcollins Mar 12, 2021

rbtcollins Mar 12, 2021

glandium Apr 28, 2021

kellda Apr 28, 2021


		let root = RegKey::predef(HKEY_CURRENT_USER);

		let key = match root.open_subkey(r"Software\Microsoft\Windows\CurrentVersion\Uninstall\rustup")

		@@ -344,6 +344,7 @@ pub fn install(

		if !opts.no_modify_path {
		do_add_to_path()?;

Add an entry to the installed programs on windows #2670

Add an entry to the installed programs on windows #2670

Conversation

kellda commented Feb 17, 2021

kellda commented Feb 17, 2021

kinnison commented Feb 20, 2021

kellda commented Feb 24, 2021

kinnison commented Mar 11, 2021

kellda commented Mar 11, 2021

kinnison commented Mar 11, 2021

kellda commented Mar 11, 2021

kinnison commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins commented Mar 11, 2021

rbtcollins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment