Replace `winreg` with `windows-registry`

[InfyniteHeap]

Closes #3779.

Since there are probably some issues in winreg crate, I changed this dependency to windows-registry, which is developed by Microsoft.

[djc]

Sorry, but it turns out I had to make some changes to registry-related API in #3893. Hopefully it won't be too hard to rebase on top of my changes.

[InfyniteHeap]

Sorry, but it turns out I had to make some changes to registry-related API in #3893. Hopefully it won't be too hard to rebase on top of my changes.

Oh, I think I'd better replace dependency from scratch lest appear some unexpect problems.😅

[InfyniteHeap]

This PR will keep closed until windows-registry updates.

[rami3l]

@InfyniteHeap FYI https://github.com/microsoft/windows-rs/issues/3119 has been closed, so you might restart your work on this one!

[InfyniteHeap]

@InfyniteHeap FYI microsoft/windows-rs#3119 has been closed, so you might restart your work on this one!

Sounds great! I'll restart my work when new version of windows-registry is published on crates.io! :)

[rami3l]

@InfyniteHeap According to https://github.com/microsoft/windows-rs/issues/3148#issuecomment-2263527164, you may now test how the upstream change works with this PR via Cargo.toml's [patch] section.

[InfyniteHeap]

@InfyniteHeap According to microsoft/windows-rs#3148 (comment), you may now test how the upstream change works with this PR via Cargo.toml's [patch] section.

windows-rs is not a single crate but a workspace with all windows crates included, but this way seems only accepts the git repo which is a single crate. So, I cloned windows-rs on my computer and manually set dependencies to test.

[rami3l]

@InfyniteHeap According to microsoft/windows-rs#3148 (comment), you may now test how the upstream change works with this PR via Cargo.toml's [patch] section.

windows-rs is not a single crate but a workspace with all windows crates included, but this way seems only accepts the git repo which is a single crate. So, I cloned windows-rs on my computer and manually set dependencies to test.

@InfyniteHeap Hmmm that shouldn't matter IIRC. I've done the same thing for aws-lc-rs just a few weeks ago, and that was also a monorepo. Cargo knows from the monorepo's Cargo.toml how to get to the library by its name.

[rami3l]

Hmmm that shouldn't matter IIRC. I've done the same thing for aws-lc-rs just a few weeks ago, and that was also a monorepo. Cargo knows from the monorepo's Cargo.toml how to get to the library by its name.

@InfyniteHeap FWIW here's the commit: https://github.com/rust-lang/rustup/pull/3917/commits/3e7b23a4b0a2162c3271e10d2bd0365b3fd13755

[InfyniteHeap]

The issue of Cargo.toml format will be resolved when code itself is ready to be merged into upstream repo.

[InfyniteHeap]

Now all the CI tests about code itself are passed, so, I think it's suitable to review where should I modify.

[InfyniteHeap]

@InfyniteHeap Thanks! Now this PR looks much better to me than the previous versions. However I still cannot merge it since you're using a Cargo patch.

For the moment being, to better prepare for this PR, it should be reorganized in the form of atomic commits such that:

1. Every commit is a unit of change that only does one thing and does it well, and thus can be easily reverted;
2. The commits should be ordered properly so that every one gives a green CI.

You can refer to this comment (https://github.com/rust-lang/rustup/pull/3980#discussion_r1702752864) and the resulting commit history to better understand what is expected from your commit history.

Many thanks in advance!

Hint: In general, you should use Git's interactive rebase to rewrite the history.

Yes, I'll tidy up this PR once the new version of windows-registry is published.

As of the interactive rebase, I don't know why but when I rebasing commit histories, the git (exactly, the gnupg) always prompts me that I don't have a secret key, even though I indeed have one. I think whether it's because I am using a Windows computer. I'll try using WSL instead. That's why I frequently force-push (and this lead to this PR be frequently closed) commits. Apologize for this!

This is what I encountered:

PS: I've installed gnupg from this website, and I successfully generated a gpg key. But it is strange that git itself cannot sign the commits, but github desktop and visual studio can.

[rami3l]

@InfyniteHeap Have you tried docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key?platform=windows?

[InfyniteHeap]

@InfyniteHeap Have you tried docs.github.com/en/authentication/managing-commit-signature-verification/telling-git-about-your-signing-key?platform=windows?

Yes, I tried (even several times), and that's what I annoyed: It still doesn't work! I also checked the .gitconfig file and can confirm that the key exists in it (if it doesn't exist, the github desktop and vs also cannot sign commits).

Sometimes, when I directly call gpg on my computer, it cannot be launched and the terminal shows me the IPC connecting errors.

[InfyniteHeap]

@rami3l I tested on WSL and succeeded in signing commits! Actually, I also cannot sign commits when first opening WSL, but after I referred to this doc, I eventually succeeded!😄

[rami3l]

@InfyniteHeap Regarding the formatting errors, you're expected to use taplo-cli v0.9.3 for this. If you're using the VSCode extension, you can install a new version of taplo-cli yourself (via cargo or your package manager of choice) and choose not to use the (now outdated) bundled taplo in the TOML extension's configuration. taplo's project owner is AFK and so new versions of the VSCode extension still cannot be published properly. This is the workaround I can think of for the moment being.

[rami3l]

@InfyniteHeap A gentle ping: this branch is in conflict now, which means you need to rebase this PR to get your CI checks back.

[InfyniteHeap]

@InfyniteHeap A gentle ping: this branch is in conflict now, which means you need to rebase this PR to get your CI checks back.

According to the conflict contents, I think this conflict will be automatically resolved once I use the newer version of windows-registry and remove the [patch] table. If it doesn't resolved, then I'll try rebasing this PR.

[InfyniteHeap]

@InfyniteHeap A gentle ping: this branch is in conflict now, which means you need to rebase this PR to get your CI checks back.

According to the conflict contents, I think this conflict will be automatically resolved once I use the newer version of windows-registry and remove the [patch] table. If it doesn't resolved, then I'll try rebasing this PR.

Ok, well, I've resolved it. :P

[InfyniteHeap]

This CI was failed, because while reqwest v0.12.7 (the newest version so far) adpoed windows-registry v0.2, this PR adopts the dependency coming from master branch. Comparing to windows-registry v0.2, the master-branch one had introduced some breaking API changes. So, I'll either try making reqwest use newer version of windows-registry after windows-rs publishing new release, or stick the version of reqwest to v0.12.5, which hadn't adopted windows-registry yet at that time.

[kennykerr]

This CI was failed, because while reqwest v0.12.7 (the newest version so far) adpoed windows-registry v0.2, this PR adopts the dependency coming from master branch. Comparing to windows-registry v0.2, the master-branch one had introduced some breaking API changes. So, I'll either try making reqwest use newer version of windows-registry after windows-rs publishing new release, or stick the version of reqwest to v0.12.5, which hadn't adopted windows-registry yet at that time.

You're probably running into this: https://github.com/microsoft/windows-rs/pull/3214

[ChrisDenton]

It looks like HSTRING can simplify things and remove the need for unsafe. E.g.:

diff

diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs
index 035d308c..c6c91fa0 100644
--- a/src/cli/self_update/windows.rs
+++ b/src/cli/self_update/windows.rs
@@ -5,14 +5,13 @@ use std::io::Write;
 use std::os::windows::ffi::{OsStrExt, OsStringExt};
 use std::path::Path;
 use std::process::Command;
-use std::slice;
 use std::sync::{Arc, Mutex};
 #[cfg(any(test, feature = "test"))]
 use std::sync::{LockResult, MutexGuard};

 use anyhow::{anyhow, Context, Result};
 use tracing::{info, warn};
-use windows_registry::{Key, Type, Value, CURRENT_USER};
+use windows_registry::{Key, Type, Value, CURRENT_USER, HSTRING};
 use windows_result::HRESULT;
 use windows_sys::Win32::Foundation::ERROR_FILE_NOT_FOUND;

@@ -480,7 +479,8 @@ fn _apply_new_path(new_path: Option<Vec<u16>>) -> Result<()> {
     if new_path.is_empty() {
         environment.remove_value("PATH")?;
     } else {
-        environment.set_bytes("PATH", Type::ExpandString, &to_winreg_bytes(new_path))?;
+        let new_path = HSTRING::from_wide(&new_path);
+        environment.set_expand_hstring("PATH", &new_path)?;
     }

     // Tell other processes to update their environment
@@ -620,12 +620,8 @@ pub(crate) fn do_add_to_programs(process: &Process) -> Result<()> {
     uninstall_cmd.push(path);
     uninstall_cmd.push("\" self uninstall");

-    key.set_bytes(
-        "UninstallString",
-        Type::String,
-        &to_winreg_bytes(uninstall_cmd.encode_wide().collect()),
-    )
-    .context("Failed to set `UninstallString`")?;
+    key.set_hstring("UninstallString", &HSTRING::from(&uninstall_cmd))
+        .context("Failed to set `UninstallString`")?;
     key.set_string("DisplayName", "Rustup: the Rust toolchain installer")
         .context("Failed to set `DisplayName`")?;
     do_update_programs_display_version(env!("CARGO_PKG_VERSION"))?;
@@ -641,22 +637,13 @@ pub(crate) fn do_remove_from_programs() -> Result<()> {
     }
 }

-/// Convert a vector UCS-2 chars to a null-terminated UCS-2 string in bytes
-pub(crate) fn to_winreg_bytes(mut v: Vec<u16>) -> Vec<u8> {
-    v.push(0);
-    unsafe { slice::from_raw_parts(v.as_ptr().cast::<u8>(), v.len() * 2).to_vec() }
-}
-
 /// This is used to decode the value of HKCU\Environment\PATH. If that key is
 /// not REG_SZ | REG_EXPAND_SZ then this returns None. The winreg library itself
 /// does a lossy unicode conversion.
 pub(crate) fn from_winreg_value(val: Value) -> Option<Vec<u16>> {
     match val.ty() {
         Type::String | Type::ExpandString => {
-            // Copied from winreg
-            let mut words = unsafe {
-                slice::from_raw_parts(val.as_ptr().cast::<u16>(), val.as_ref().len() / 2).to_owned()
-            };
+            let mut words = Vec::from(val.as_wide());
             while words.last() == Some(&0) {
                 words.pop();
             }
@@ -922,7 +909,7 @@ mod tests {
         let environment = CURRENT_USER.create("Environment").unwrap();
         let path = environment.get_value("PATH").unwrap();
         assert_eq!(path.ty(), Type::ExpandString);
-        assert_eq!(super::to_winreg_bytes(wide("foo")), path.as_ref());
+        assert_eq!(HSTRING::from("foo").as_wide(), path.as_wide());
     }

     #[test]
@@ -932,11 +919,7 @@ mod tests {
         let _guard = RegistryGuard::new(&USER_PATH);
         let environment = CURRENT_USER.create("Environment").unwrap();
         environment
-            .set_bytes(
-                "PATH",
-                Type::ExpandString,
-                &super::to_winreg_bytes(wide("foo")),
-            )
+            .set_expand_hstring("PATH", &HSTRING::from("foo"))
             .unwrap();

         {

A further improvement would be to use the HSTRING or HStringBuilder types throughout instead of Vec<u16>. But I've not investigated what that would look like.

[ChrisDenton]

In case it helps, I created a branch that's rebased on master with my changes in case anyone wants to build on it: https://github.com/rust-lang/rustup/compare/master...ChrisDenton:rustup:winreg (note that it temporarily disables rustls because building their TLS is a right pain, feel free to drop that commit though).

[djc]

Since upstream is waiting for feedback on their API before they release, I think we should try to remove all unsafe code related to the registry in this PR, and give them feedback on where this is impossible with their current API.

(Please rebase this branch on master instead of appending merge commits.)

[InfyniteHeap]

It seems that the openssl-sys has some issues that prevent the commits from being passed.

[rami3l]

It seems that the openssl-sys has some issues that prevent the commits from being passed.

@InfyniteHeap You shouldn't update Cargo.lock for dependencies irrelevant to this PR. That's automatically handled by renovate on our side.

[InfyniteHeap]

It seems that the openssl-sys has some issues that prevent the commits from being passed.

@InfyniteHeap You shouldn't update Cargo.lock for dependencies irrelevant to this PR. That's automatically handled by renovate on our side.

It seems that I should only open VS Code to let it automatically update Cargo.lock instead of manually running cargo update...

[kennykerr]

I could implement Deref in place of as_wide and that would provide all of this directly.

[rami3l]

Current status: Waiting for a new upstream release to ship https://github.com/microsoft/windows-rs/issues/3148...

[kennykerr]

Here's the Deref implementation for HSTRING. https://github.com/microsoft/windows-rs/pull/3291