Support building System.DirectoryServices ref assembly against net5.0

[jeffhandley]

For dotnet/designs#139, I need to be able to mark 2 types in System.DirectoryServices as [Obsolete] starting in .NET 5: DirectoryServicesPermission and DirectoryServicesPermissionAttribute.

At present, the src project has the following:

<TargetFrameworks>$(NetCoreAppCurrent)-Windows_NT;netstandard2.0;netcoreapp2.0-Windows_NT;_$(NetFrameworkCurrent)</TargetFrameworks>

And the ref assembly has simply:

<TargetFrameworks>netstandard2.0;_$(NetFrameworkCurrent)</TargetFrameworks>

I can conditionally add the attributes to the src project, but I cannot currently add the attributes to the ref assembly without also affecting netstandard2.0.

I attempted to add $(NetCoreAppCurrent) into the target frameworks for the ref assembly but that produced the following build errors that I was unable to overcome:

System.DirectoryServices.cs(61,73): error CS0115: 'ActiveDirectorySecurity.AccessRuleFactory(IdentityReference, int, bool, InheritanceFlags, PropagationFlags, AccessControlType, Guid, Guid)': no suitable method found to override [C:\Users\jeffhand\git\dotnet\runtime\src\libraries\System.DirectoryServices\ref\System.DirectoryServices.csproj]
System.DirectoryServices.cs(65,72): error CS0115: 'ActiveDirectorySecurity.AuditRuleFactory(IdentityReference, int, bool, InheritanceFlags, PropagationFlags, AuditFlags, Guid, Guid)': no suitable method found to override [C:\Users\jeffhand\git\dotnet\runtime\src\libraries\System.DirectoryServices\ref\System.DirectoryServices.csproj]
CSC : error CS0012: The type 'CLSCompliantAttribute' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [C:\Users\jeffhand\git\dotnet\runtime\src\libraries\System.DirectoryServices\ref\System.DirectoryServices.csproj]

The AccessRuleFactory AuditRuleFactory methods that are expressed as not being found seem to be available in netstandard2.0 as far as I could tell, and they're shown on the .NET API Catalog too. I can't figure out why they're not visible.

[ghost]

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

[safern]

I looked into this and basically the problem is that System.IO.FileSystem.AccessControl builds against netstandard2.0 and you're getting:

the type 'CLSCompliantAttribute' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. 

If I go and add NetCoreAppCurrent to System.IO.FileSystem.AccessControl and fix the build issues, then I can build System.DirectoryServices ref assembly for net5.0.

@ericstj is that desired?

[ericstj]

Technically you could fix this by adding a reference to netstandard.dll (facade) to DirectoryServices. That would let it unify the types. We don't currently build netstandard.dll early enough though, so @safern's suggestion is probably best if we want to do this. Another option would be to push down the types if we thought that would be cleaner.

Question: why would we ship a nuget package in .NET 5.0 that obsoletes types exposed from the same package when targeting .NET5.0, but not when targeting .NETStandard. This creates a bit of a loophole. Is there a problem obsoleting everywhere the package exposes API? I guess the spec is relying on new members added to ObsoleteAttribute which are not present on .NETStandard which means we cannot use the new attribute members to obsolete API in packages that expose it in .NETStandard.

[ViktorHofer]

Technically you could fix this by adding a reference to netstandard.dll (facade) to DirectoryServices. That would let it unify the types. We don't currently build netstandard.dll early enough though, so @safern's suggestion is probably best if we want to do this.

When #35606 goes in, the netstandard shim is built early enough and can be referenced. Another option would be to P2P the shim.

[jeffhandley]

@ericstj

Question: why would we ship a nuget package in .NET 5.0 that obsoletes types exposed from the same package when targeting .NET5.0, but not when targeting .NETStandard. This creates a bit of a loophole. Is there a problem obsoleting everywhere the package exposes API? I guess the spec is relying on new members added to ObsoleteAttribute which are not present on .NETStandard which means we cannot use the new attribute members to obsolete API in packages that expose it in .NETStandard.

Correct that we would not be able to use the new properties. Additionally, features like CER and CAS are functional on .NET Framework, whereas they are not functional on .NET 5--the spirit of the obsoletions was to annotate features that are no longer functional in .NET 5, so I was only opting to apply the attributes where we know the feature isn't available. https://github.com/dotnet/designs/blob/master/accepted/2020/better-obsoletion/obsoletions-in-net5.md#other-considerations

[ericstj]

I have a hard time making myself comfortable with the idea that in a nuget package that supports multiple frameworks we make it more complicated to obsolete the types exposed by that package and we expend more effort in supporting those types in down level platforms.

[jeffhandley]

@ericstj If we do decide to apply the obsoletions to netstandard, would we still need to conditionally compile the ref assembly such that it uses the .NET 5.0 ObsoleteAttribute with DiagnosticId/UrlFormat while using either the traditional ObsoleteAttribute downlevel or using an internally-defined attribute with the properties?

Meaning... would we still need net5.0-conditional compilation?

[safern]

Meaning... would we still need net5.0-conditional compilation?

Nope... the conditional compilation is only needed to distribute different compile assets for different frameworks.

EDIT: actually yes if we use different ObsoleAttribute form

[ViktorHofer]

@jeffhandley what should we do with this issue? Asking as I don't see anything actionable on our (infrastructure) side.

[jeffhandley]

@ViktorHofer I think we have the following options:

1. Conditionally apply the [Obsolete] attribute only when targeting net5.0+ (my original proposal)
2. Apply the [Obsolete] attribute always, but when targeting net5.0+, use the new DiagnosticId and UrlFormat properties on the attribute
3. Create an internal System.ObsoleteAttribute within the ref assembly that has the DiagnosticId and UrlFormat properties on it, and use that attribute instead

Options 1 and 2 would still require building against net5.0, while option 3 would not. For option 3, if that is the recommended approach, I will need to determine which parts of our tooling do and do not support custom/internal ObsoleteAttribute types. Is option 3 what you all recommend, @ericstj, @ViktorHofer, and @safern?

[safern]

If our tooling support option 3 I think that would be the best option from my perspective. However, if we can't do that, I think doing 2 is also fine.

If we're going with 2 or 3, should we also do this for other OOB packages that had their APIs marked as Obsolete?

[jeffhandley]

I was finally able to confirm that this can work. Defining an internal ObsoleteAttribute in a netstandard2.0 library that includes DiagnosticId and UrlFormat, and consuming that API in another assembly will be respected by the tooling (command-line build as well as VS build), and it will show the warning/error respecting the diagnostic id and url format.

I'll move forward with that approach in System.DirectoryServices for #39413.

Thanks for the guidance!