NuGet / Home

Repo for NuGet Client issues
Other
1.49k stars 250 forks source link

Allow package authors to provide AfterRestore behavior via MSBuild #7617

Open kzu opened 5 years ago

kzu commented 5 years ago

When PackageReference came along and dropped support for install.ps1, package authors lost the ability to perform some potentially expensive one-time stuff during install. Examples of this might be some additional downloads, extraction of embedded resources or pre-generation of immutable stuff (i.e. generating some obj-style code that never changes for a given shipped package (i.e. Xamarin/Java bindings for libraries, etc.).

A potential solution would be to introduce a native MSBuild mechanism by which package authors, through the same build targets they can currently provide, can expose a target to run after nuget restore completed, like so:

<Target Name="DoSomeExpensiveFirstTimeThing" 
        AfterTargets="AfterRestore"
        Inputs="$(MSBuildThisFileDirectory)\..\lib\ref\MyLib.dll"
        Outputs="$(MSBuildThisFileDirectory)\..\obj\**\*.cs">
…
</Target>

The benefits of this approach are many, among them:

  1. Supported in IDEs and command line restores as well
  2. No need for additional knowledge for package authors: plain MSBuild that works just as if you provided a target to run AfterBuild

The actual implementation of this could go in the main Restore targets, where it could simply be a call to:

   <MSBuild Projects="$(MSBuildProjectFullPath)" Targets="AfterRestore" Properties="ForceEval=$([System.Guid]::NewGuid().ToString())" />

where the actual AfterRestore would be declared simply as an empty target for the purposes of others to add AfterTargets or BeforeTargets to:

<Target Name="AfterRestore" />

The ForceEval property above should force MSBuild evaluation of the project again, which this time would include the targets provided by the restored nuget packages (if any), and run the AfterRestore target along with any package-provided targets that run before/after it.

Alternatively, the targets could specify AfterTargets="Restore" if the Restore target was modified to be somewhat like this:

<Target Name="Restore" DependsOnTargets="RealRestore" />

<Target Name="RealRestore" Condition="'$(FakeRestore)' != 'true'">
   …
   <MSBuild Projects="$(MSBuildProjectFullPath)" Targets="Restore" Properties="FakeRestore=true" />
</Target>

That way, the actual restore RealRestore (which would be just the current Restore renamed) wouldn't be executed at all, but all nuget-provided targets with AfterTargets=Restore would. (thanks @jonpryor for the indirect idea ;))

We could use this mechanism in Xamarin packages that need to download additional files from sources we cannot distribute with our packages, such as additional Google-provided jar files and Android SDK files, native bindings or what-not :)

kzu commented 5 years ago

/cc @bholmes @mhutch @rrelyea @jonpryor

dansiegel commented 5 years ago

I really like this... unlike the former ps1 method, this just works everywhere that msbuild works. I do however have a concern with the fact that NuGet does not respect transitive build assets. If this is added this would be yet another example for why NuGet needs to add transitive restoration of build targets

bording commented 5 years ago

@kzu Perhaps I'm missing something, but how would this be a replacement for install.ps1? Install was an action that occurred once, so install.ps1 was executed once.

With PackageReferences there is no install step, everything is a restore. Restores happen all the time, not just once. If you ran logic in your proposed AfterRestore step, it would get run much more often than the old install.ps1 files.

jainaashish commented 5 years ago

Agree with @bording this is not that simple. But we need to some replacement for install.ps1 or uninstall.ps1 for PackageReference. Until then, current workaround is to use msbuild targets to do some of those stuffs as part of build.

kzu commented 5 years ago

Heya @bording, sure thing, but that's the magic of MSBuild: a properly crafted target that has proper Inputs and Outputs defined can trivially make itself no-op and virtually free on all those subsequent restores.

bording commented 5 years ago

Heya @bording, sure thing, but that's the magic of MSBuild: a properly crafted target that has proper Inputs and Outputs defined can trivially make itself no-op and virtually free on all those subsequent restores.

That only helps for incremental builds. Any time you trigger a full build, those targets are still going to run. That means you'd still be running that logic far more often than the old install.ps1 concept, so I still don't see how this could be a viable replacement for a something that previously would have been guaranteed to run once.

mhutch commented 5 years ago

That only helps for incremental builds. Any time you trigger a full build, those targets are still going to run.

Only if their output is deleted in the Clean target :)

bording commented 5 years ago

Only if their output is deleted in the Clean target :)

True, but I'd argue that if a target is writing something to disk, then it should also be adding that output to FileWrites so it's cleaned properly as well. 😄

Regardless, even if you are taking advantage of incremental builds, relying on a target to try and replace the old install.ps1 doesn't seem like it would work. For a given project, the install action of running install.ps1 would happen once. A target will have no such guarantee, even if it avoids having its files removed in the Clean target. Nothing prevents me from manually deleting the bin/obj folders, which are the most likely place for these output files, and then the target will be run again.

An "AfterRestore" target could be useful, but it's not a replacement for the old install.ps1 behavior.

kzu commented 5 years ago

@mhutch the author might need per-package outputs that don't change at all per version (i.e. download some additional files from the web, pregenerate some bindings or MSBuild items or what-not) and in that case it would be up to the author to put those files in a place where there are not cleared, i.e. in the package install dir itself (i.e. under a .tmp/obj/whatever)

@bording forget .ps1, that's gone and never coming back. Focus on what's useful of this feature, and I'm sure regardless of what ps1 did, you can replicate the actual observable behavior with this. I challenge you to find a case where it won't ;)

bording commented 5 years ago

@bording forget .ps1, that's gone and never coming back. Focus on what's useful of this feature, and I'm sure regardless of what ps1 did, you can replicate the actual observable behavior with this. I challenge you to find a case where it won't ;)

I'm not advocating for install.ps1 coming back. Good riddance. My point is that if you had something that was previously being done in there, you had a reasonable expectation that it would be done once per package installation. Using restore as a hook for a target doesn't seem like a viable replacement for the "I want something to happen once" scenario that install.ps1 provided. Restore happens far too often in the package reference world for that to work, even if you're using Inputs and Outputs to try and have it skip the target sometimes.

kzu commented 5 years ago

Executing a target only once after a package is installed, regardless of how many times this AfterRestore is invoked is beyond trivial. I'm on mobile but I bet I can write that anyway (skim over typos):

<Target Name="Install" AfterTargets="AfterRestore" Inputs="$(MSBuildThisFileFullPath)" Outputs="$(MSBuildThisFileDirectory)/install.stamp">
  // Do you one time ever thing, then
  <WriteLinesToFile File="$(MSBuildThisFileDirectory)/install.stamp" />
</Target>

That's really all there is to it. Trivial. And you can also do much more beyond the "on install only", you can also do the equivalent of init.ps1 too by emitting something that inputs/outputs to the consuming project intermediate directory instead. Bingo! ;)

bording commented 5 years ago

@kzu That means you're writing a file into the global package folder, which could be wiped on a system as well, and then that target will be run again.

Or if you're running on a CI system that has as clean package folder for every run, again that target will be run again.

Even worse, you've now introduced a file into a global location that would be shared by every project that uses that package. That means that you could be running that target once for the system. and not once per project.

kzu commented 5 years ago

And what's the scenario where that would be a problem? If the system is wiped, then you most certainly want a "run on install" thing to run again, no?

If you want to run once per project, that's easily achievable by writing to the project intermediate output too (with another target).

I still don't see the problem. And you're forgetting that the target has the full .NET and cmdline capabilities that any powershell had too, so you can run whatever checks you want to determine the need to run something again (i.e. reading the registry, running msiexec, or bash or whatever).