Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Move XamlAccessLevel to System.Windows.Extensions.dll#38908

Merged
bartonjs merged 8 commits intomasterfrom
dev/vatsan/xaml-perms
Jul 3, 2019
Merged

Move XamlAccessLevel to System.Windows.Extensions.dll#38908
bartonjs merged 8 commits intomasterfrom
dev/vatsan/xaml-perms

Conversation

@vatsan-madhavan
Copy link
Copy Markdown
Member

@vatsan-madhavan vatsan-madhavan commented Jun 25, 2019 •
edited
Loading

As part of dotnet/wpf#969 (and other PR's), WPF is trying to remove its references to System.Security.Permissions and remove CAS related code-patterns. In order to complete this work, XamlAccessLevel and XamlLoadPermission had been moved to System.Windows.Extensions.dll

The idea was to decouple WPF from XamlLoadPermission and XamlAccessLevel completely. We have come to realize that decoupling from XamlLoadPermission has been relatively straightforward, but removing the dependency on XamlAccessLevel is proving to be hard. WPF depends on XamlAccessLevel for non-CAS related patterns (and exposes these to developers).

We believe that we need continued access to XamlAccessLevel. In order to do this right, one approach being proposed here is to move XamlAccessLevel to System.Windows.Extensions.dll.

Note that I'm proposing that XamlLoadPermission would continue to reside in System.Security.Permission.dll - moving this type to System.Windows.Extensions.dll would make it so that accessing XamlAccessLevel would pull in a reference to System.Security.Permissions.dll - an effect we'd like to avoid.

/cc @ericstj , @rladuca , @ojhad
/cc @dotnet/wpf-developers

Edit: updating description to reflect change in proposal. instead of introducing a new assembly System.Xaml.Permissions.dll, the proposal is now to move XamlAccessLevel to System.Windows.Extensions.dll

@ericstj
Copy link
Copy Markdown
Member

ericstj commented Jun 25, 2019

I’m not thrilled about a new assembly for a single type. Did you consider alternatives? /cc @bartonjs

@vatsan-madhavan
Copy link
Copy Markdown
Member Author

vatsan-madhavan commented Jun 25, 2019 •
edited
Loading

I’m not thrilled about a new assembly for a single type. Did you consider alternatives?

We'd need a candidate assembly that is referenced by System.Security.Permissions, and can serve as a home for XamlAccessLevel.

How do you feel about moving it to System.Runtime or System.Runtime.Extensions rather than creating a new assembly for it?

@vatsan-madhavan
Copy link
Copy Markdown
Member Author

vatsan-madhavan commented Jun 25, 2019

I just noticed that System.Runtime.Extensions is already home to a few System.Security.Permissions related types.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Jun 26, 2019 •
edited
Loading

System.Windows.Extensions may be a good home for this. I do not think this belongs to System.Runtime.Extensions.

@vatsan-madhavan
Copy link
Copy Markdown
Member Author

vatsan-madhavan commented Jun 26, 2019 •
edited
Loading

System.Windows.Extensions may be a good home for this. I do not think this belongs to System.Runtime.Extensions.

That should work for us. Microsoft.WindowsDesktop.App already depends on (and packages) System.Windows.Extensions.

System.Security.Permissions would need a reference to System.Windows.Extensions - is this ok?

@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/xaml-perms branch from cf2b403 to d33e9b4 Compare June 26, 2019 05:39
@vatsan-madhavan vatsan-madhavan changed the title Move XamlAccessLevel to new assembly System.Xaml.Permissions.dll Move XamlAccessLevel to new assembly System.Windows.Extensions.dll Jun 26, 2019
@vatsan-madhavan vatsan-madhavan changed the title Move XamlAccessLevel to new assembly System.Windows.Extensions.dll Move XamlAccessLevel to System.Windows.Extensions.dll Jun 26, 2019
@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/xaml-perms branch from 042611d to aa11b99 Compare June 26, 2019 08:10
@ericstj
Copy link
Copy Markdown
Member

ericstj commented Jun 26, 2019

I'd imagine we don't care so much about System.Security.Permissions alone? It would mean S.S.P gets a couple new package dependencies: System.Windows.Extensions, and transitively, System.Drawing.Common. What do folks think about that?

@vatsan-madhavan vatsan-madhavan changed the title Move XamlAccessLevel to System.Windows.Extensions.dll WIP: Move XamlAccessLevel to System.Windows.Extensions.dll Jun 26, 2019
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Jun 26, 2019 •
edited
Loading

System.Security.Permissions

Agree. I do not think we care much about this package.

@stephentoub
Copy link
Copy Markdown
Member

stephentoub commented Jun 26, 2019

Agreed.

@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/xaml-perms branch 2 times, most recently from 5d08b61 to 8a3709f Compare July 2, 2019 20:07
@vatsan-madhavan vatsan-madhavan changed the title WIP: Move XamlAccessLevel to System.Windows.Extensions.dll Move XamlAccessLevel to System.Windows.Extensions.dll Jul 2, 2019
@vatsan-madhavan vatsan-madhavan requested a review from ericstj July 2, 2019 21:46
@vatsan-madhavan
Copy link
Copy Markdown
Member Author

vatsan-madhavan commented Jul 2, 2019

Removing WIP from title.

@ericstj, would you mind taking a look at this PR?

ericstj
ericstj reviewed Jul 2, 2019
View reviewed changes
Comment thread src/System.Security.Permissions/ref/System.Security.Permissions.Forwards.cs Outdated
Comment thread src/System.Security.Permissions/ref/System.Security.Permissions.cs Outdated
@vatsan-madhavan
Move XamlAccessLevel to System.Windows.Extensions
a1bb317 
WPF needs continued access to XamlAccessLevel type. System.Xaml.Permissions.XamlLoadPermission also needs to use XamlAccessLevel.

WPF is trying to remove its references to System.Security.Permissions. In order to complete this work, XamlAccessLevel is being moved to System.Windows.Extensions.dll (while leaving XamlLoadPermission in System.Permission.dll as-is). This will allow WindowsDesktop.App to bundle System.Windows.Extensions.dll (but not carry System.Security.Permissions.dll).
@vatsan-madhavan
Limit to netcoreapp/windows
1ed17d7
@vatsan-madhavan
Conditioning XamlLoadPermission tests to run only under netcoreapp
b16c62a
@vatsan-madhavan
Add missing namespaces
35c400d
@vatsan-madhavan
Limit XamlLoadPermission tests to Windows only
f8a4f82
@vatsan-madhavan
Add System.Security.Permissions.netcoreapp.cs
636a7b4
@vatsan-madhavan
Remove type-forward for XamlAccessLevel - this is not needed since th…
28f31c9 
…is only serves preview7 to preview 8 compatiblity needs.
@vatsan-madhavan vatsan-madhavan force-pushed the dev/vatsan/xaml-perms branch from 8a3709f to 28f31c9 Compare July 3, 2019 19:48
ericstj
ericstj approved these changes Jul 3, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM @bartonjs do you care to give it a look as System.Security owner?

@bartonjs bartonjs merged commit 758b1bf into master Jul 3, 2019
@vatsan-madhavan vatsan-madhavan deleted the dev/vatsan/xaml-perms branch July 3, 2019 23:27
@rladuca rladuca mentioned this pull request Jul 7, 2019
Closed
@ojhad ojhad mentioned this pull request Jul 8, 2019
Merged
@dagood dagood mentioned this pull request Jul 9, 2019
Merged
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Jul 9, 2019
@dougbu
React to new transitive dependencies
da3a9df 
- see dotnet/corefx#38908

