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 S.S.Permissions reference from S.DirectoryServices #82453

Merged
merged 15 commits into from
Mar 16, 2023

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Feb 21, 2023

Contributes to #64592.

The approach is mostly based on ericstj@3426ddd which is described at #64592 (comment). Basically, there are 5 types in S.DS that depend on types from S.S.P and this PR moves those to S.S.P so that the unwanted dependency from S.DS to S.S.P is removed. Those 5 types are either obsolete (CAS-related) or used only in parameters in those obsolete methods so they would not normally be referenced \ used.

This is considered a breaking change and may require in rare cases an extra reference to S.S.P (if any of the 5 types are referenced).

The System.DirectoryServices.nuspec goes from:


    <dependencies>
      <group targetFramework="net6.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net7.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net8.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
        <dependency id="System.IO.FileSystem.AccessControl" version="5.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.AccessControl" version="6.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.Principal.Windows" version="5.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>

to

    <dependencies>
      <group targetFramework="net6.0" />
      <group targetFramework="net7.0" />
      <group targetFramework="net8.0" />
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.IO.FileSystem.AccessControl" version="5.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.AccessControl" version="6.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.Principal.Windows" version="5.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>

See also #82259 which removes the S.S.P dependency from System.Configuration.ConfigurationManager.

Details:

  • The S.DS project does not have a netfx build; netfx has its own copy of the assembly.
    • The 5 types are forwarded to S.S.P for netcore\netstandard. The netfx version is not changed of course and the 5 types continue to live in S.DS.
  • The S.S.P project has a netfx build (along with netcore and netstandard)
    • The 5 types now live here for netcore\netstandard but forward to S.DS for netfx.

@steveharter steveharter added this to the 8.0.0 milestone Feb 21, 2023
@steveharter steveharter self-assigned this Feb 21, 2023
@steveharter steveharter added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Feb 21, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Feb 21, 2023
@ghost
Copy link

ghost commented Feb 21, 2023

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@ghost
Copy link

ghost commented Feb 23, 2023

Tagging subscribers to this area: @dotnet/area-system-directoryservices, @jay98014
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #64592.

The approach is mostly based on ericstj@3426ddd which is described at #64592 (comment). Basically, the 5 types in S.DS that depend on types from S.S.P are being moved to S.S.P in order to remove the unwanted dependency from S.DS to S.S.P. Those 5 types are either obsolete or used only in parameters in obsolete CAS-related methods.

This will still be considered a breaking change, and may require in rare cases (if the obsolete types are used) an extra reference to S.S.P.

The System.DirectoryServices.nuspec goes from:


    <dependencies>
      <group targetFramework="net6.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net7.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework="net8.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
      </group>
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.Security.Permissions" version="8.0.0-dev" exclude="Build,Analyzers" />
        <dependency id="System.IO.FileSystem.AccessControl" version="5.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.AccessControl" version="6.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.Principal.Windows" version="5.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>

to

    <dependencies>
      <group targetFramework="net6.0" />
      <group targetFramework="net7.0" />
      <group targetFramework="net8.0" />
      <group targetFramework=".NETStandard2.0">
        <dependency id="System.IO.FileSystem.AccessControl" version="5.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.AccessControl" version="6.0.0" exclude="Build,Analyzers" />
        <dependency id="System.Security.Principal.Windows" version="5.0.0" exclude="Build,Analyzers" />
      </group>
    </dependencies>

See also #82259 which removes the S.S.P dependency from S.Configuration.ConfigurationManager.

Author: steveharter
Assignees: steveharter
Labels:

area-System.DirectoryServices, breaking-change, needs-breaking-change-doc-created

Milestone: 8.0.0

@steveharter steveharter requested a review from ericstj March 8, 2023 21:01
<?xml version="1.0" encoding="utf-8"?>
<!-- https://learn.microsoft.com/en-us/dotnet/fundamentals/package-validation/diagnostic-ids -->
<Suppressions xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema">
<Suppression>
Copy link
Member

Choose a reason for hiding this comment

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

These errors are all saying that you are missing the type-forwards from the implementation assembly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The forwards were added; the "baseline" suppressions are still necessary.

Copy link
Member

Choose a reason for hiding this comment

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

They should not be necessary. API Compat should not raise the error if the type is forwarded - it should see that forward and compare the forwarded definition.

Copy link
Member

Choose a reason for hiding this comment

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

@ViktorHofer looks like something is going wrong with APICompat following type forwards. I pulled this branch and reproduced it. I can inspect the assemblies and I see that the new S.DS has a type forward for these types in S.S.P and the types exist there.

@steveharter please file an issue for this and merge with the suppression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM though we need to find out why APICompat doesn't follow the forward. That can be handled separately.

@steveharter
Copy link
Member Author

steveharter commented Mar 16, 2023

Logged #83551 for CI failures:

System.Runtime.Serialization.Xml.XsdDataContractExporterTests.ExporterApiTests.Export(testname: "Exp2", export: Action`1 { Method = Void <Export_MemberData>b__7_2(System.Runtime.Serialization.XsdDataContractExporter), Target = <>c { } }, schemaCheck: Action`2 { Method = Void <Export_MemberData>b__3(System.String, System.Xml.Schema.XmlSchemaSet), Target = <>c__DisplayClass7_0 { autoImportKVP = False } })
DataContractSerializerTests.DCS_MemoryStream_Serialize_UsesBuiltInAdapter
DataContractSerializerTests.DCS_FileStreamSurrogate

Other failures appear related to #83482.

@steveharter
Copy link
Member Author

Breaking change issue: dotnet/docs#36724

@steveharter steveharter removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DirectoryServices breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants