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

Netstandard closure in NETCoreApp #15313

Merged
merged 2 commits into from Jan 19, 2017

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jan 19, 2017

Fixes #15083.

This adds a number of assemblies to netcoreapp.

The process I used to do this was look at our current NETCoreApp netstandard.dll ref and add all of its dependencies as both ref & lib, since we must support neststandard.dll. I then re-enabled closure validation for netstandard, then I added all other libs needed to make it closure complete.

@weshaggard please review. If you see anything here that shouldn't be it probably means we need to push types down.

For every assembly that is forwarded to by netstandard.dll, ensure it is
a reference as part of NETCore.App
@ericstj
Copy link
Member Author

ericstj commented Jan 19, 2017

@dotnet-bot test Innerloop Windows_NT Debug Build and Test

@@ -3,5 +3,7 @@
<Import Project="..\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.0.2.0</AssemblyVersion>
<IsNETCoreApp>true</IsNETCoreApp>
Copy link
Member

Choose a reason for hiding this comment

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

This one looks suspect. We should understand what is pulling it in.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an implementation dependency of IsolatedStorage:

<Reference Include="System.IO.FileSystem.AccessControl" />

@JeremyKuhne added the dependency here: 86f5542

Copy link
Member

Choose a reason for hiding this comment

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

@JeremyKuhne do we need this dependency? It will require us to pull in ACL's and its closure into the shared framework.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm filing an issue for this and moving on.

Copy link
Member

Choose a reason for hiding this comment

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

@weshaggard We need the functionality for sure. We could copy the code I suppose but I'm a little worried about missing fixes given this is code we haven't really extensively utilized.

Copy link
Member Author

Choose a reason for hiding this comment

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

No need to copy it, just move it to common. Let's move the discussion over to #15337

@ericstj ericstj merged commit b92863a into dotnet:master Jan 19, 2017
@karelz karelz modified the milestone: 2.0.0 Jan 21, 2017
Copy link
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

LGTM

We should compare and make sure we have all the shims we have in the netstandard library package now.

@ericstj
Copy link
Member Author

ericstj commented Jan 23, 2017

We should compare and make sure we have all the shims we have in the netstandard library package now.

@weshaggard NETStandard.Library is the source of that info, NETCore.App just makes sure it has everything NETStandard.Library has. This change was just trying to anticipate what you were going to add.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…re-NETCoreApp

Netstandard closure in NETCoreApp

Commit migrated from dotnet/corefx@b92863a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants