This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Omissions in types supported by both Desktop and Core - System.Security.AccessControl.AuthorizationRuleCollection #17604
Merged
ViktorHofer
merged 9 commits into
dotnet:master
from
ViktorHofer:CannotRemoveBaseTypeOrInterface-ReadOnlyCollectionBase
Mar 29, 2017
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
af09a9a
AuthorizationRuleCollection inheritance changed to ReadOnlyCollection…
ViktorHofer ebaeeaf
fixed link path
ViktorHofer 813dbd1
refs for AuthorizationRuleCollection adjusted
ViktorHofer 9724e07
Included refs and updated ApiCompat baseline
ViktorHofer f424140
Indentation fixed in csproj
ViktorHofer 468877b
AuthorizationRuleCollection inheritance changed to ReadOnlyCollection…
ViktorHofer e6ac95b
fixed link path
ViktorHofer 505b5b9
Included refs and updated ApiCompat baseline
ViktorHofer 66fd645
updated baseline
ViktorHofer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 0 additions & 20 deletions
20
src/System.Security.AccessControl/ref/System.Security.AccessControl.Manual.cs
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Before we merge this -- @joperezr @weshaggard
S.S.AccessControl builds for
netstandard
but S.C.NonGeneric builds fornetcoreapp
anduap
specifically. So assume this reference is prohibited. But why does NonGenreic not build fornetstandard
only?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.
Oh, it's because it depends on
System.Runtime
andS.R.Extensions
andS.Globalization
and those don't havenetstandard
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]
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'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?
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.
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.
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.
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?
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.
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).
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.
should we merge the PR for now and track these ideas in a separate issue?
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'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.
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 logged: #17660