Skip to content
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

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

Merged
merged 6 commits into from Nov 22, 2022
Merged

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

merged 6 commits into from Nov 22, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

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 the community-contribution Indicates that the PR has been added by a community member label 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

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

@marek-safar
Copy link
Contributor

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

Conflicts are resolved.

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

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

@teo-tsirpanis
Copy link
Contributor Author

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

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

Fixed.

@teo-tsirpanis
Copy link
Contributor Author

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

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

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

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

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
@dotnet dotnet 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