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

Order of import has changed in NuGet 2 / 3 / 4 #4229

Closed
ericstj opened this issue Jan 7, 2017 · 7 comments
Closed

Order of import has changed in NuGet 2 / 3 / 4 #4229

ericstj opened this issue Jan 7, 2017 · 7 comments
Labels
Resolution:ByDesign This issue appears to be ByDesign
Milestone

Comments

@ericstj
Copy link

ericstj commented Jan 7, 2017

I'm not sure how actionable this is, but I noticed it today as a breaking change and wanted to make you aware.

In NuGet2 package targets are imported at the end of the project file by NuGet writing them to the targets file based on the order of package install (?)

In NuGet3 package targets are imported near the end of Microsoft.Common.targets via ImportAfter hook. Order of import of after hook is file-system-dependent (MSBuild evaluating the *) then order of packages in NuGet's generated project-specific targets files appears to be alphabetical (?)

In NuGet4 package targets are imported via a new hook, MSBuildProjectExtensionsPath, which occurs before ImportAfter. This hook has the same order as ImportAfter due to use of MSBuild's wildcard, and likely the same order in NuGet's generated project-specific targets file.

Order of targets is a big deal and can make some things impossible to do if you don't get imported late enough. Consider a change for NuGet v4 to move later in the evaluation rather than earlier. Of course this would require a change in MSBuild, but it seems you already have a change in MSBuild to move you earlier, perhaps just put that later instead 😉

/cc @jeffkl @emgarten

@jeffkl
Copy link
Contributor

jeffkl commented Jan 9, 2017

We debated for a while about where the import should occur. We settled on this order because it ensures that your local environment gets to set things first and last.

Technically the NuGet creates things imported via MSBuildProjectExtensionsPath (via a wildcard) and it can control the order of imports within its generated file. I'm not sure what the order is but I did get the impression that it's alphabetic.

We are pushing teams to use NuGet packages and steering them away from locally installed SDKs which means the ImportBefore/ImportAfter folder location hook would ideally be deprecated. We will rely on NuGet to get the order correct for packages. They might need to be imported based on some sort of dependency chain or the order specified in the package config.

@emgarten
Copy link
Member

emgarten commented Jan 9, 2017

On NuGet's ordering of the imports:

  1. NuGet 2.8.x would write them in install order, but when updating packages NuGet this could get out of sync. The client wasn't actually reading what was already in the file to decide where the new targets should go.
  2. During 3.x XProj didn't support targets/props so this area was not focused on.
  3. 4.x ensures everything is in dependency order and has the right behavior now.

To make this easier for package authors in the future NuGet now writes out the tooling version and the project type (packages.config vs project.json vs PackageReference) to the generated .props file so that if needed a package can determine if it needs to work around past problems or error out with a helpful message.

@ericstj
Copy link
Author

ericstj commented Jan 13, 2017

What are your thoughts about making them happen after SDK imports? The nice characteristic about 2.x was that you could override anything you wanted.

@jeffkl
Copy link
Contributor

jeffkl commented Jan 13, 2017

The SDK is what imports the NuGet props/targets.

https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.props#L19
https://github.com/Microsoft/msbuild/blob/xplat/src/XMakeTasks/Microsoft.Common.props#L59

https://github.com/dotnet/sdk/blob/master/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets#L40
https://github.com/Microsoft/msbuild/blob/xplat/src/XMakeTasks/Microsoft.Common.targets#L127

The imports happen fairly early and late but just after and before what is next to the proejcts. We believe NuGet packages should still be able to extend the build, are there cases where this isn't possible?

@natemcmaster
Copy link

It's possible to extend the build using current import order, but it's not possible to override some parts of the build.

For example, if I wanted create a NuGet package that ships a target called "Pack", this cannot override the pack that is in Microsoft.NET.Sdk because Microsoft.NET.Sdk/Sdk/Sdk.targets imports the pack targets at the very end, after MSBuildProjectExtensionsPath targets are imported.

@jeffkl
Copy link
Contributor

jeffkl commented Jan 13, 2017

The only way an SDK could allow someone else to override its targets would be to expose a property like CustomAfterSdkTargets that it would import at the end of it. Technically it could do an import of a place on disk as well but we're trying to move away from that.

Another established pattern is to use a DependsOn property or BeforeTargets/AfterTargets to extend a target.

+@rainersigwald

@zhili1208 zhili1208 added the Resolution:ByDesign This issue appears to be ByDesign label Oct 17, 2017
@zhili1208 zhili1208 added this to the 4.5 milestone Oct 17, 2017
@zhili1208
Copy link
Contributor

No actionable here, close it

amaitland added a commit to cefsharp/CefSharp that referenced this issue Jan 23, 2018
Resolves #2156

Nuget packages updated to use ItemGroup None instead of Copy target
Nuget props restructure to support Win32 target
x86 and Win32 targets are exactly the same in content, they differ in name only, so switched to using an <Otherwise/> block
Remove AnyCpu <When/> option from props files and consolidate with x86 and Win32. Set Private to false and rely on the
<None/> entires to copy the files to the output folder

Ideally would move the OffScreen, WinForms and WPF targets file content to the props (as they're actually props now), unfortunately
we need guaranteed ordering and Nuget 2.8.x doesn't provide that, if we leave them as .targets it'll happen late enough to make sure
the cef.redist list of files is provided.
NuGet/Home#4229 (comment)
amaitland added a commit to cefsharp/CefSharp that referenced this issue Jan 23, 2018
Resolves #2156

Nuget packages updated to use ItemGroup None instead of Copy target
Nuget props restructure to support Win32 target
x86 and Win32 targets are exactly the same in content, they differ in name only, so switched to using an <Otherwise/> block
Remove AnyCpu <When/> option from props files and consolidate with x86 and Win32. Set Private to false and rely on the
<None/> entires to copy the files to the output folder

Ideally would move the OffScreen, WinForms and WPF targets file content to the props (as they're actually props now), unfortunately
we need guaranteed ordering and Nuget 2.8.x doesn't provide that, if we leave them as .targets it'll happen late enough to make sure
the cef.redist list of files is provided.
NuGet/Home#4229 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution:ByDesign This issue appears to be ByDesign
Projects
None yet
Development

No branches or pull requests

5 participants