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

Removed Security.Permission reference #9055

Merged
merged 6 commits into from
Sep 18, 2023
Merged

Conversation

maridematte
Copy link
Contributor

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
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!

src/Shared/ExceptionHandling.cs Outdated Show resolved Hide resolved
src/Shared/ExceptionHandling.cs Outdated Show resolved Hide resolved
Copy link
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
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
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
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
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
Contributor 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.

src/Shared/ExceptionHandling.cs Outdated Show resolved Hide resolved
Copy link
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
8 checks passed
@maridematte maridematte deleted the 8962 branch January 10, 2024 13:38
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