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

ExcludeRestorePackageImports in nuget.g.targets #4013

Closed
aL3891 opened this issue Nov 26, 2016 · 13 comments
Closed

ExcludeRestorePackageImports in nuget.g.targets #4013

aL3891 opened this issue Nov 26, 2016 · 13 comments

Comments

@aL3891
Copy link

aL3891 commented Nov 26, 2016

Hello!
i'd like to run a target in my nuget package during restore but it seems like the ExcludeRestorePackageImports parameter is set to true so my target doesn't run. That condition comes from here as far as I understand:

https://github.com/NuGet/NuGet.Client/blob/27645cbb0be2c84dd047c8287338920bd273688f/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/BuildAssetsUtils.cs#L164

What is the reason for not allowing nuget targets to run during restore? Each of the import already has an exist condition so as I understand it, no errors would happen if a target wasn't already there. Is there another way to get around it?

Now I know what you're thinking, if you have a package with a targets file that you'd like to run on restore, you'd have to restore twice to get it to run. That is a bit inconvenient, but it still worth it in my opinion because it enables some interesting scenarios, in my case, enabling defining packages for a project in alternate ways

@emgarten
Copy link
Member

enabling defining packages for a project in alternate ways

Package references from packages are the reason targets are excluded. Would you share your scenario and what you are attempting do with dependencies beyond the current target framework selection done today?

Restore would need to loop indefinitely and re-evaluate all of the MSBuild targets and projects each time to check for new dependencies. It would lead to a much slower restore and right now isn't possible for most restore scenarios.

Original issue: #3604

@aL3891
Copy link
Author

aL3891 commented Nov 28, 2016

Basically I've got a package that will allow people to use a project.json file if they want to, or specify references in other ways.

My project is open source, i'd love feedback if you think there is a better way to do this :)
https://github.com/aL3891/CustomPackageReferences

I think it would be fine to just evaluate the targets once (and run restore multiple times in the rare case it was needed), not for each added package, at least for my scenario. Another option would be to somehow tell the system that a particular set of targets would be for restore. Today there is a build in the packages that is auto included, but what if there was a restore folder for targets that should be included in restore?

@aL3891
Copy link
Author

aL3891 commented Nov 28, 2016

I reviewed the original issue and agree it can seem a little odd that you have to restore twice, but on the other had I think there is a lot of value in enabling different ways to define packages, it's at the very least something that people is very passionate about :)

I personally actually don't mind msbuild, especially the new version, but some people do and why close the door on having this kind of extensibility? I think a lot of people will find it worth it to do two restores if it means they can define packages the way they want.

Would it really have to loop indefinitely though? it would only have to loop again if a packagereference got added right? I understand that this would add a lot of extra logic, i'm not even sure you can "go back" to a previous target in msbuild but i'm no expert

I would love more perspectives on this though :)

@emgarten
Copy link
Member

I really like your idea of being able to specify references in other formats in CustomPackageReferences. My suggestion is to create a tool that generates a targets file containing the PackageReferences, and run it before NuGet restore.

Projects can import the targets generated by your tool. Note that this wouldn't work well with the VS UI or other NuGet tools that aren't expecting this, but it could allow you to get this working for command line scenarios.

As for double restoring, restore needs to know the top level references up front, I won't get into all the edge cases here, but if those were able to change each time a restore happened it would make resolving much slower and difficult for users to debug when trying to track down why a certain package was brought in.

@aL3891
Copy link
Author

aL3891 commented Nov 28, 2016

Interesting, could that still be delivered as a nuget package? Also, how would I hook that targets file in, should the tool put it in the main proj file? Ideally i'd want this process to be automatic but I suppose the flow could be

dotnet restore
dotnet importcustomPackageReferences
dotnet restore
dotnet run

or is there a better way? i'm still getting to know the new msbuild so i'm sorry if this is obvious stuff :)

@emgarten
Copy link
Member

With nuget.exe you could use install to extract the package to a folder and reference the tool from there. There isn't an equivalent command in dotnet for that yet.

I hadn't thought of trying to make a tool package for this, is that your intention with dotnet importcustomPackageReferences?

Right now there isn't a way to restore only tools, so you would need to follow the order you described.

The target auto import hook can be used by dropping a targets file in obj with a name matching this format:
https://github.com/Microsoft/msbuild/blob/862ce619a23877cbfad54fd4acd547ef92f499b3/src/XMakeTasks/Microsoft.Common.targets#L117-L118

@aL3891
Copy link
Author

aL3891 commented Nov 29, 2016

Right, I was under the impression that "installing" packages was a concept that was going away? Perhaps its just not supported yet

Yes, when you said "tool" my mind jumped to a tool package :) After your post I also got to thinking about a "custom tool" tool but as I understand it, that's a visual studio only thing.. it would be a nice thing to have though

