search

fsprojects / Paket

A dependency manager for .NET with support for NuGet packages and Git repositories.
https://fsprojects.github.io/Paket/
MIT License
1.99k stars 520 forks source link

Importing custom targets does not work with .NET Core & Paket 4.X #2190

Closed cjbhaines closed 6 years ago

cjbhaines commented 7 years ago

Description

We have a NuGet package with a build target inside it which we use to enforce our "build police" (checking for things like namespace prefix and other things). I can see that paket is not adding the targets reference into the csproj file.

paket.dependencies:

source http://nuget.geniussports.net/NugetWebFeed/nuget

nuget GeniusSports.Common.Targets import_targets: true

nuget System.Net.NetworkInformation ~> 4.0
nuget Newtonsoft.Json ~> 9.0

paket.references

GeniusSports.Common.Targets import_targets: true

Newtonsoft.Json
System.Net.NetworkInformation

Repro steps

Please provide the steps required to reproduce the problem

See attached ZIP. I have also attached our target nuget package.

PaketRepro.zip

GeniusSports.Common.Targets.1.0.2.nupkg.zip

Expected behavior

This is added to the csproj:

  <Import Project="..\packages\MSBuildTasks\build\MSBuildTasks.targets" />
  <Import Project="..\packages\GeniusSports.Common.Targets\build\GeniusSports.Common.targets" />

Actual behavior

No build targets are added.

forki commented 7 years ago

did that work with odler versions?

cjbhaines commented 7 years ago

Interestingly no. So I've checked this on a normal .NET 4.6.2 project and it does work:

<Import Project="..\..\..\packages\GeniusSports.Common.Targets\build\GeniusSports.Common.targets" Condition="Exists('..\..\..\packages\GeniusSports.Common.Targets\build\GeniusSports.Common.targets')" Label="Paket" />
  <Import Project="..\..\..\packages\MSBuildTasks\build\MSBuildTasks.targets" Condition="Exists('..\..\..\packages\MSBuildTasks\build\MSBuildTasks.targets')" Label="Paket" />

Doesn't work on any of my .NET Standard/Core projects.

forki commented 7 years ago

Ah so you want these target's installed into MSBuild 15 csproj?

cjbhaines commented 7 years ago

I think so. Interestingly they don't work if I add the reference to them manually anyway. Has the format and process of using targets changed in MSBuild15?

We have been using these successfully with Paket 3.X, VS2015 and .NET 4.6.2 projects for some time now.

Disclaimer that I didn't write these build targets, I'm not a user. I never actually checked they worked once we started developing with .NET Standard/Core, just made an assumption they would.

forki commented 7 years ago

@cjbhaines I have no idea you would need to ask the MSbuild people /shrug

cjbhaines commented 7 years ago

Ok let me see if we can get our targets working with a manual reference. If so it would be good if targets could be included in .NET Standard/Core projects like they do for .NET 4.6.2. I will report back when I know more. Cheers!

forki commented 7 years ago

please let me know when we need to do something in paket, but right now I don't really know

SavaNDragos commented 7 years ago

Hi guys, I looked over this issue and I can confirm the target files should be imported. https://docs.microsoft.com/en-us/dotnet/articles/core/tools/cli-msbuild-architecture (you can see here under (With the move to the new project system, the previous diagram changes: ) that they should work.

SavaNDragos commented 7 years ago

Also targets can work on .net framework or .net core.

cjbhaines commented 7 years ago

@forki do you think it's a big job to add the target imports for the new csproj format / .NET Standard & .NET Core projects?

forki commented 7 years ago

It's probably not too hard. Will take a look next week when I'm back from vacation.

Am 20.03.2017 14:39 schrieb "Chris Haines" notifications@github.com:

@forki https://github.com/forki do you think it's a big job to add the target imports for the new csproj format / .NET Standard & .NET Core projects?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsprojects/Paket/issues/2190#issuecomment-287779228, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNCxnN7M0xEoF37iyizN81sPLEnVeks5rno-vgaJpZM4MfTV5 .

cjbhaines commented 7 years ago

Thanks mate, much appreciated!

forki commented 7 years ago

OK. What I need is a repro with a project that was already converted to paket and then a description of the diff you want to see.

cloudRoutine commented 7 years ago

Fixing this and fixing how paket doesn't pull in .props files properly across dependencies will give us everything we need to generate graph json nuget.g.props and nuget.g.targets for restore.

Then maybe we could use the paket.lock to populate the assets.json and we'd have everything we need to do restore on our own and be able to run msbuild afterward.

forki commented 7 years ago

As I said: I'd appreciate a full repro

cjbhaines commented 7 years ago

@forki I'll try and get this done over the next couple of days, it's not a straight forward repo to produce because I need a publicly available package with targets. Can you think of any packages on the official NuGet feed which contain targets?

forki commented 7 years ago

no I don't know any. maybe you can ask on twitter?

cloudRoutine commented 7 years ago

@cjbhaines Microsoft.VSSDK.BuildTools FSharp.NET.Sdk

SavaNDragos commented 7 years ago

Another one https://www.nuget.org/packages/MSBuildTasks/1.5.0.235

0x53A commented 7 years ago

Microsoft.Net.Compilers / FSharp.Compiler.Tools both include .props to override the current compiler.

Fody also includes a .targets file.

I think the difference is that .props should be included at the top of the project file, and .targets at the end.

At least that is what nuget does, whatever much that is worth.

cjbhaines commented 7 years ago

I have prepared a repro for you. Inside the repro you will find 2 solutions in folders called VS2015 and VS2017. Both solutions have been prepared from scratch (no VS2017 automatic upgrade process). There are various different frameworks included under each solution, with the results below.

Bug 2190 Repo.zip

Paket version: 4.1.5

The paket dependencies file contains 2 packages containing build targets:

source https://www.nuget.org/api/v2

nuget Microsoft.VSSDK.BuildTools
nuget MSBuildTasks

The paket references file in each project references these packages with the import_targets option set to true:

Microsoft.VSSDK.BuildTools import_targets: true
nuget MSBuildTasks import_targets: true

VS2015:

VS2017:

Expected results: All projects under the VS2017 solution have build targets referened. Actual results: DotNetStandard and DotNetCore do not get build targets referenced.

The build targets import should look like this, if comparing from a successful import:

<Import Project="..\..\packages\Microsoft.VSSDK.BuildTools\build\Microsoft.VSSDK.BuildTools.targets" Condition="Exists('..\..\packages\Microsoft.VSSDK.BuildTools\build\Microsoft.VSSDK.BuildTools.targets')" Label="Paket" />

<Import Project="..\..\packages\MSBuildTasks\build\MSBuildTasks.targets" Condition="Exists('..\..\packages\MSBuildTasks\build\MSBuildTasks.targets')" Label="Paket" />
cloudRoutine commented 7 years ago

@cjbhaines have you tried running dotnet restore for the vs2017 sln on the commandline first and then trying to build it with msbuild15?

(the repro is referencing a bunch of targets from vs packages I don't have installed (UWP, xaml stuff) so I can't properly test this out myself atm)

cjbhaines commented 7 years ago

@cloudRoutine that wasn't the point of the repo. We have some custom build targets we distribute internally but I obviously can't reference those in a public repro. Those are not being included which is why this bug was opened. I just couldn't think of any projects apart from VS build tools which have custom build targets on NuGet which is why those are used, so this is not to be confused with a restore or build issue. You don't actually need to build this to see the packet repro, it's about update/install not adding build targets to the csproj files.

cloudRoutine commented 7 years ago

I asked because the information would be useful for fixing this and other related issues, it wasn't a "just do this instead" suggestion

cjbhaines commented 7 years ago

I doubled checked this against Paket 5.0.0-beta003 and it is still an issue.

@cloudRoutine everything seems to build fine in 2015 and 2017 after running dotnet restore, however the imports are still not added on a paket install. I have reduced the size of the repro to just have .NET, Standard and Core so you can open it.

Bug.2190.Repro.SimplifiedWithoutUWP.zip

For .NET 4.6.2, the requested imports in the paket.references:

Microsoft.VSSDK.BuildTools import_targets: true
MSBuildTasks import_targets: true

Resulting in the following being added to the .NET 4.6.2 csproj:

  <Import Project="..\..\packages\Microsoft.VSSDK.BuildTools\build\Microsoft.VSSDK.BuildTools.targets" Condition="Exists('..\..\packages\Microsoft.VSSDK.BuildTools\build\Microsoft.VSSDK.BuildTools.targets')" Label="Paket" />
  <Import Project="..\..\packages\MSBuildTasks\build\MSBuildTasks.targets" Condition="Exists('..\..\packages\MSBuildTasks\build\MSBuildTasks.targets')" Label="Paket" />

The same requested imports for the .NET Standard and Core projects do not get imported from the package when paket install is called. This predates the new csproj format and looks to have been a long standing issue.

I guess the Paket logic to add custom imports is only enabled for specific runtimes? As it's working for regular .NET and UWP but not for .NET Standard and Core.

matthid commented 7 years ago

Does nuget add the imports? Because in netcore world the props and target files are just copied to the obj dir and picked up by msbuild15 automatically. No Import needed. That's why you need to call restore before it works after all.

cjbhaines commented 7 years ago

Ah sorry I didn't realise that, I thought they had just move the references into obj. I can now see this in my obj g.targets file:

<?xml version="1.0" encoding="utf-8" standalone="no"?>
<Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <PropertyGroup>
    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
  </PropertyGroup>
  <ImportGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <Import Project="$(NuGetPackageRoot)msbuildtasks\1.5.0.235\build\MSBuildTasks.targets" Condition="Exists('$(NuGetPackageRoot)msbuildtasks\1.5.0.235\build\MSBuildTasks.targets')" />
    <Import Project="$(NuGetPackageRoot)microsoft.vssdk.buildtools\15.0.26201\build\Microsoft.VSSDK.BuildTools.targets" Condition="Exists('$(NuGetPackageRoot)microsoft.vssdk.buildtools\15.0.26201\build\Microsoft.VSSDK.BuildTools.targets')" />
  </ImportGroup>
</Project>

So that has imported the MS packages fine. Interestingly I still can't get it to import my custom targets:

paket.dependencies:

source http://nuget.geniussports.net/NugetWebFeed/nuget

nuget GeniusSports.Common.Targets

source https://www.nuget.org/api/v2

nuget Microsoft.VSSDK.BuildTools
nuget MSBuildTasks

paket.references:

GeniusSports.Common.Targets import_targets: true
Microsoft.VSSDK.BuildTools import_targets: true
MSBuildTasks import_targets: true

Comparing my package to the MSBuildTasks packages it looks like everything is fine:

geniussports common targets

msbuildtasks

I have tried:

All without luck. I can see that the dependency has been added to the obj references file:

GeniusSports.Common.Targets,2.0.0,Direct
Microsoft.VSSDK.BuildTools,15.0.26201,Direct
MSBuildTasks,1.5.0.235,Direct

But nothing has been added to the g.targets file for the Genius Sports targets. Hopefully I'm not doing something silly here.

I have attached our targets package for reference as well.

GeniusSports.Common.Targets.2.0.0.nupkg.zip

0x53A commented 7 years ago

Hehehe, the filename Must be <packageid>.targets. It looks like your package has the id GeniusSports.Common.Targets so the file should be named /build/GeniusSports.Common.Targets.targets.

cjbhaines commented 7 years ago

and queue the "it was the being silly part" music! :) Thanks for your help! I will test this when I'm back in the office on Monday and report back