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.

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

Remove duplicated interop structures in PlatformDetection and ones from src/common instead #27462

Closed
ViktorHofer opened this issue Sep 24, 2018 · 12 comments
Labels
area-Meta help wanted [up-for-grabs] Good issue for external contributors test-enhancement Improvements of test source code
Milestone

Comments

@ViktorHofer
Copy link
Member

A lot of interop structures are copied. We should instead just include these structure from the common location: dotnet/corefx#32433 (comment)

@ViktorHofer ViktorHofer changed the title Cleanup duplicated interop calls in PlatformDetection to use ones from src/common instead Remove duplicated interop structures in PlatformDetection and ones from src/common instead Sep 24, 2018
@artkpv
Copy link
Contributor

artkpv commented Sep 27, 2018

Hello, I'm removing these duplicates. Did for RTL_OSVERSIONINFOEX and stopped at OpenProcessToken. Didn't find for GetCurrentApplicationUserModelId and CloseHandle. Probably to add into src\Common?
Some progress at dotnet/corefx@master...artkpv:duplicates-32438

@ViktorHofer
Copy link
Member Author

Right, if we use the same interop code in multiple places that isn't yet in the shared Common folder, we should put it there.

@danmoseley
Copy link
Member

@artkpv I sent you a collaborator link. If you accept, I can assign this issue to you. Note: being a collaborator auto subscribes to the repo, you may want to adjust that after accepting.

@artkpv
Copy link
Contributor

artkpv commented Oct 6, 2018

Reporting the progress. Now I've stopped at getting SafeTokenHandle to use OpenProcessToken(..). The difficulty is that I don't understand why if I reference System.Diagnostics.Process lib with this class my CoreFx.Private.TestUtilities can not find it (error CS0246: The type or namespace name 'SafeTokenHandle' could not be found). It uses older lib in .\bin\ref\System.Diagnostics.Process\4.2.1.0\... So I'm reading (this and that) and experimenting on how to reference projects. Looks tangled so far :)

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Oct 6, 2018

If you push your changes I can take a look.

EDIT: I see they are already pushed. I'll take a look later.

@ViktorHofer
Copy link
Member Author

Hadn't had time to look into it. Maybe somebody else can help unblocking @artkpv.

cc @Anipik @danmosemsft)

@Anipik
Copy link
Contributor

Anipik commented Oct 7, 2018

@artkpv you are getting this error because SafeTokenHandle is an internal class . which OpenProcessToken you are trying to remove ?

@artkpv
Copy link
Contributor

artkpv commented Oct 8, 2018

@Anipik Yes, missed that. This is to reuse Common\src\Interop\Windows\advapi32\Interop.OpenProcessToken.cs in CoreFx.Private.TestUtilities. That's inconsistency, I image, as external method in Common uses class which is not there. So I guess solution would be to move SafeTokenHandle into Common\src\Microsoft\Win32\SafeHandles\ and use it in 3 libs (System.Diagnostics.Process, System.Security.AccessControl and the one).

Update. Seems to relate to #16030

@artkpv
Copy link
Contributor

artkpv commented Oct 12, 2018

@Anipik It is unfinished from #16030 times.

After looking at it more, the same is here and there. Probably should be ticket for this. Below is the list of files with the same class as I see.

  1. public "System.Security.Principal.Windows\src\Microsoft\Win32\SafeHandles\SafeAccessTokenHandle.cs"
    Doc: "Provides a safe handle to a Windows thread or process access token."
    Used in
    CoreFx.Private.TestUtilities (3)
    System.Diagnostics.PerformanceCounter (1)
    System.IO.Pipes.Tests (2)
    System.Net.Security (2)
    System.Security.Principal.Windows (41)
    System.Security.Principal.Windows.Tests (4)

  2. internal "System.Security.AccessControl/src/System/Security/SafeSecurityHandles.cs"
    In System.Security.AccessControl (14)

  3. internal "Common\src\Microsoft\Win32\SafeHandles\SafeTokenHandle.cs"
    In System.Diagnostics.Process (5)

  4. public partial calss SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid System.Diagnostics.Process\src\Microsoft\Win32\SafeHandles\SafeProcessHandle.Windows.cs
    Doc: "Provides a managed wrapper for a process handle."
    In System.Diagnostics.PerformanceCounter (12)
    System.Diagnostics.Process (65)
    System.Diagnostics.Process.Tests (7)

  5. internal "System.Diagnostics.Process\src\Microsoft\Win32\SafeHandles\SafeThreadHandle.cs"
    Only in System.Diagnostics.Process (19).

And 8 versions for OpenProcessToken (https://source.dot.net/#q=OpenProcessToken)

So where what should we use? :) Probably we can use only SafeProcessHandle and SafeAccessTokenHandle as they are public. I'm going to try to do this next.

@artkpv
Copy link
Contributor

artkpv commented Oct 29, 2018

@ViktorHofer I guess this issue can be closed. I'm going to make another issue for the above comment.

@ViktorHofer
Copy link
Member Author

Thanks a lot!

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta help wanted [up-for-grabs] Good issue for external contributors test-enhancement Improvements of test source code
Projects
None yet
Development

No branches or pull requests

5 participants