Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Omissions in types supported by both Desktop and Core - System.Security.AccessControl.AuthorizationRuleCollection #17604

Conversation

ViktorHofer
Copy link
Member

Adjustments for System.Security.AccessControl.AuthorizationRuleCollection to correctly inherit from System.Collections.NonGeneric.ReadOnlyCollectionBase. For more info see here: https://github.com/dotnet/corefx/issues/15255#issuecomment-289550875

@ViktorHofer ViktorHofer force-pushed the CannotRemoveBaseTypeOrInterface-ReadOnlyCollectionBase branch from 6d79c52 to 8d74fa8 Compare March 28, 2017 12:34
@ViktorHofer ViktorHofer self-assigned this Mar 28, 2017
@ViktorHofer ViktorHofer added this to the 2.0.0 milestone Mar 28, 2017
@ViktorHofer ViktorHofer added area-Meta area-System.Security blocking-release tenet-compatibility Incompatibility with previous versions or .NET Framework labels Mar 28, 2017
@@ -34,6 +34,7 @@
<Compile Include="System\IO\FileSystemAclExtensions.net46.cs" />
</ItemGroup>
<ItemGroup Condition="'$(TargetGroup)'!='netfx'">
<Reference Include="System.Collections.NonGeneric" />
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Indentation

@@ -120,6 +120,7 @@
</ItemGroup>
<ItemGroup>
<Reference Include="System.Collections" />
<Reference Include="System.Collections.NonGeneric" />
Copy link
Member

Choose a reason for hiding this comment

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

Before we merge this -- @joperezr @weshaggard

S.S.AccessControl builds for netstandard but S.C.NonGeneric builds for netcoreapp and uap specifically. So assume this reference is prohibited. But why does NonGenreic not build for netstandard only?

Copy link
Member

@danmoseley danmoseley Mar 28, 2017

Choose a reason for hiding this comment

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

Oh, it's because it depends on System.Runtime and S.R.Extensions and S.Globalization and those don't have netstandard configurations. Can we unravel this somehow?

If I change S.C.NG to netstandard I get C:\git\corefx\buildvertical.targets(153,5): error : Could not find a configuration for ProjectReference '..\..\System.Runtime\ref\System.Runtime.csproj' from configurations 'netcoreapp;uap' when building 'System.Collections.NonGeneric' for configuration 'netstandard-Debug' . [C:\git\corefx\src\System.Collections.NonG eneric\ref\System.Collections.NonGeneric.csproj]

Copy link
Member

Choose a reason for hiding this comment

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

I'm missing something conceptually -- I guess types in netstandard can be defined in libraries that don't have a netstandard configuration. That makes sense I guess. So is this new reference legal?

Copy link
Member

Choose a reason for hiding this comment

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

S.S.AccessControl doesn't build for netstandard, at least not the implementation. The implementation is also platform-specific (uap, netcoreapp) which is why this is a valid reference.

Copy link
Member

Choose a reason for hiding this comment

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

That's what confused me -- the ref is built for netstandard. Why's that? Do we just do that for convenience whenever it happens to compile successfully for netstandard?

Copy link
Member

Choose a reason for hiding this comment

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

We should actually consider switching this library to be netstandard but if we do that then we may need to set some explicit configurations to Generate platform not supported exceptions (i.e. unix and maybe uap).

Copy link
Member Author

Choose a reason for hiding this comment

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

should we merge the PR for now and track these ideas in a separate issue?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with merging this once CI is green. We do need to file an issue about all the AccessControl APIs about whether or not we want to support then for netstandard or not. @joperezr was going to file a similar issue, so chat with him about it.

Copy link
Member

Choose a reason for hiding this comment

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

I logged: #17660

@weshaggard
Copy link
Member

@ViktorHofer it looks like you didn't correctly merge with the latest from master as your PR shows a lot of files which aren't relevant to your changes.

@ViktorHofer
Copy link
Member Author

@weshaggard yes, alread on it...

@ViktorHofer ViktorHofer force-pushed the CannotRemoveBaseTypeOrInterface-ReadOnlyCollectionBase branch from 8ae1649 to 505b5b9 Compare March 28, 2017 22:43
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Mar 28, 2017

@weshaggard Tests should finish soon. Please take another look. As stated above if you agree I would merge this PR and open a new issue with what you suggested.

@ViktorHofer ViktorHofer merged commit 922b702 into dotnet:master Mar 29, 2017
@ViktorHofer ViktorHofer deleted the CannotRemoveBaseTypeOrInterface-ReadOnlyCollectionBase branch March 29, 2017 00:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants