Skip to content

Clean-up Microsoft.Win32.Registry and remove the non-Windows code. #77996

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.

Sign up for GitHub

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

Jump to bottom
Merged
ViktorHofer merged 6 commits into from
Nov 22, 2022
Merged

Clean-up Microsoft.Win32.Registry and remove the non-Windows code. #77996

ViktorHofer merged 6 commits into from
Nov 22, 2022

Conversation

@teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Nov 8, 2022

In a similar vein with other Windows-exclusive libraries, this PR makes the sources of Microsoft.Win32.Registry compile only on Windows, removes the non-Windows files (they just threw PNSE), merges the .Windows source files and cleans-up some stuff.

Fixes #77974

@ghost ghost added area-Microsoft.Win32 community-contribution Indicates that the PR has been added by a community member labels Nov 8, 2022
@ghost
Copy link

ghost commented Nov 8, 2022

Tagging subscribers to this area: @dotnet/area-microsoft-win32
See info in area-owners.md if you want to be subscribed.

Issue Details

In a similar vein with other Windows-exclusive libraries, this PR makes the sources of Microsoft.Win32.Registry compile only on Windows, removes the non-Windows files (they just threw PNSE), merges the .Windows source files and cleans-up some stuff.

Fixes #77974

Author: teo-tsirpanis
Assignees: -
Labels:

area-Microsoft.Win32
Milestone: -

@stephentoub
Copy link
Member

stephentoub commented Nov 8, 2022

@marek-safar, how close are you to merging #72667? This is going to conflict.

@marek-safar
Copy link
Contributor

marek-safar commented Nov 8, 2022

how close are you to merging #72667? This is going to conflict.

Want to merge is ASAP (likely today)

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Nov 8, 2022

Conflicts are resolved.

@build-analysis build-analysis bot mentioned this pull request Nov 9, 2022
Closed
@carlossanlop
Copy link
Contributor

carlossanlop commented Nov 9, 2022

Are any of the CI failures concerning, @teo-tsirpanis ?

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Nov 9, 2022

It fails with CSC(0,0): error CS2001: (NETCORE_ENGINEERING_TELEMETRY=Build) Source file 'D:\a\_work\1\s\src\libraries\System.Private.CoreLib\src\Microsoft\Win32\SafeHandles\SafeRegistryHandle.Windows.cs' could not be found but I removed it from the corelib project.

@marek-safar
Copy link
Contributor

marek-safar commented Nov 10, 2022

The error comes from this project

runtime/src/libraries/System.Diagnostics.PerformanceCounter/src/System.Diagnostics.PerformanceCounter.csproj

Line 61 in db21365

<Compile Include="$(CoreLibSharedDir)Microsoft\Win32\SafeHandles\SafeRegistryHandle.Windows.cs"

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Nov 10, 2022

Fixed.

@teo-tsirpanis
Copy link
Contributor Author

teo-tsirpanis commented Nov 21, 2022

Can we merge it? It will conflict with #78558.

@teo-tsirpanis teo-tsirpanis mentioned this pull request Nov 21, 2022
Closed
@teo-tsirpanis teo-tsirpanis reopened this Nov 22, 2022
ViktorHofer
ViktorHofer reviewed Nov 22, 2022
View reviewed changes
src/libraries/Common/src/Interop/Windows/Advapi32/Interop.RegEnumKeyEx.cs
SafeRegistryHandle hKey,
int dwIndex,
[Out] char[] lpName,
ref char lpName,
Copy link
Member

@ViktorHofer ViktorHofer Nov 22, 2022

Choose a reason for hiding this comment

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

For my own education, why did you change this from a char array to a ref char?

Copy link
Contributor Author

@teo-tsirpanis teo-tsirpanis Nov 22, 2022

Choose a reason for hiding this comment

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

Because the name is in a stackalloced span (could make it a char*, don't remember why I didn't).

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 22, 2022

Apparently, two tests are failing deterministically. Let me retry them once again.

@build-analysis build-analysis bot mentioned this pull request Nov 22, 2022
Closed
@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 22, 2022

Test failures are due to a known issue: #78718

@ViktorHofer ViktorHofer merged commit 74c9e4a into dotnet:main Nov 22, 2022
@teo-tsirpanis teo-tsirpanis deleted the registry-cleanup branch November 22, 2022 20:24
@Poppyto Poppyto mentioned this pull request Nov 22, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@ViktorHofer ViktorHofer ViktorHofer approved these changes

Assignees

No one assigned

Labels

area-Microsoft.Win32 community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Simplify Microsoft.Win32.Registry sources split

5 participants

@teo-tsirpanis @stephentoub @marek-safar @carlossanlop @ViktorHofer