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

System.Windows.Extensions.dll included in Linux SDK output #9213

Closed
mthalman opened this issue Aug 29, 2023 · 10 comments
Closed

System.Windows.Extensions.dll included in Linux SDK output #9213

mthalman opened this issue Aug 29, 2023 · 10 comments
Labels
Priority:2 Work that is important, but not critical for the release triaged

Comments

@mthalman
Copy link
Member

mthalman commented Aug 29, 2023

The Linux version of the SDK is including the System.Windows.Extensions.dll file in the following paths:

  • ./sdk/x.y.z/FSharp/runtimes/win/lib/netx.y/System.Windows.Extensions.dll
  • ./sdk/x.y.z/runtimes/win/lib/netx.y/System.Windows.Extensions.dll

This is a regression as a result of the change in #9158. That change caused the live version of System.Security.Permissions to be used. That version has a dependency on System.Windows.Extensions, even when targeting Linux. So System.Windows.Extensions is being built and included in the output of the SDK. This is not a regression from #9158 because this change also exists in Microsoft-built SDK for Linux. The change from #9158 just made it visible in the source-build SDK diff tests.

There are other files existing in this runtimes/win path as well, which is a suspicious path when targeting Linux:

  • System.Diagnostics.EventLog.dll
  • System.Diagnostics.EventLog.Messages.dll
  • System.Security.Cryptography.Pkcs.dll
  • System.ServiceProcess.ServiceController.dll
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@mthalman mthalman changed the title Sysetm.Windows.Extensions.dll included in SDK output System.Windows.Extensions.dll included in SDK output Aug 29, 2023
@mthalman mthalman transferred this issue from dotnet/source-build Aug 29, 2023
@mthalman mthalman changed the title System.Windows.Extensions.dll included in SDK output System.Windows.Extensions.dll included in Linux SDK output Aug 29, 2023
@mthalman
Copy link
Member Author

@ghost
Copy link

ghost commented Aug 29, 2023

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

The Linux version of the SDK is including the System.Windows.Extensions.dll file in the following paths:

  • ./sdk/x.y.z/FSharp/runtimes/win/lib/netx.y/System.Windows.Extensions.dll
  • ./sdk/x.y.z/runtimes/win/lib/netx.y/System.Windows.Extensions.dll

This is a regression as a result of the change in #9158. That change caused the live version of System.Security.Permissions to be used. That version has a dependency on System.Windows.Extensions, even when targeting Linux. So System.Windows.Extensions is being built and included in the output of the SDK. This is not a regression from #9158 because this change also exists in Microsoft-built SDK for Linux. The change from #9158 just made it visible in the source-build SDK diff tests.

There are other files existing in this runtimes/win path as well, which is a suspicious path when targeting Linux:

  • System.Diagnostics.EventLog.dll
  • System.Diagnostics.EventLog.Messages.dll
  • System.Security.Cryptography.Pkcs.dll
  • System.ServiceProcess.ServiceController.dll
Author: mthalman
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@ViktorHofer
Copy link
Member

ViktorHofer commented Aug 29, 2023

cc @dotnet/area-system-security for the above System.Security.Permissions question.

One question is why there is no Windows-based condition check for this reference:

System.Security.Permissions doesn't target any RIDs. It provides RID agnostic TFMs: https://github.com/dotnet/runtime/blob/b9b51d3bbdd547a2542720c772d7e85fa6a82837/src/libraries/System.Security.Permissions/src/System.Security.Permissions.csproj#L3

If we would want to only include that reference for Windows, we would need to start targeting RIDs which has drawbacks (package size, build complexity, ...).

From what I can see, the S.W.E. reference is necessary because of XamlAccessLevel types being used in S.S.P: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Security.Permissions/src/System/Xaml/Permissions/XamlLoadPermission.cs#L19

As that type is already marked as obsolete on .NETCoreApp and was only brought back for WPF, it might be interesting to discuss if we can actually remove it as part of a breaking change.

@jkotas
Copy link
Member

jkotas commented Aug 29, 2023

The whole S.S.P is obsolete. I do not think it is worth doing anything with it. People should be deleting references to it as part of modernizing their projects.

There are other files existing in this runtimes/win path as well, which is a suspicious path when targeting Linux:

This is dotnet/sdk#16895

@ViktorHofer
Copy link
Member

The whole S.S.P is obsolete. I do not think it is worth doing anything with it. People should be deleting references to it as part of modernizing their projects.

We ourselves also reference S.S.P. Should we stop doing that as part of a bigger breaking change and delete S.S.P eventually?

@jkotas
Copy link
Member

jkotas commented Aug 30, 2023

S.S.P is part of .NET Framework compat. I think it needs to exist in this repo as long as we are building the .NET Framework compat live.

Cleaning it up from dependencies of assemblies that end up in .NET SDK would be nice. I expect that most of the work for that would be outside of this repo.

@ericstj
Copy link
Member

ericstj commented Sep 8, 2023

We ourselves also reference S.S.P.

We've actually removed most of these references in dotnet/runtime - making them private to exclude the package reference. Keeping the type forwards (dangling) for compat. I think System.Xaml and WindowsBase may still reference it, but it seems like they are only using for forwards. We do actually expose it from the WindowsDesktop shared framework refpack -- perhaps we can remove it entirely 9.0.

I had a look at MSBuild and I'm not sure it even uses System.Security.Permissions. Most that code looks to me like it's under ifdef that's only defined for NetFX - one exception seems to be XmlSyntaxException which needs an ifdef. It seems that it does still pull in SSP through a reference to 7.0 ConfiugrationManager, when updating to 8.0 we can remove this completely. I was able to update to 8.0 packages and remove SSP completely and it no longer shows up in the output. #9212. EDIT: Looks like a similar PR was already raised: #9055. I think that PR should be driven in.

@AR-May
Copy link
Member

AR-May commented May 14, 2024

Let's check that it was fixed.

@AR-May AR-May added triaged Priority:2 Work that is important, but not critical for the release and removed needs-triage Have yet to determine what bucket this goes in. labels May 14, 2024
@ViktorHofer
Copy link
Member

Yes, I think this got resolved.

@AR-May AR-May closed this as completed May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:2 Work that is important, but not critical for the release triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants