Skip to content
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

dotnet pack3 always adds exclude="Build,Analyzers" to .nuspec #3697

Closed
joelverhagen opened this issue Oct 19, 2016 · 7 comments
Closed

dotnet pack3 always adds exclude="Build,Analyzers" to .nuspec #3697

joelverhagen opened this issue Oct 19, 2016 · 7 comments
Assignees
Labels
Milestone

Comments

@joelverhagen
Copy link
Member

Steps

  1. dotnet new a .csproj
  2. dotnet restore3
  3. dotnet pack3
  4. Open up the .nuspec in the produced .nupkg

Expected

The Microsoft.NETCore.App dependency should look like this:

<dependency id="Microsoft.NETCore.App" version="1.0.1" />

Actual

It looks like this:

<dependency id="Microsoft.NETCore.App" version="1.0.1" exclude="Build,Analyzers" />
@rohit21agrawal rohit21agrawal self-assigned this Oct 19, 2016
@rrelyea rrelyea added this to the 3.6 RC milestone Oct 20, 2016
@emgarten
Copy link
Member

exclude="Build,Analyzers" exists for package/project duality.

Example 1
ProjectA -> ProjectB -> ProjectC (contains c.targets)
Result: ProjectA does not reference c.targets

Example 2
ProjectA -> ProjectB -(exclude build, analyzers)-> ProjectC (contains c.targets)
Result: ProjectA does not reference c.targets

Example 3
ProjectA -> PackageB -(no excludes)-> PackageC (contains c.targets)
Result: ProjectA includes c.targets

The reason dotnet pack does not add these excludes is because it was written before the flags existed and never updated.

I think the issue here is, should nuget pack continue writing out the dependency exactly as the project contained it, or should it drop this extra information and leave it up to the user to decide how to handle it.

@rrelyea rrelyea modified the milestones: 4.0 RC2, 4.0 RC Oct 27, 2016
@rrelyea
Copy link
Contributor

rrelyea commented Oct 27, 2016

lock down behavior in rc2. but this feels right.

@rrelyea rrelyea modified the milestones: 4.0 RC2, 4.0 RC3 Nov 29, 2016
@rrelyea rrelyea modified the milestones: 4.0 RTM, 4.0 RC3 Jan 10, 2017
@rrelyea rrelyea modified the milestones: 4.0.1, 4.0 RTM Jan 24, 2017
@rrelyea
Copy link
Contributor

rrelyea commented Jan 24, 2017

will consider adapting in future, if it becomes issue.

@emgarten
Copy link
Member

@davidfowl what are your thoughts on this?

Having these excludes is technically correct for duality, but if the dependency packages later change and start including build assets the older parent package would block them, which may be unintentional.

@dsplaisted
Copy link

Does this mean by default you won't get build targets from transitive package dependencies? That seems really wrong.

I think the ideal way to support package project duality with this would be to have you get the targets imported when you reference a project. I'm not sure how feasible that is.

It also seems like the current behavior also breaks package / project duality. Consider the following:

ProjectA -> PackageB (with b.targets)
Result: ProjectA imports b.targets

ProjectA -> ProjectB (with b.targets)
Result: ProjectA doesn't import b.targets

@dsplaisted
Copy link

I ran into this today. The xunit.performance repo instructs you to reference the xunit.performance package as well as a version of Microsoft.Diagnostics.Tracing.TraceEvent:

  1. Add a reference to the latest xunit.performance.api.dll
  2. Add a reference to the latest Microsoft.Diagnostics.Tracing.TraceEvent
    (It deploys native libraries needed to merge the *.etl files)

This is because the TraceEvent package includes native libraries which are included via a .targets file. So even though the xunit.performance package has a dependency on the TraceEvent package, when you consume the xunit.performance package the native assets don't flow through to the consuming project, because by default the dependency has exclude=Build. It looks like they tried to fix this by specifying <IncludeAssets>All</IncludeAssets>, but that doesn't help. The correct workaround is to set <PrivateAssets>None</PrivateAssets>, but this is not easily discoverable.

@jainaashish
Copy link
Contributor

We've decided to change this default behavior for build assets and being tracking via #6091

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants