Skip to content

Don't include base FrameworkReferences when creating NuGet packages #3102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dsplaisted
Copy link
Member

Per comment from @nkolev92:

I think regardless of whether it gets reverted or not, the SDK should set "pack=false" to Microsoft.NETCore.App.

@dsplaisted dsplaisted requested review from nkolev92 and a team April 9, 2019 00:03
@@ -46,7 +46,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Publish="true" />
</ItemGroup>
<ItemGroup Condition=" '$(DisableImplicitFrameworkReferences)' != 'true' and '$(TargetFrameworkIdentifier)' == '.NETStandard' And '$(_TargetFrameworkVersionWithoutV)' >= '2.1'">
<FrameworkReference Include="NETStandard.Library" IsImplicitlyDefined="true" />
<FrameworkReference Include="NETStandard.Library" IsImplicitlyDefined="true" Pack="false"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a new FrameworkReference?

What does it mean compared to the rest of the frameworks?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're (going to be) using a targeting pack for .NET Standard, so the FrameworkReference is how we bring that in.

With Pack=false I think this should not affect NuGet or NuGet packages (just like the base Microsoft.NETCore.App FrameworkReference will be handled).

Basically, FrameworkReferences which are represented by a target framework moniker should have Pack=false and not be represented in NuGet packages or flow transitively. Does Pack=false have this behavior with the FrameworkReference work?

Copy link
Contributor

@nkolev92 nkolev92 Apr 9, 2019

Choose a reason for hiding this comment

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

The last part is new to me.
Restore does not account for Pack=false. It flows everything by default.

Basically the way it's currently working is targeted more for the optional shared frameworks.

Do we need to reconsider that? Do we need to add a PrivateAssets like switch to FrameworkReference items?

Copy link
Member Author

Choose a reason for hiding this comment

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

@nkolev92 Yes, we need to be able to prevent the NETStandard.Library FrameworkReference from flowing. Does Pack=false prevent it from being added to the NuGet package? To me it would make sense if that also prevented it from flowing through the projects, but we can also add a PrivateAssets-type flag to it if need be.

Copy link
Contributor

@nkolev92 nkolev92 Apr 10, 2019

Choose a reason for hiding this comment

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

Restore does not read the "pack" attribute and it probably shouldn't.
pack=false will prevent it from being included in the package only.

We should discuss this in a separate meeting or a Monday sync-up.

The existence of a netstandard framework reference also kills our transitivity with FrameworkReference story.

https://github.com/NuGet/Home/wiki/%5BSpec%5D-FrameworkReference-in-NuGet#what-happens-in-the-future-ifwhen-frameworkreference-are-added-to-other-frameworks

We need to rethink that as well.

Issue: NuGet/Home#7988

//cc @rrelyea

@dsplaisted
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted dsplaisted merged commit 7c52b29 into dotnet:master Apr 18, 2019
dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020
….6 (dotnet#3102)

- Microsoft.DotNet.Cli.Runtime - 3.1.100-preview1.19505.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants