Properly factor DllImports into Common\src\Interop

[stephentoub]

We now have a bunch of libraries with their own DllImports, whereas the expected approach is that all DllImports in product src should be included from src\Common\src\Interop, appropriately factored by platform and library, and then those source files included into the relevant assemblies that need them.

• Microsoft.Diagnostics.Tracing.EventSource.Redist
• System.Data.SqlClient
• System.Diagnostics.EventLog
• System.DirectoryServices
• System.DirectoryServices.AccountManagement
• System.DirectoryServices.Protocols
• System.Drawing.Common
• System.IO.Compression
• System.IO.Compression.Brotli
• System.IO.Ports
• System.Management
• System.Reflection.Metadata
• System.Runtime.Caching
• System.Security.Cryptography.Csp
• System.Security.Cryptography.Pkcs
• System.Security.Cryptography.X509Certificates

This all needs to be refactored and dedup'd.

https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md

[Marusyk]

Hello, I will try to prepare PR for this, if nobody is against it

[stephentoub]

@Marusyk, absolutely, thanks. I'd suggest starting with either EventLog, System.DirectoryServices., or System.Security..

[Marusyk]

Hello @stephentoub,

Could you please check if I am on the right way?
I start with EventLog

And one more question: should it be like one big PR or a few small ones?

[stephentoub]

Thanks, @Marusyk! That looks very good. Please go ahead and create a PR for it so it can be properly reviewed and merged.

should it be like one big PR or a few small ones?

One per library (e.g. EventLog) or family of libraries (e.g. System.DirectoryServices.*) would be good.

[StanislavUshakov]

Can I take System.DirectoryServices?

I have a couple of questions. As I can see there are a lot of [DllImport] for functions that are currently presented in src/Common/Interop, e.g. LoadLibrary, GetProcAddress from "kernel32.dll" are duplicated. Also there is an internal class LoadLibrarySafeHandle which is a duplicate of Common\CoreLib\Microsoft\Win32\SafeHandles\SafeLibraryHandle.cs
So our steps are:

1. Remove duplicates from System.DirectoryServices and use methods and classes from src/Common.
2. Add non-existing methods to src/Common and remove them from System.DirectoryServices
3. If current DllImport method from src/Common/Interop doesn't have SetLastError = true and System.DirectoryServices needs it, we have to modify Interop\Windows\kernel32\Interop.GetProcAddress.cs (because it is used to throw an exception e.g. in GetProcAddress with GetLastWin32Error)

If I am right with these assumptions, could you please take a look at Refactored LoadLibrary, FreeLibrary, GetProcAddress
using CommonInterop = Interop is a temporary fix while we have Interop namespace in the project. I will remove it later.

[StanislavUshakov]

And another question: what are the differences between Common/src/Interop/Windows and Common/src/CoreLib/Interop/Windows?
Some methods are duplicated: Interop.FreeLibrary in Common/src/Interop/Windows and Interop.FreeLibrary in Common/src/CoreLib/Interop/Windows Which one should we use: from Common/src/Interop/Windows?
And what if there is no method in Common/src/Interop/Windows, but it exists in Common/src/CoreLib/Interop/Windows (e.g. CloseHandle) Can we reference this file?

[danmoseley]

Common/src/CoreLib/Interop/Windows are mirrored from src/System.Private.Corelib/shared/Interop/Windows in the CoreCLR repo. Files there are in CoreCLR presumably because some code in System.Private.Corelib needs them. We mirror all of src/System.Private.Corelib/shared from the CoreCLR repo so that any code in CoreFX can reuse it if it needs to. This falls in this category -- if there is duplication, then generally we would want to keep the corelib copy, and change CoreFX code to use that one, and delete the CoreFX copy. You can see examples of using files out of CoreFX's ``Common/src/CoreLib` in various project files in CoreFX.

[frbncis]

Hi! Can I take a crack at System.DirectoryServices.Protocols? There's a lot of them (~65), but it looks like it's mostly elbow grease.

[danmoseley]

@frbncis sure - but we should take care, as our test coverage of that is mostly manual IIRC.

[satano]

Hello there. Can I take System.Security.*?

Also it would be nice to update the checkboxes in the issue to mark what was already done.

[satano]

Ping @danmosemsft, @stephentoub

[danmoseley]

@satano thanks for the offer, please do. Are checkboxes correct now?

[satano]

I did not look into the sources, but according to the merged PR names, the checkboxes are correct. So I will look into System.Security.Cryptography.X509Certificates.

[danmoseley]

OK!

[hareshambaliya]

@stephentoub I would like to contribute to this issue. Will you please guide me about where to start? What is pending?