Pack runs too often -- dotnet pack fails with There is a circular dependency in the target dependency graph involving target "Pack" #4381

Closed
rrelyea opened this Issue Jan 23, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@rrelyea
Contributor

rrelyea commented Jan 23, 2017

see details and attachments in vsfeedback 744337
filed by @ericstj

part of repro steps are here:
In trying to workaround other issues I went to the commandline.

Run dotnet pack on the attached csproj.
Expect: success
Actual: Fails with error
C:\Program Files\dotnet\sdk\1.0.0-rc3-004517\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(56,11): error MSB4006: There is a circular dependency in the target dependency graph involving target "Pack". [C:\Users\ericstj\Documents\Visual Studio 2017\Projects\ClassLibrary\ClassLibrary\ClassLibrary.csproj]

@rrelyea rrelyea added this to the 4.0 RTM milestone Jan 23, 2017

@rrelyea rrelyea changed the title from dotnet pack fails with There is a circular dependency in the target dependency graph involving target "Pack" to Pack runs too often -- dotnet pack fails with There is a circular dependency in the target dependency graph involving target "Pack" Jan 24, 2017

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jan 24, 2017

Contributor

The issue here is that when $(GeneratePackageOnBuild) property is set in a csproj (this is done via Packages page of a project in Visual Studio), we register "Pack" to be run as an AfterTarget of "Build". However, the Pack target, by default, has Build as one of the DependsOnTargets, thus introducing a circular dependency in the target dependency graph.

The proposed solution here is :

  • If $(GeneratePackageOnBuild) is set to True, we coerce $(NoBuild) property to be true as well, which removes Build from the list of targets that Pack depends on. As a result if someone calls Pack, we do not build the project. If Pack is called on a project which hasn't been built, it blows up with a FileNotFoundException when it tries to find the DLLs it has to add, but cannot find them on disk. Another change we make under the hood is that instead of calling Pack as an AfterTarget of Build, we append the pack target to $(BuildDependsOn), which also solves the problem on Pack being called after each inner build because of being an AfterTarget of build and thus also fixes : #4380

  • If $(GeneratePackageOnBuild) is set to false, we respect the defaults, and let the Pack target depend on Build.
    Thoughts?
    CC : @rrelyea @srivatsn @PiotrP @emgarten @blackdwarf

Contributor

rohit21agrawal commented Jan 24, 2017

The issue here is that when $(GeneratePackageOnBuild) property is set in a csproj (this is done via Packages page of a project in Visual Studio), we register "Pack" to be run as an AfterTarget of "Build". However, the Pack target, by default, has Build as one of the DependsOnTargets, thus introducing a circular dependency in the target dependency graph.

The proposed solution here is :

  • If $(GeneratePackageOnBuild) is set to True, we coerce $(NoBuild) property to be true as well, which removes Build from the list of targets that Pack depends on. As a result if someone calls Pack, we do not build the project. If Pack is called on a project which hasn't been built, it blows up with a FileNotFoundException when it tries to find the DLLs it has to add, but cannot find them on disk. Another change we make under the hood is that instead of calling Pack as an AfterTarget of Build, we append the pack target to $(BuildDependsOn), which also solves the problem on Pack being called after each inner build because of being an AfterTarget of build and thus also fixes : #4380

  • If $(GeneratePackageOnBuild) is set to false, we respect the defaults, and let the Pack target depend on Build.
    Thoughts?
    CC : @rrelyea @srivatsn @PiotrP @emgarten @blackdwarf

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Jan 24, 2017

I don't even think its right to run pack on build. See some of the other issues I filed.
https://developercommunity.visualstudio.com/content/problem/13228/packtask-failed-unexpectedly-when-creating-package.html
https://developercommunity.visualstudio.com/content/problem/13286/pack-target-for-cross-compiling-class-library-only.html

Pack shouldn't be run in the context of TFM-specific Build at all. It needs to run in the same context as Restore.

ericstj commented Jan 24, 2017

I don't even think its right to run pack on build. See some of the other issues I filed.
https://developercommunity.visualstudio.com/content/problem/13228/packtask-failed-unexpectedly-when-creating-package.html
https://developercommunity.visualstudio.com/content/problem/13286/pack-target-for-cross-compiling-class-library-only.html

Pack shouldn't be run in the context of TFM-specific Build at all. It needs to run in the same context as Restore.

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jan 25, 2017

Contributor

@ericstj curious why you think it's not right to run pack on build?

As for your other issues, we are tracking them at : #4380 and #4289

Contributor

rohit21agrawal commented Jan 25, 2017

@ericstj curious why you think it's not right to run pack on build?

As for your other issues, we are tracking them at : #4380 and #4289

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Jan 25, 2017

@rohit21agrawal for the same reason that publish doesn't run on build. It's not needed for the inner-loop scenarios. /cc @blackdwarf

ericstj commented Jan 25, 2017

@rohit21agrawal for the same reason that publish doesn't run on build. It's not needed for the inner-loop scenarios. /cc @blackdwarf

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jan 25, 2017

Contributor

@ericstj i agree that pack shouldn't run after inner build.

but we have a property that can be set via the VS UI called GeneratePackageOnBuild (this is set to False by default) , and the right behavior for it is to be able to generate nupkg after the outer build is complete. Do you think it's wrong to have such a capability?

Contributor

rohit21agrawal commented Jan 25, 2017

@ericstj i agree that pack shouldn't run after inner build.

but we have a property that can be set via the VS UI called GeneratePackageOnBuild (this is set to False by default) , and the right behavior for it is to be able to generate nupkg after the outer build is complete. Do you think it's wrong to have such a capability?

@ericstj

This comment has been minimized.

Show comment
Hide comment
@ericstj

ericstj Jan 25, 2017

Inner build, yeah: agreed. I meant something different by inner-loop. Referring to when a developer is doing an "inner-loop" development cycle; fast successive source & test changes. You don't want that to have unnecessary steps.

It seems odd to me. CLI didn't have that behavior AFAIK. I'm also unaware of any other project type that does its publish as part of build (though I am sure there are some). Most that i can think of have a separate publish step, to keep inner-loop lean and make signing straight forward, etc. That said, you aren't making it the default here, just an option. But it seems to me the option that most folks should use is calling pack directly since most of the time they don't need the nupkg and you're over-promoting the pack on build by giving it a checkbox in the UI. My suggestion here is DCR-level so I don't want you to take my word for it. Obviously the folks who spec'ed that feature should weigh in and make sure the e2e scenarios they envision are being realized.

ericstj commented Jan 25, 2017

Inner build, yeah: agreed. I meant something different by inner-loop. Referring to when a developer is doing an "inner-loop" development cycle; fast successive source & test changes. You don't want that to have unnecessary steps.

It seems odd to me. CLI didn't have that behavior AFAIK. I'm also unaware of any other project type that does its publish as part of build (though I am sure there are some). Most that i can think of have a separate publish step, to keep inner-loop lean and make signing straight forward, etc. That said, you aren't making it the default here, just an option. But it seems to me the option that most folks should use is calling pack directly since most of the time they don't need the nupkg and you're over-promoting the pack on build by giving it a checkbox in the UI. My suggestion here is DCR-level so I don't want you to take my word for it. Obviously the folks who spec'ed that feature should weigh in and make sure the e2e scenarios they envision are being realized.

@niemyjski

This comment has been minimized.

Show comment
Hide comment
@niemyjski

niemyjski Feb 4, 2017

Can we get a new RC out soon. There are so many issues I've hit this week it's not even funny. The good news is most if not all of them have been fixed. We really need a new rc this coming week.

Can we get a new RC out soon. There are so many issues I've hit this week it's not even funny. The good news is most if not all of them have been fixed. We really need a new rc this coming week.

niemyjski added a commit to FoundatioFx/Foundatio that referenced this issue Feb 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment