search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.48k stars 4.76k forks source link

System.DirectoryServices libraries should conform to interop guidelines #51563

Open mthalman opened 3 years ago

mthalman commented 3 years ago

The following list of directories/files do not conform to the interop guidelines:

joperezr commented 3 years ago

@mthalman was this covered by your recent PR https://github.com/dotnet/runtime/pull/51325 ?

mthalman commented 3 years ago

No, that PR was only meant to move the needle closer to conforming with the guidelines, but not all the way. My original statement is still relevant and should be addressed:

Specifically, the file organization section states that all Interop.*.cs files should all be defined in the Common directory.

benjaminsampica commented 3 years ago

Hello - I'd love to pick this up if it's still available.

benjaminsampica commented 3 years ago

@buyaa-n I apologize but some personal things have come up and I don't think I'll get to this in a timely fashion.

krwq commented 3 years ago

no worries @benjaminsampica

iinuwa commented 3 years ago

I think I can tackle this. I have a few questions though:

joperezr commented 3 years ago

Hey @iinuwa thanks for volunteering! Yes, Pal stands for Platform Abstraction Layer, since most of those calls are then forwarded to the right native library (wldap32 on Windows and openLdap on Linux) so we wanted a middle layer that would expose a common layer which would then reroute the calls appropriately. Usually we move stuff under Common whenever it applies to more than one runtime library, and for now these only really apply to System.DirectoryServices.Protocols so at least at first I would keep them where they are.

The System.DirectoryServices.Protocols.csproj file adds all the Linux files under the $(TargetsUnix) == 'true' flag. That means all the .Linux.cs files should be renamed to .Unix.cs, right?

I believe so yes. Originally when we added non-Windows support for this library we started with Linux, so most files where Linux-specific. We then added support to OS X as well, at which point we only updated some of the files but as you have noted, not everything was renamed.

Since this is just a code reorganization and we're so close to the .NET 6 release, this should be pushed to 7.0.0, right? Is there a specific target branch I should use, or does it just go into main?

I think this would definitely is not a must of 6.0 release. There is currently no branch for work post-6.0 yet, but this doesn't mean that we couldn't also take this for 6.0, it just means it is not one of our current priorities. Right now we are mostly focusing on addressing all issues with milestone == 6.0, but feel free to send out a PR for this if you want to. If something unexpected happens and we can't take in this change for 6.0, we can always ask you to retarget the PR to whatever (future) branch that has all post 6.0 stuff.

Insomniak47 commented 3 years ago

@iinuwa are you still taking this or is this up in the air again? If so I don't mind picking it up

iinuwa commented 3 years ago

You can go ahead and take it. I got stuck with the Windows-only projects since I don't have a Windows machine to develop with.

Oct 3, 2021 09:52:29 Chris Nantau @.***>:

@iinuwa[https://github.com/iinuwa] are you still taking this or is this up in the air again? If so I don't mind picking it up

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub[https://github.com/dotnet/runtime/issues/51563#issuecomment-932967097], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AGTO4XWZT3HQ5HM3MUWHBETUFBU2ZANCNFSM43IL3LDQ]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACkAAAApCAYAAACoYAD2AAAABHNCSVQICAgIfAhkiAAAAB1JREFUWIXtwTEBAAAAwqD1T20Hb6AAAAAAAADgNBptAAEG9UMuAAAAAElFTkSuQmCC###24x24:true###][Tracking image][https://github.com/notifications/beacon/AGTO4XUQRWYQG4T2WP2ZZB3UFBU2ZA5CNFSM43IL3LD2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOG6N7FOI.gif]

danmoseley commented 2 years ago

@Insomniak47 are you still interested? it should be an easy one.

RosyMapleMoth commented 9 months ago

I'm interested in working on this if it is no longer under development by anyone

buyaa-n commented 9 months ago

I'm interested in working on this if it is no longer under development by anyone

I think it is fixed with Improve System.DirectoryServices interop guidelines conformance, @chrisdcmoore is there anything left that is not covered by your PR?

huoyaoyuan commented 3 months ago

Related: #106316

I believe that Marshal and HGlobal based interop should also be considered as legacy.