-
Notifications
You must be signed in to change notification settings - Fork 258
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
Allow package authors to provide AfterRestore behavior via MSBuild #7617
Comments
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 |
@kzu Perhaps I'm missing something, but how would this be a replacement for 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 |
Agree with @bording this is not that simple. But we need to some replacement for |
Heya @bording, sure thing, but that's the magic of MSBuild: a properly crafted target that has proper |
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 |
Only if their output is deleted in the |
True, but I'd argue that if a target is writing something to disk, then it should also be adding that output to Regardless, even if you are taking advantage of incremental builds, relying on a target to try and replace the old An "AfterRestore" target could be useful, but it's not a replacement for the old |
@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 ;) |
I'm not advocating for |
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):
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! ;) |
@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. |
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). |
When
PackageReference
came along and dropped support forinstall.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 someobj
-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:
The benefits of this approach are many, among them:
AfterBuild
The actual implementation of this could go in the main Restore targets, where it could simply be a call to:
where the actual
AfterRestore
would be declared simply as an empty target for the purposes of others to addAfterTargets
orBeforeTargets
to: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 theAfterRestore
target along with any package-provided targets that run before/after it.Alternatively, the targets could specify
AfterTargets="Restore"
if theRestore
target was modified to be somewhat like this:That way, the actual restore
RealRestore
(which would be just the currentRestore
renamed) wouldn't be executed at all, but all nuget-provided targets withAfterTargets=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 :)
The text was updated successfully, but these errors were encountered: