-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 System.Security.Permissions from MSBuild #9212
Conversation
@@ -7,57 +7,52 @@ | |||
<SourceBuild RepoName="source-build-reference-packages" ManagedOnly="true" /> | |||
</Dependency> | |||
<!-- Necessary for source-build. This allows the live version of the package to be used by source-build. --> | |||
<Dependency Name="System.CodeDom" Version="7.0.0"> | |||
<Dependency Name="System.CodeDom" Version="8.0.0-preview.7.23375.6"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What implications does this have on coherency? I know source-build will override with "whatever is current" but our real packages would have a stale reference (to RC2) at GA, right? So we'd ideally need to wedge ourselves between runtime and sdk, where we're not currently? That sounds unfortunate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really just doing this as a POC. As we noticed MSBuild/SDK was actually shipping the 7.0 binares in the SDK up until RC when they started updating the transitive references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem busted that MSBuild packages released in 8.0 are going to reference 7.0... Not sure I agree with that layering.
@@ -26,21 +26,21 @@ | |||
</PropertyGroup> | |||
<!-- Production Dependencies --> | |||
<PropertyGroup> | |||
<SystemCollectionsImmutableVersion>7.0.0</SystemCollectionsImmutableVersion> | |||
<SystemConfigurationConfigurationManagerVersion>7.0.0</SystemConfigurationConfigurationManagerVersion> | |||
<SystemCollectionsImmutableVersion>8.0.0-preview.7.23375.6</SystemCollectionsImmutableVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SCI version in Visual Studio is controlled by the Roslyn team and it generally takes only stable versions, with nontrivial time to update. We have to match the central VS one by policy. So we can make this move only for our net8.0 target right now.
See also #9055 |
Is it obsolete in .NET Framework, too? I'd imagine we wouldn't touch that, in which case perhaps we should keep the reference but only if we're building for framework? |
Actually looks like #9055 already takes care of this. Closing in favor. |
We don't need any reference on Framework since the package doesn't define anything there. We'd only need the package on framework if some other library built against netstandard2.0 and needed it. |
Fixes #9213
Context
System.Security.Permissions is obsolete. Referencing the package brings in this obsolete library and a number of other libraries are not needed (System.Windows.Extensions).
Changes Made
Remove reference to System.Security.Permissions since all the source that uses it is under ifdef that's only defined for .NETFramework.
To fully remove it I needed to also update to 8.0 packages (since 7.0 ConfigurationManager still referenced SSP in 7.0). If we wanted to fix this without the update we could hack a direct reference to SSP with ExcludeAssets=all.
Testing
Build 🤞
Notes
We may not want to take this yet because @rainersigwald tells me VS needs MSBuild to stay on 7.0 until after GA. Can't say I fully understand that but opening up this PR to prove the dependency can be removed.