nit: remove duplicate URL for the dotnet-core blob feed
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Jul 9, 2019
@dougbu
Update dependencies from 5 repositories (#11939)
c07d39d 
* Update dependencies from build '20190706.1' of 'https://github.com/dotnet/arcade'
  - 'Microsoft.DotNet.Arcade.Sdk': '1.0.0-beta.19351.4' => '1.0.0-beta.19356.1'
  - 'Microsoft.DotNet.GenAPI': '1.0.0-beta.19351.4' => '1.0.0-beta.19356.1'
  - 'Microsoft.DotNet.Helix.Sdk': '2.0.0-beta.19351.4' => '2.0.0-beta.19356.1'

* Update dependencies from build '20190706.2' of 'https://github.com/dotnet/roslyn'
  - 'Microsoft.Net.Compilers.Toolset': '3.3.0-beta1-19351-01' => '3.3.0-beta2-19356-02'

* Update dependencies from build '20190706.5' of aspnet/AspNetCore-Tooling
  - 'Microsoft.AspNetCore.Mvc.Razor.Extensions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19356.5'
  - 'Microsoft.AspNetCore.Razor.Language': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19356.5'
  - 'Microsoft.CodeAnalysis.Razor': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19356.5'
  - 'Microsoft.NET.Sdk.Razor': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19356.5'

* Update dependencies from build '20190705.1' of 'https://github.com/aspnet/Blazor'
  - 'Microsoft.AspNetCore.Blazor.Mono': '0.10.0-preview8.19351.2' => '0.10.0-preview8.19355.1'

* Update dependencies from from build '20190704.5' of 'https://github.com/aspnet/EntityFrameworkCore'
  - 'Microsoft.EntityFrameworkCore.Tools': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'Microsoft.EntityFrameworkCore.SqlServer': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'dotnet-ef': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'Microsoft.EntityFrameworkCore': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'Microsoft.EntityFrameworkCore.InMemory': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'Microsoft.EntityFrameworkCore.Relational': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'
  - 'Microsoft.EntityFrameworkCore.Sqlite': '3.0.0-preview8.19353.14' => '3.0.0-preview8.19354.5'

Coherency updates...
  - 'Microsoft.AspNetCore.Analyzer.Testing': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.AspNetCore.BenchmarkRunner.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.ActivatorUtilities.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Caching.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Caching.Memory': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Caching.SqlServer': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Caching.StackExchangeRedis': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.CommandLineUtils.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.AzureKeyVault': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.Binder': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.CommandLine': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.EnvironmentVariables': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.FileExtensions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.Ini': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.Json': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.KeyPerFile': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.UserSecrets': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration.Xml': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Configuration': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.DependencyInjection.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.DependencyInjection': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.DiagnosticAdapter': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Diagnostics.HealthChecks.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Diagnostics.HealthChecks': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.FileProviders.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.FileProviders.Composite': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.FileProviders.Embedded': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.FileProviders.Physical': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.FileSystemGlobbing': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.HashCodeCombiner.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Hosting.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Hosting': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.HostFactoryResolver.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Http': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Localization.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Localization': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.Abstractions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.AzureAppServices': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.Configuration': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.Console': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.Debug': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.EventSource': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.EventLog': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.TraceSource': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Logging.Testing': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.ObjectPool': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Options.ConfigurationExtensions': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Options.DataAnnotations': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Options': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.ParameterDefaultValue.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.Primitives': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.TypeNameHelper.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.ValueStopwatch.Sources': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Extensions.WebEncoders': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Internal.Extensions.Refs': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.JSInterop': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Mono.WebAssembly.Interop': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Internal.AspNetCore.Analyzers': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'
  - 'Microsoft.AspNetCore.Testing': '3.0.0-preview8.19353.10' => '3.0.0-preview8.19354.4'

  - 'Microsoft.Extensions.DependencyModel': '3.0.0-preview8-27903-07' => '3.0.0-preview8-27904-06'
  - 'Microsoft.NETCore.App.Ref': '3.0.0-preview8-27903-07' => '3.0.0-preview8-27904-06'
  - 'Microsoft.NETCore.App.Runtime.win-x64': '3.0.0-preview8-27903-07' => '3.0.0-preview8-27904-06'
  - 'NETStandard.Library.Ref': '2.1.0-preview8-27903-07' => '2.1.0-preview8-27904-06'

  - 'Microsoft.Bcl.AsyncInterfaces': '1.0.0-preview8.19353.3' => '1.0.0-preview8.19354.4'
  - 'Microsoft.CSharp': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'Microsoft.NETCore.Platforms': '3.0.0-preview8.19353.3' => '3.0.0-preview8.19354.4'
  - 'Microsoft.Win32.Registry': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.ComponentModel.Annotations': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Diagnostics.EventLog': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.IO.Pipelines': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Net.Http.WinHttpHandler': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Net.WebSockets.WebSocketProtocol': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Reflection.Metadata': '1.7.0-preview8.19353.3' => '1.7.0-preview8.19354.4'
  - 'System.Runtime.CompilerServices.Unsafe': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Security.Cryptography.Cng': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Security.Cryptography.Pkcs': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Security.Cryptography.Xml': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Security.Permissions': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Security.Principal.Windows': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.ServiceProcess.ServiceController': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Text.Encodings.Web': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Text.Json': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'
  - 'System.Threading.Channels': '4.6.0-preview8.19353.3' => '4.6.0-preview8.19354.4'

* React to new transitive dependencies
- see dotnet/corefx#38908

nit: remove duplicate URL for the dotnet-core blob feed

* Remove `.Layouts` namespace from Blazor (thanks @rynowak❕)

* fix one more straggler (thanks @rynowak❕)
@karelz karelz added this to the 3.0 milestone Jul 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ericstj ericstj ericstj approved these changes

@bartonjs bartonjs bartonjs approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@vatsan-madhavan @ericstj @jkotas @stephentoub @bartonjs @karelz