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

Refactor interop usage in DirectoryServices #51325

Merged
merged 4 commits into from
Apr 19, 2021
Merged

Conversation

mthalman
Copy link
Member

This is a partial implementation to more closely conform to the interop guidelines.

The goals here were the following:

  • Clean up the ExternDlls class to only contain strings for those assemblies that are used by and unique to System.DirectoryServices and System.DirectoryServices.AccountManagement.
  • Remove hardcoded library names in [DllImport].
  • Use the common Interop.Libraries class when possible.

@mthalman
Copy link
Member Author

FYI: The motivation for these changes is to remove cruft and provide a clearer picture of what dependencies libraries have. This is to support the work that is going on for dotnet/core#5651.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work cleaning this up!

fyi - @buyaa-n @joperezr @krwq

public static extern unsafe int ADsGetLastError(out int error, char* errorBuffer, int errorBufferLength, char* nameBuffer, int nameBufferLength);

[DllImport(ExternDll.Activeds, CharSet = CharSet.Unicode)]
[DllImport(global::Interop.Libraries.Activeds, CharSet = CharSet.Unicode)]
Copy link
Member

Choose a reason for hiding this comment

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

Why are all these global::s necessary? Is there some kind of naming conflict this is working around?

Copy link
Member

Choose a reason for hiding this comment

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

Is there some kind of naming conflict this is working around?

Yes, there is an Interop namespace in this project.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, the System.DirectoryServices.Interop namespace. Presumably once we clean up the rest of the interop we can remove the "global::".

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks for the clean up.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking care of this @mthalman!

@mthalman mthalman merged commit 9f10999 into main Apr 19, 2021
@mthalman mthalman deleted the dev/mthalman/interop branch April 19, 2021 18:53
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants