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

[Bug]: props and targets files are not imported from build folder of transitive dependencies if custom platform name is used #11253

Closed
sigurn opened this issue Sep 21, 2021 · 18 comments
Labels
Resolution:NotABug This issue appears to not be a bug Resolution:Question This issues appears to be a question, not a product defect Type:Bug

Comments

@sigurn
Copy link

sigurn commented Sep 21, 2021

NuGet Product Used

MSBuild.exe

Product Version

MSBuild: 16.11.0.36601, NuGet: 5.11.0

Worked before?

MSBuild: 15.9.21.664, NuGet: 4.9.3

Impact

I'm unable to use this version

Repro Steps & Context

  1. MSBuild project SomeProject.proj with Microsoft.NET.Sdk
  2. Project has a package reference for PackageA
  3. PackageA has dependency group with targetFramework=".NETFramework4.7.2-bla"; PackageB is a dependency in this dependency group
  4. PackageA has file build\PackageA.props; PackageB has file build\PackageB.props
  5. MSBuild project SomeProject.proj has TargetFramework=net472-bla
  6. Run msbuild.exe /nologo /t:Restore SomeProject.proj

I expect:
SomeProject.nuget.g.props file has imports of props files from both packages:

...
  <ImportGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <Import Project="$(NuGetPackageRoot)PackageB\<Version>\build\PackageB.props" Condition="Exists('$(NuGetPackageRoot)PackageB\<Version>\build\PackageB.props')" />
    <Import Project="$(NuGetPackageRoot)PackageA\<Version>\build\PackageA.props" Condition="Exists('$(NuGetPackageRoot)PackageA\<Version>\build\PackageA.props')" />
  </ImportGroup>
...

In fact:
SomeProject.nuget.g.props file has imports from PackageA only:

...
<ImportGroup Condition=" '$(ExcludeRestorePackageImports)' != 'true' ">
    <Import Project="$(NuGetPackageRoot)PackageA\<Version>\build\PackageA.props" Condition="Exists('$(NuGetPackageRoot)PackageA\<Version>\build\PackageA.props')" />
  </ImportGroup>
...

Verbose Logs

No response

@nkolev92
Copy link
Member

Hey @sigurn,

The build folder is not transitive by default, please see: https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#controlling-dependency-assets.
Build targets can be hard to write when they are applied transitively.

What do you actually need from the build targets for B?
That might help answer the question on how to handle your issue.

You've got options for sure.

  1. If you control packageB, you could consider a buildTransitive folder.
  2. If you control packageA, you could tweak the PrivateAssets value to include the packageB targets transitively. Note that if you create a new package/project, it will allow whatever ref is packageA being consumed through.
  3. Consider whether you really need the build targets of packageB. Build targets tend to widen the compatibility matrix of packages and may sometimes cause bad uninstalls (build/bla.targets effectively makes the package compatible with a lot more, consider build\tfm\bla.targets instead).

Closing this given that it's really a question not a bug, but I'd be happy to help you narrow down your issue.

@nkolev92 nkolev92 added Resolution:NotABug This issue appears to not be a bug Resolution:Question This issues appears to be a question, not a product defect labels Sep 21, 2021
@sigurn
Copy link
Author

sigurn commented Sep 22, 2021

Thank you very much for the fast reply.

The main problem here is that backward compatibility is broken between NuGet versions. Switching the same project to new MSBuild/NuGet version makes it unbuildable.

Build targets from B contains scripts for installer like

<ItemGroup>
  <WixFile Include="$(MSBuildThisFileDirectory)..\install\bla.wxi" />
</ItemGroup>
  1. I control PackageB and that means that to switch from VS2017 to VS2019 version I need to change all 300 packages to move all props files from build folder to buildTransitive.
  2. I control PackageA and it already have <PackageReference Include="PackageB" Version="1.*" PrivateAssets="none" />
  3. Yes I am aware of wide compatibility matrix but it is not and issue in this case.

I considered it as a bug for several reasons:

  1. Given that the same config worked in VS2017 - backward compatibility is broken. It would be nice to get old behavior back with some flags or so.
  2. Changing TargetFramework from net472-bla to net472 solves the problem so behavior differs just by changing Platform part of TargetFramework.
  3. I've already tried moving files to buildTransitive folder for one module but that didn't solve the problem. So both folders (build and buildTransitive) work the same for net472 TargetFramework and both don't work for net472-bla. It looks like the problem is in how Platform part of TargetFramework is handled.

@sigurn
Copy link
Author

sigurn commented Sep 29, 2021

@nkolev92 Do you have any ideas, suggestions?

@nkolev92
Copy link
Member

Oh my bad. Apparently comments on closed issues where I'm not tagged end up in a different folder.

Not sure what you mean by platform here. When you put something after net472, that makes it a profile, which means more specific than net472.

I control PackageB and that means that to switch from VS2017 to VS2019 version I need to change all 300 packages to move all props files from build folder to buildTransitive.

Why do you think you need to do this?
VS2019 will work the same way as VS2017. buildTransitive is supposed to be an easier way of doing it and it's only available in VS2019.

Given that the same config worked in VS2017 - backward compatibility is broken. It would be nice to get old behavior back with some flags or so.

Can you help me understand what in particular doesn't work in VS 2019, but works in VS2017?
Note that buildTransitive is only available in VS2019.

Changing TargetFramework from net472-bla to net472 solves the problem so behavior differs just by changing Platform part of TargetFramework.

Same question as above applied.
Maybe you can zip up something to clearly communicate the repro to any mistakes on our end attempting your repro.

I've already tried moving files to buildTransitive folder for one module but that didn't solve the problem. So both folders (build and buildTransitive) work the same for net472 TargetFramework and both don't work for net472-bla. It looks like the problem is in how Platform part of TargetFramework is handled.

I think a repro would be great!

Thanks!

@sigurn
Copy link
Author

sigurn commented Oct 4, 2021

@nkolev92 here is repository which reproduces the issue

@sigurn
Copy link
Author

sigurn commented Oct 11, 2021

Hi @nkolev92! Did the project help to understand the issue? Any ideas how to solve the issue?

@zivkan
Copy link
Member

zivkan commented Oct 11, 2021

In my opinion, this is by design.

With .NET 5, the Target Framework specification changed, in particular, there's now a net5.0-windows Target Framework, which has an implied version of Windows when missing, but can also be explicitly specified, for example net5.0-windows10.0.19041.0.

In versions of NuGet before we added support for net5.0, NuGet had a long-standing bug where we would parse the TargetFramework MSBuild property, and use that for NuGet's compatibility checking, even when it was different to what target framework the .NET SDK, the compiler, the project system, and any other parts of the build would use. In order to support the implicit platform version, we had to change NuGet (version 5.8, from memory, which corresponds to Visual Studio 2019 16.8) to read the MSBuild properties TargetFrameworkIdentifier, TargetFrameworkVersion, TargetPlatformIdentifier and TargetPlatformVersion.

I believe this explains why you're seeing the net472-test dependency in Visual Studio 2017 (Nuget 4.9), but not in Visual Studio 2019 (NuGet 5.11). While this is a change in behaviour, something that used to work for you is no longer working, I don't believe that net472-bla or net472-test are valid. In fact, I don't believe that the .NET Framework Targeting Pack or .NET SDK support any net472-* target framework (TargetFrameworkMoniker=.NETFramework,Version=4.7.2,ClientProfile=????). If you can help us understand how and/or why you're using this, we can consider whether to re-introduce this capability, but since it's not an official target framework, I don't believe it's in scope to be supported.

@sigurn
Copy link
Author

sigurn commented Oct 12, 2021

@zivkan We have a product which consists of many modules (every module is a NuGet package). So there are dependencies which are needed just in build time, there are dependencies which are needed in runtime and there are dependencies which we need to build deployment packages. Every module (NuGet package) has pieces for deployment and there is separate list of dependencies which are needed for deployment package (i.e. MSI). There are actually several deployment packages there are standalone installers, update packages, etc. So we use such TargetFrameworks for managing those dependencies.

@sigurn
Copy link
Author

sigurn commented Oct 15, 2021

@zivkan any ideas how to solve this problem?

@sigurn
Copy link
Author

sigurn commented Oct 15, 2021

I am OK with using something like net5.0-test1.0 but that thing doesn't work either because of this check in Microsoft.NET.TargetFrameworkInference.targets file:

<Target Name="_CheckForUnsupportedTargetPlatformIdentifier"
          BeforeTargets="_CheckForInvalidConfigurationAndPlatform;RunResolvePackageDependencies;GetFrameworkPaths;GetReferenceAssemblyPaths;CollectPackageReferences"
          Condition="'$(TargetPlatformIdentifier)' != '' and '$(TargetFrameworkIdentifier)' == '.NETCoreApp' and $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), 5.0))" >

    <PropertyGroup>
      <TargetPlatformSupported Condition="'$(TargetPlatformIdentifier)' == 'Windows'">true</TargetPlatformSupported>
    </PropertyGroup>

    <NETSdkError Condition="'$(TargetPlatformSupported)' != 'true'"
                 ResourceName="UnsupportedTargetPlatformIdentifier"
                 FormatArguments="$(TargetPlatformIdentifier)" />
  </Target>

@zivkan
Copy link
Member

zivkan commented Oct 15, 2021

I'm not sure this would achieve what you would want it to, but you could upvote this issue: #7900. Or maybe this issue: #5154

For a package where you only want for compile not runtime (copied to bin folder), you can use <PackageReference Include="packageId" version="1.2.3" ExcludeAssets="runtime" />. Similarly, if you only want the runtime files, not compile, ExcludeAssets="compile". Our docs list the available asset groups.

If you control the packages themselves, you can author them differently, so this behavior is automatic, rather then requiring the package consumer to do the work. Packages that contain compile-time only assets, should have them in ref/net472/your.dll, nothing under lib/. Packages that are runtime only can put the assemblies in runtimes/any/lib/net472/your.dll. It might be necessary to add an empty ref/net472/_._ to get NuGet's compatibility checking to work correctly. This isn't a scenario many customers ask about, so I don't know all the edge case behaviors. The ref/runtimes feature is normally used in a different scenario (when there's both in the same package).

Or maybe using MSBuild's Choose-When-Otherwise syntax could get you what you need: https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-conditional-constructs?view=vs-2019

Unfortunately your project/package was using Target Frameworks that were never valid (never appeared in any supported Target Framework list), and we don't have plans to change this at this time.

@sigurn
Copy link
Author

sigurn commented Oct 18, 2021

@zivkan Could you explain why with VS2019 if packages and project use TargetFramework native then PackageA and PackageB are restored but if TargetFramework is native-windows10.0 (both in packages and in project) then only PackageA is restored?

@zivkan
Copy link
Member

zivkan commented Oct 18, 2021

The last version of NuGet without net5.0-windows support was NuGet 5.7, and here is the code that is used to determine which TFM a project uses. We can see for C++ projects (the only project type where native is expected, it doesn't even read the targetFramework variable, it just returns a hardcoded native NuGetFramework object. NuGet 5.11, which is what is in VS2019 latest version, 16.11 has the same code. Simply having the project extension vcxproj triggers this. Therefore, I assume you're not doing this from a vcxproj, but that's the only way I know of where native TFMs are supported. So, I can't guess what you're doing, or why it works.

@sigurn
Copy link
Author

sigurn commented Oct 19, 2021

@zivkan I don't think that piece of code is related. I don't use vcxproj file. What I am doing you can find in the project, I just changed TargetFramework to native and then native-windows10.0 to compare behavior. The interesting thing is when I set TargetFramework to native-windows10.0 (PackageA has dependencies section for native-windows10.0) and build build.proj with VS2019 (actually run build_vs2019.cmd) it restores only PackageA. If I run nuget (version 5.11.0.10) from command line like

nuget install NuGet.Issue11253.PackageA -Framework native-windows10.0

it installs both packages PackageA and PackageB. So command line nuget works as expected whereas MSBuild works differently. This leads me to a conclusion that there is something wrong in how MSBuild uses NuGet. Do you know if there is a way to make it work the same as command line nuget?

@zivkan
Copy link
Member

zivkan commented Oct 20, 2021

That code I linked is exactly what is used in PackageReference restore (and maybe packages.config in Visual Studio). It doesn't matter that you're using some custom proj file, rather than a typcial csproj, vcxproj, fsproj, vbproj. If Microsoft.Common.props is being imported (via whatever SDK the project uses), that's the code that figures out what target frameworks the project uses.

NuGet.exe install, regardless of whether the -Framework argument is used, doesn't use that MSBuildProjectFrameworkUtility code because it's not trying to detect a project's target framework. I don't know what TFM it uses when you don't specify a framework, but if you do, it just needs to not throw an exception in NuGetFramework.Parse(). NuGet doesn't know what .NET Framework client profiles are valid (for net472-custom), or .NET platforms (for net5.0-custom), hence it requires to run via the .NET SDK's msbuild targets to validate.

MSBuild's "contract" for how target framework is defined, has always been the TargetFrameworkMoniker, TargetFrameworkIdentifier, and TargetFrameworkVersion properties. TargetFramework was a short version for convenience that NuGet was always doing "wrong", and had to be fixed to support net5.0. Now that NuGet has fixed this issue, it's possible to do silly things like:

<Project Sdk="Microsoft.Net.Sdk">
   <PropertyGroup>
     <TargetFramework>AnythingIWant</TargetFramework>
     <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
     <TargetFrameworkVersion>5.0</TargetFrameworkVersion>
     <TargetFrameworkMoniker>$(TargetFrameworkIdentifier),Version=$(TargetFrameworkVersion)</TargetFrameworkMoniker>
   </PropertyGroup>
</Project>

This will now work. Due to a design flaw in NuGet's assets file format, it's not possible to have multiple target frameworks use the same canonical TFM (the issue to implement "TF as alias" would fix that). The TargetFramework MSBuild property is only parsed if the TFM, TFI and TFV properties are not set, but in that case we can't guarantee behaviour of non-valid framework names. Think of it like C/C++ language "undefined behavior".

@sigurn
Copy link
Author

sigurn commented Oct 21, 2021

@zivkan I understand your point about validation but if net472-custom is not valid then I expect that MSBuild will throw error something like Profile 'custom' for .NETFramework,Version=v4.7.2 is not supported or something like that. Instead it just ignores transitive dependencies and restores high level packages only.
Another point is If it is supported by Nuget it is not clear why it is not supported by MSBuild. Especially when SDK like "Microsoft.Build.NoTarget" is used. Here is the description from the SDK:

The Microsoft.Build.NoTargets MSBuild project SDK allows project tree owners the ability to define projects that do not compile an assembly. This can be useful for utility projects that just copy files, build packages, or any other function where an assembly is not compiled.

That is exactly our case we just use MSBuild for building installer no assemblies building or so. And we expect that restore will just restore packages without any extra validation. Just the same as command line NuGet does. Does it make sense? Don't you think that current inconsistent behavior makes some mess?

@zivkan
Copy link
Member

zivkan commented Oct 27, 2021

I found this: https://github.com/dotnet/sdk/blob/22c4860dcb2cf6b123dd641cc4a87a50380759d5/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets#L28-L39

It appears that the .NET SDK does not support automatic inference of .NET Framework profiles, and that customers are explicitly required to set the TargetFrameworkIdentifier, Version and Profile values.

If you'd like MSBuild and/or the .NET SDK to infer these values, and error out when it's not supported, you'll need to create issues in https://github.com/dotnet/msbuild or https://github.com/dotnet/sdk. I validated that NuGet's NuGetFramework.Parse works a I expect and contains the appropriate properties for callers to obtain the relevant values.

Microsoft.Build.NoTarget is supported by an independent team, their repo is at https://github.com/microsoft/MSBuildSdks/ for issues specific to those SDKs.

@sigurn
Copy link
Author

sigurn commented Oct 28, 2021

@zivkan, Wow! Thank you very much for the information. It really did the trick and now, when I set TargetFrameworkProfile explicitly, everything works as expected. Thanks a lot for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution:NotABug This issue appears to not be a bug Resolution:Question This issues appears to be a question, not a product defect Type:Bug
Projects
None yet
Development

No branches or pull requests

4 participants