Aha that's very interesting! so if I just make a targets/props file called <projectname>.csproj.whatever.targets it will get imported automatically?

-edit-

oh sorry I misunderstood what you ment by install, you mean something like what chocolatey does right? like installing it as a separate thing that is later run. What I ment was the way you could run a script when the user installs your package

@emgarten
Copy link
Member

you mean something like what chocolatey does right?

I just mean get the binary on the machine any way you can.

As a tool package this would need to be installed into each project. NuGet.exe on the other hand is typically downloaded before restore and then it runs over the entire solution.

A tool package would run cross platform, to run cross platform outside of a project you may need to restore a fake project referencing your tool to get everything in place.

@aL3891
Copy link
Author

aL3891 commented Nov 29, 2016

Right, ideally i'd like the process to be a little more automatic, I think it would be fine to add a reference to each project, my main problem right now is the bootstrapping of the target into the project, but a package tool might just be the way to go ´InitializeProjectJson´ or something like that. it could then add a target.

I know you said you didn't want to go into specifics, but regarding generating a targets file with PackageReferences. Is there a difference between pregenerating a targets file rather than adding the PackageReference items during the restore like I do now? As long as either target runs before the nugget targets, nuget wont be able to tell the difference will it?

@emgarten
Copy link
Member

NuGet restore has this flow today:

  1. Gather all package reference information from MSBuild
  2. Gather all project to project reference information from MSBuild
  3. Exit MSBuild (New targets cannot be dynamically added in and reevaluated to add new references)
  4. Walk project dependencies and pick the best match for each
  5. Download packages
  6. Write out restore information

For large projects evaluating all of the project to project references and package references can take a significant amount of time. If NuGet were to allow the possibility of new references from packages it just downloaded it would need to always re-evaluate everything to check if this happened, then resolve everything again. At the end it would need to re-evaluate another time incase more packages were added, and this could continue indefinitely.

Packages could also end up adding in references that conflicted with references added by the user. For example if the user has a reference to PackageA 2.0.0 and another package contains a reference to PackageA 1.0.0. There isn't a good way for NuGet to reason about this and it puts the user in a situation where they need to track down why they are getting errors about duplicate references, and then they need to exclude the package or fix this somehow.

Beyond that there are also scenarios that could happen such as
PackageD 1.0.0 -> PackageX 1.0.0 -> PackageY 1.0.0 (targets file ->) PackageX 2.0.0 -> PackageA 1.0.0

Problem 1: Here this is a circular dependency, but NuGet is unable to see it because it is not defined in the nuspec, and the package resolver cannot just load up MSBuild and reevaluate everything again for each package to find out.

Problem 2: On the second restore PackageX 2.0.0 will now be a top level reference and will override the older version. This means that PackageY will no longer be used. Since PackageY is gone the reference to PackageX 2.0.0 is now gone and NuGet will need to restore again and get into an infinite loop.

If you generate the targets before restore

  • You avoid multiple restores and infinite loops
  • Reference problems are easier to debug since it is all available in the targets file

@aL3891
Copy link
Author

aL3891 commented Nov 30, 2016

Interesting, thanks for the in depth answer!

I agree that doing this in a single restore pass would potentially get very difficult and slow, but if we accept that, are the problems you mention (debugging aside, I agree there) resolved by pregenerating the targets file?

As I see it in both cases you'd initially have to restore twice,
in the first case the flow would then be

dotnet restore
dotnet customreferencestool < targets file generated, manually run each time references change
dotnet restore

If instead you'd put a custom task in a target and add that to the main proj file (like I do manually today) you'd have

dotnet restore
dotnet initializecustomreference < done once to add target to main proj file
dotnet restore < custom task adds project reference items on the fly based on its own logic, such as reading project.json

As I understand, in both cases there could potentially override the users packages, or rather they'd be able to specify conflicting/circular references and that would cause problems, but if a Task reads the project.json and emits ProjectReference items you could skip the step of running the tool each time project.json was updated, but as you say, debugging package problems would be harder.

In a perfect world you'd be able to specify a custom tool for the package file and have that generate the targets file on save, that'd be the best of both worlds :) but afaik that's not supported

@MarkKharitonov
Copy link

MarkKharitonov commented Dec 7, 2018

Gentlemen, how about the following scenario - I am writing UI tests that need chromedriver.exe to run. This is a run-time dependency that must be placed into the bin folder. The way to do it seems to import the nuget package Selenium.WebDriver.ChromeDriver. This package contains an import targets file that makes sure the chromedriver.exe is published or copied to the bin folder.

I am importing the package using PackageReference, but since the import targets is ignored the chromedriver.exe is not published/copied/whatever.

What am I supposed to do in this situation?

@MarkKharitonov
Copy link

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

No branches or pull requests

5 participants