Fix up the set of Windows compatibility libraries exposed in WindowsDesktop - #7324
Fix up the set of Windows compatibility libraries exposed in WindowsDesktop#7324dagood wants to merge 4 commits into
Conversation
MichaelSimons
left a comment
MichaelSimons
left a comment
There was a problem hiding this comment.
I have no insight on the System.IO.Pipes.AcessControl version assumption.
| <MicrosoftDiaSymReaderNativePackageVersion>1.7.0</MicrosoftDiaSymReaderNativePackageVersion> | ||
| <!-- Infrastructure and test-only. --> | ||
| <MicrosoftSourceLinkVersion>1.0.0-beta2-18618-05</MicrosoftSourceLinkVersion> | ||
| <!-- TODO: Sort these into the above. These are out of the way at the moment to make it easier to resolve conflicts. --> |
There was a problem hiding this comment.
I'll do these with my merge commit. (If this PR is the way to go.)
| <IgnoredReference Condition="'$(PackageTargetRuntime)' == ''" Include="DirectWriteForwarder" /> | ||
|
|
||
| <!-- We want to intentionally exclude the ref assembly for System.Security.Permissions. --> | ||
| <IgnoredReference Condition="'$(PackageTargetRuntime)' == ''" Include="System.Security.Permissions" /> |
There was a problem hiding this comment.
Careful here: who's referencing this? I would have expected that reference to be gone. We should wait to remove it until it is.
There was a problem hiding this comment.
I see... when I remove the ignore, the list is:
Assembly 'System.Xaml' is missing dependency 'System.Security.Permissions'
Assembly 'System.Configuration.ConfigurationManager' is missing dependency 'System.Security.Permissions'
Assembly 'System.Security.Cryptography.Xml' is missing dependency 'System.Security.Permissions'
Assembly 'WindowsBase' is missing dependency 'System.Security.Permissions'
Assembly 'System.Windows.Forms.Design.Editors' is missing dependency 'System.Security.Permissions'
Assembly 'System.Drawing' is missing dependency 'System.Security.Permissions'
Assembly 'System.DirectoryServices' is missing dependency 'System.Security.Permissions'
There was a problem hiding this comment.
We need to determine why this stuff is pulling it in. Dangling type-forwards are OK, actual type references are not.
There was a problem hiding this comment.
We haven't had an update from WPF for a while, and it looks like there are some relevant changes we don't have yet: dotnet/wpf@ea72094...master.
There was a problem hiding this comment.
Ah... dotnet-wpf-int isn't producing for 5.0 yet. Filed https://github.com/dotnet/core-setup/issues/7343 to figure out how to get these updated. For now, I'll keep System.Security.Permissions so the rest can be mergeable.
There was a problem hiding this comment.
That's not the only cause for this. Some of these other dependencies are concerning. I suspect we'll need to suppress this, since even once everything is fixed WindowsBase will have typeforwards to System.Security.Permission, but we need to examine all the references before suppressing to find out if there is other work to do to safely omit this.
There was a problem hiding this comment.
Filed https://github.com/dotnet/core-setup/issues/7344 to follow up on this, I'll keep this PR to what seems doable before then. Thanks for the explanation, this makes sense. (I don't know what the best way to check the references is to get this done myself.)
| <RuntimeOnlyPackageReference Include="System.DirectoryServices" Version="$(SystemDirectoryServicesVersion)" /> | ||
| <RuntimeOnlyPackageReference Include="System.IO.FileSystem.AccessControl" Version="$(SystemIOFileSystemAccessControlVersion)" /> | ||
| <RuntimeOnlyPackageReference Include="System.Threading.AccessControl" Version="$(SystemThreadingAccessControlVersion)" /> | ||
| <RefOnlyPackageReference Include="System.IO.Pipes.AccessControl" Version="$(SystemIOPipesAccessControl)" /> |
There was a problem hiding this comment.
Why did you need to explicitly make it RefOnly? Can we instead make conflict resolution do the work to remove anything that would conflict with the base shared-framework and lose?
There was a problem hiding this comment.
AFAIK we don't have tooling like that yet, I created https://github.com/dotnet/core-setup/issues/7340 to track that. Definitely seems possible.
There was a problem hiding this comment.
What do you mean by "tooling like that"? It should be built into the SDK. You just need to set a property that points to the platform manifest you generated in this repository.
There was a problem hiding this comment.
Scratch that, an item. Try just setting PackageConflictPlatformManifests item.
There was a problem hiding this comment.
That's exactly what I mean, wasn't aware of that. 😄 I'll give it a try.
There was a problem hiding this comment.
Seems to work as expected! Thanks!
I had to change GetNETCoreAppIgnoredReference a bit for closure validation: when verifying the runtime package, it ignores netcoreapp refs, not runtimes, causing a number of missing reference errors. I think that was just a bug that happened to rely on the asset duplication we had.
|
I diffed the data files (runtimelist/frameworklist) to get a summary of the results: https://gist.github.com/dagood/5f328e6148acd7cc8ee1afcc6c60f814. All the removals are in the NETCoreApp sfx, other changes look like what we intended. |
|
I didn't rerun crossgen in my local build, which looks like it's failing now. Looking into it. |
|
It's due to WindowsBase removal: |
|
I'm going to apply this PR to |
|
Should probably move this PR to the new dotnet/windowsdesktop repo. |
|
This PR makes changes to some files now in the sharedfx tooling SDK and relies on the fact that netcoreapp and windowsdesktop are both in this repo, so I can't actually move the PR over. However, I opened dotnet/windowsdesktop#7 to track this and I'm closing this PR. |
4 participants
For https://github.com/dotnet/core-setup/issues/7290.
This PR assumes that we should use
System.IO.Pipes.AccessControl4.5.1. (https://github.com/dotnet/core-setup/issues/7290#issuecomment-513926657)