Skip to content

Removed Security.Permission reference#9055

Merged
maridematte merged 6 commits intodotnet:mainfrom
maridematte:8962
Sep 18, 2023
Merged

Removed Security.Permission reference#9055
maridematte merged 6 commits intodotnet:mainfrom
maridematte:8962

Conversation

@maridematte
Copy link
Copy Markdown
Member

Fixes #8962

Changes Made

Moved some refences for exception handling so System.Security.Permissions is not referenced in 8.0.
Removed System.Security.Permissions from the package list in files.

Testing

N/A

Copy link
Copy Markdown
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

Copy link
Copy Markdown
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

This looks good, however I believe that we'll unfortunately need to hunt and kill all the transitive dependencies as well :-/

You can use the project.assets.json files to find them and untagle the transitivity chain:

> \.build.cmd
...
> cd artifacts\obj
> findstr /spin /c:"System.Security.Permissions" project.assets.json

One example is System.Configuration.ConfigurationManager

image

@akoeplinger
Copy link
Copy Markdown
Member

akoeplinger commented Sep 4, 2023

This would've helped with dotnet/source-build#3571 / #9158.

@maridematte do you plan on updating the PR? I can help address the PR feedback if you don't have time.

@JanKrivanek
Copy link
Copy Markdown
Member

@akoeplinger Unfortunately the S.C.ConfigurationManager cannot be easily removed without substantial refactoring of existing code (we depend on it on core as well).
One way out is upgrading the S.S.Permissions to 8.0 as the System.Drawing dependency is no longer pulled.

Is this blocking you anywhere? The bug and PR you mentioned are both closed - so I'm trying to see what's the priority of this one

@akoeplinger
Copy link
Copy Markdown
Member

akoeplinger commented Sep 5, 2023

@JanKrivanek the problem in dotnet/source-build#3571 was that a type from S.S.Permissions was loaded, this PR would've prevented that one.

As for S.C.ConfigurationManager the 8.0 version of the nuget package no longer depends on S.S.Permissions, so we'd need to upgrade to that.

This is not blocking for me, I just noticed this while looking at related PRs. But I think #8962 looks quite important if the msbuild package now can't be used on Mac/Linux.

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Sep 9, 2023

IMO this PR is goodness as it is. If consumers want, they can also reference the latest ConfigurationManager package from 8.0 to lift up the dependency that MSBuild has on it and eliminate all references to SSP. @rainersigwald let me know the SDK will do this so this should remove the file from the final layout.

@maridematte
Copy link
Copy Markdown
Member Author

In an offline discussion we decided to not upgrade S.C.ConfigurationManager to the latest version, as that package should be automatically upgraded by the SDK.

Copy link
Copy Markdown
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

@maridematte maridematte merged commit 85c9254 into dotnet:main Sep 18, 2023
@maridematte maridematte deleted the 8962 branch January 10, 2024 13:38
JanProvaznik added a commit that referenced this pull request Sep 5, 2025
### Context

Since #6148 we produce only reference assemblies for .NET Standard 2.0,
which means that several of the our NuGet packages' dependencies when
targeting .NET Standard 2.0 are unused.

### Changes Made

The project files for `Microsoft.Build.Framework`, `Utilities.Core` and
`Tasks` were updated to apply `PrivateAssets="all"` to all package
references that are not exposed in the package's public API, when
targeting .NET Standard 2.0.

### Testing

I ran `dotnet pack` on these projects and validated manually that on
.NET Standard 2.0, `Microsoft.Build.Framework` has zero dependencies,
`Utilities.Core` depends only on `Framework`, and `Tasks` depends only
on the previous two.

Because Roslyn [keeps some internal APIs in reference
assemblies](https://github.com/dotnet/roslyn/blob/main/docs/features/refout.md#definition-of-ref-assemblies),
these reference assemblies still reference some assemblies whose
respective package is not depended upon. I manually validated with ILSpy
that the types in these assemblies are used only by internal APIs.

### Notes

This is going to be a (minor) source-breaking change if a .NET Standard
2.0 project uses APIs from on one of the removed packages and
transitively depended on it. They will have to directly depend on them,
and it's not the first time we do a change like this (#9055).

I don't think that this is a binary-breaking change because the .NET
Standard 2.0 binaries are not being used at runtime.

---------

Co-authored-by: Jan Provazník <janprovaznik@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dependency on System.Drawing.Common (>= 7.0.0) blocks latest Microsoft.Build package

5 participants