Add support for setting string values with the REG_EXPAND_SZ type when modifying the registry

[InfyniteHeap]

Suggestion

Coming from https://github.com/rust-lang/rustup/pull/3896/files#r1666173552. As what we discussed in this review, we should set string values with REG_EXPAND_SZ type when modifying PATH. So, we need a way to set the string type when calling set_string() and set_hstring().

Appendix: We also need a way to verify whether String or HSTRING has REG_SZ or REG_EXPAND_SZ. See https://github.com/rust-lang/rustup/blob/044083c9cc58b0ae23eeb1a390ca17ddc6f859d2/src/cli/self_update/windows.rs#L950.

[kennykerr]

Here you go: #3184

[kennykerr]

#3184 is now merged - can you please test the rustup port against github? I don't plan to publish another crate release until we're confident this is in good shape for the foreseeable future. @InfyniteHeap @rami3l

[rami3l]

I don't plan to publish another crate release until we're confident this is in good shape for the foreseeable future.

@kennykerr rust-lang/rustup#3896 looks great with the Cargo patch that points to the latest dev version of the lib! Thanks to you and @InfyniteHeap's efforts, the migration seems quite straightforward now.

[djc]

I'd like to check some more things before the release, we're left with some unsafe code in rustup that might be nice to migrate into this crate.

[kennykerr]

@rami3l - glad to hear things are going well!

@djc - sounds good - please open a new issue for any bug/feature requests.

[InfyniteHeap]

I'd like to check some more things before the release, we're left with some unsafe code in rustup that might be nice to migrate into this crate.

The reason why I add unsafe code is that, since I cannot manually construct a Value to compare to what I got from get_value(), I have to manually seperately convert the got Value into Type and &[u8] to fit the requirements. See my latest commit and this issue.

[kennykerr]

That issue isn't clear on what it's asking for. By the way, #3190 provides a few small improvements for convertibility.

[InfyniteHeap]

That issue isn't clear on what it's asking for. By the way, #3190 provides a few small improvements for convertibility.

My initial idea is that, I probably need a pair of generic-feasible traits that can make Value and other types be inter-converted, as what did in the previous rustup code.

The original effect I wanted to reach out looks like this (for illustration purposes only, because this code definitely cannot be compiled):

pub fn set_value<T: TryFrom<Value>>(&self, new: Option<&T>) -> Result<()> {
    self.set(new.map(|v| &Value::try_from(v).unwrap()))
}

fn set(&self, new: Option<&Value>) -> Result<()> {
    let sub_key = CURRENT_USER.create(self.sub_key)?;
    match new {
        Some(new) => Ok(sub_key.set_value(self.value_name, new)?),
        None => Ok(sub_key.remove_value(self.value_name)?),
    }
}

...But now I think it is no need indeed to require such this feature.

In any cases, this question is now resolved! xd

[erikdesjardins]

My initial idea is that, I probably need a pair of generic-feasible traits that can make Value and other types be inter-converted, as what did in the previous rustup code.

TryFrom<Value> and TryInto<Value> are a pair of generic traits that can convert to/from Value.

Your code example will work, just with TryInto instead of TryFrom:

pub fn set_value<T: TryInto<Value>>(&self, new: Option<T>) -> Result<()> {
    self.set(new.map(|v| &v.try_into().unwrap()))
}

or alternatively (less idiomatic):

pub fn set_value<T>(&self, new: Option<T>) -> Result<()> where Value: TryFrom<T> {
    self.set(new.map(|v| &Value::try_from(v).unwrap()))
}

[rami3l]

I'd like to check some more things before the release, we're left with some unsafe code in rustup that might be nice to migrate into this crate.

@djc Do you think so far it's safe for @kennykerr to cut a new release, given the current state of rust-lang/rustup#3896? Many thanks in advance!

[djc]

Sorry for the long delay, was on vacation but circling back to this now. Having some trouble cross-compiling the rustup code for Windows in the current state, so will see if I can figure this out.