Replace `winreg` with `windows-registry` #3896

InfyniteHeap · 2024-06-20T14:07:48Z

Closes #3779.

APIs provided by winreg crate is not safe, simple and abstract enough. Besides that, it probably also contains protential bugs. Because of that, it is better to replace it with windows-registry crate, which is officially published and maintained by Microsoft.

src/cli/self_update/windows.rs

src/cli/self_update/test.rs

djc

I haven't done a complete review yet, because I think there's a bunch of stuff left to clean up. The core change looks great though, thanks for working on this!

Cargo.toml

src/cli/self_update/test.rs

src/cli/self_update/windows.rs

djc

Overall a really nice improvement!

src/cli/self_update/windows.rs

tests/suite/cli_paths.rs

djc · 2024-06-22T16:49:01Z

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 · 2024-06-22T23:42:39Z

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 · 2024-06-22T23:47:27Z

This PR will keep closed until windows-registry updates.

rami3l · 2024-06-29T01:07:18Z

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

InfyniteHeap · 2024-06-29T04:27:04Z

@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! :)

src/cli/self_update/windows.rs

rami3l · 2024-09-22T00:13:25Z

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 · 2024-09-22T01:05:17Z

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

src/cli/self_update/windows.rs

rami3l

This PR is getting even clearer than the previous iteration, modulo some eventual rebases (esp. after @kennykerr's possible API adjustments). Good job!

PS: As a favor, please consider listing @ChrisDenton as a co-author of some (or all) of your commits when applicable :)

djc

This looks good to me, thanks for all the work!

kennykerr · 2024-09-22T14:41:26Z

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

rami3l · 2024-09-22T15:56:27Z

Current status: Waiting for a new upstream release to ship microsoft/windows-rs#3148...

kennykerr · 2024-09-23T15:58:52Z

Here's the Deref implementation for HSTRING. microsoft/windows-rs#3291

djc

@kennykerr what's the rationale for using all-uppercase names for stuff like HSTRING and HRESULT? Are you trying to match the upstream names exactly? It looks rather unidiomatic.

src/cli/self_update/windows.rs

kennykerr · 2024-09-24T12:54:10Z

what's the rationale for using all-uppercase names for stuff like HSTRING and HRESULT? Are you trying to match the upstream names exactly? It looks rather unidiomatic.

There are countless identifiers in the Windows API. It's just not very practical to come up with different names - it has been attempted. It also makes it a lot harder to cross-reference documentation and source code for equivalent types.

InfyniteHeap · 2024-09-24T17:30:19Z

What about these changes?

djc

I think this is looking great. @InfyniteHeap and @kennykerr thanks for all the work! Would be nice to get that windows-registry release out so that we can move forward with this.

kennykerr · 2024-09-24T22:02:03Z

Coming up: microsoft/windows-rs#3293

kennykerr · 2024-09-25T11:51:27Z

Version 0.3.0 of the windows-registry crate has been published. https://github.com/microsoft/windows-rs/releases/tag/0.60.0

djc · 2024-09-25T13:23:45Z

Can you squash all of your changes into a single commit?

InfyniteHeap · 2024-09-25T13:34:34Z

Can you squash all of your changes into a single commit?

I've done this.

rami3l

LGTM, and thanks for your work @InfyniteHeap!

PS: You may proceed with cleanup PRs such as the one discussed in #3896 (comment) shortly after.

InfyniteHeap commented Jun 20, 2024

View reviewed changes

src/cli/self_update/windows.rs Show resolved Hide resolved

rami3l reviewed Jun 20, 2024

View reviewed changes

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

rami3l reviewed Jun 20, 2024

View reviewed changes

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

rami3l reviewed Jun 20, 2024

View reviewed changes

src/cli/self_update/test.rs Outdated Show resolved Hide resolved

rami3l changed the title ~~Change dependency to windows-registry~~ Replace winreg with windows-registry Jun 20, 2024

InfyniteHeap marked this pull request as ready for review June 20, 2024 16:04

djc reviewed Jun 20, 2024

View reviewed changes

InfyniteHeap closed this Jun 21, 2024

InfyniteHeap reopened this Jun 21, 2024

InfyniteHeap closed this Jun 21, 2024

InfyniteHeap deleted the change-dependency branch June 21, 2024 03:30

InfyniteHeap restored the change-dependency branch June 21, 2024 03:31

InfyniteHeap reopened this Jun 21, 2024

InfyniteHeap closed this Jun 21, 2024

InfyniteHeap reopened this Jun 21, 2024

InfyniteHeap requested review from djc and rami3l June 21, 2024 06:32

djc reviewed Jun 21, 2024

View reviewed changes

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

InfyniteHeap closed this Jun 21, 2024

InfyniteHeap reopened this Jun 21, 2024

rami3l reviewed Jun 21, 2024

View reviewed changes

tests/suite/cli_paths.rs Outdated Show resolved Hide resolved

rami3l mentioned this pull request Jun 21, 2024

Should windows_registry::Key::(get|set)_string use OsStr instead of str? microsoft/windows-rs#3119

Closed

InfyniteHeap closed this Jun 22, 2024

InfyniteHeap reopened this Jul 5, 2024

rami3l reviewed Jul 5, 2024

View reviewed changes

src/cli/self_update/windows.rs Show resolved Hide resolved

InfyniteHeap commented Sep 22, 2024

View reviewed changes

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

InfyniteHeap commented Sep 22, 2024

View reviewed changes

src/cli/self_update/windows.rs Show resolved Hide resolved

InfyniteHeap commented Sep 22, 2024

View reviewed changes

src/cli/self_update/windows.rs Outdated Show resolved Hide resolved

rami3l reviewed Sep 22, 2024

View reviewed changes

djc approved these changes Sep 22, 2024

View reviewed changes

kennykerr mentioned this pull request Sep 23, 2024

Add Deref implementation for HSTRING microsoft/windows-rs#3291

Merged

djc reviewed Sep 24, 2024

View reviewed changes

djc approved these changes Sep 24, 2024

View reviewed changes

kennykerr mentioned this pull request Sep 24, 2024

Release 0.60.0 microsoft/windows-rs#3293

Merged

InfyniteHeap marked this pull request as ready for review September 25, 2024 05:05

replace winreg dependency

2f0399a

kennykerr mentioned this pull request Sep 25, 2024

A TryFrom<&str> impl for GUID microsoft/windows-rs#3174

Closed

rami3l approved these changes Sep 26, 2024

View reviewed changes

djc added this pull request to the merge queue Sep 26, 2024

Merged via the queue into rust-lang:master with commit bce3ed6 Sep 26, 2024

InfyniteHeap deleted the change-dependency branch September 26, 2024 07:47

InfyniteHeap mentioned this pull request Sep 26, 2024

Remove unnecessary methods #4034

Merged

rami3l mentioned this pull request Oct 6, 2024

dist: bump rustup version to 1.28.0 #4041

Closed

3 tasks

Replace winreg with windows-registry #3896

Replace winreg with windows-registry #3896

Uh oh!

Conversation

InfyniteHeap commented Jun 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc commented Jun 22, 2024

Uh oh!

InfyniteHeap commented Jun 22, 2024

Uh oh!

InfyniteHeap commented Jun 22, 2024

Uh oh!

rami3l commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

InfyniteHeap commented Jun 29, 2024

Uh oh!

Uh oh!

rami3l commented Sep 22, 2024

Uh oh!

InfyniteHeap commented Sep 22, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rami3l left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

kennykerr commented Sep 22, 2024

Uh oh!

rami3l commented Sep 22, 2024

Uh oh!

kennykerr commented Sep 23, 2024

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kennykerr commented Sep 24, 2024

Uh oh!

InfyniteHeap commented Sep 24, 2024

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

kennykerr commented Sep 24, 2024

Uh oh!

Replace `winreg` with `windows-registry` #3896

Replace `winreg` with `windows-registry` #3896

InfyniteHeap commented Jun 20, 2024 •

edited

Loading

djc left a comment •

edited

Loading

rami3l commented Jun 29, 2024 •

edited

Loading

rami3l left a comment •

edited

Loading

rami3l left a comment •

edited

Loading