Projects which set IncludeBuildOutput=false are likely to hit NU5128 #8583
Comments
|
@zivkan - let's work on the right fixes, and mitigation with docs. if it is hit alot, i'd like to be prepared with a servicing fix. |
|
SDK and tools packages use the What are use cases for a package with only I'm concerned that skipping the validation for all packages that use |
Build infrastructure packages: these are cases where we build a task dll and then package it under build or tools with targets files. I did a search on GitHub for IncludeBuildOutput and I see a number of other cases like templates and content packages that happen to use CSProj's to pack (because that's the only thing NuGet supported via task dll). Here's a suggestion:
I suspect this will still error for folks trying to build meta-packages with no ref nor lib via CSProj. It could also still error for folks building build infra packages with legit dependencies. Maybe those are acceptable/rare enough? |
|
I see the pack validation warnings as similar to compiler or fxcop warnings. They will be reported for some users who don't want to resolve the issue. They have the option of not elevating the warning to an error so that they don't break their build, and/or using The idea to skip nu5128 when the package has no lib/ref and none of the dependency groups have any dependencies is interesting. As you wrote in the issue, using However, if the package contains dependency groups, regardless of if they're empty or not, developers using Visual Studio's Package Manager UI or viewing the package on nuget.org will assume that the package is compatible with only the TFMs listed, which come from the nuspec dependency groups. But without any I want to add instructions to the docs for the warning code about |
... of course. That's a bit orthogonal to this issue which is pointing out an issue with an individual warning.
It's a (long standing, and frustrating) bug to treat dependency groups as an indication of support. Dependency groups are an implementation detail of the package. "Support" is determined by "does the package install"?
That's incorrect. It will only install where the package has applicable assets. The applicable assets may be the dependencies, may be build or content assets. Something must be applicable for it to install. Even if the warning might be pedantically correct, you have to ask "does it matter?". Consider the case where all dependency groups are empty. You're going to get the same dependencies regardless of what framework is resolved: why does it matter? Consider the case where ref/lib are empty but other asset groups are not: adding ref/lib would change the semantics of their package so its likely the wrong warning to even raise. For a warning like this to be successful it has to have a very low rate of false positives, otherwise folks will just ignore it. Given the amount of false positives we're already seeing I think something needs to be done to relax it, otherwise $(NoWarn);NU5128 will enter the ecosystem boilerplate. |
Depends if we're discussing the issue from a technical or practical point of view. If technical, yes, if the package install/restore succeeds, then it's "supported". However, if developers in the ecosystem have a certain expectation and NuGet doesn't work in the same way, either there's an education problem, or there was a design flaw. We can't change a design flaw without possibly breaking a lot of people, hence the pack warning helps better align packages with real expectations, even if it's otherwise technically valid (much like an async method with no await).
Unfortunately that's not the case. I've attached a little sample where a package contains only a Therefore I believe the NU5128 warning helps package authors create packages that are more likely what they intended, and will work in ways that I imagine package consumers expect.
I guess some false positives were from build packages, which will affect all customers that create such packages. I wonder what percentage of packages on nuget.org don't have lib or ref files, but do have other assets. But other false positives were from SDK packages which 1. will stop if we suppress the warning for PackageTypes that are not dependency packages and 2. outside of DevDiv in Microsoft, I imagine the percentage of developers creating SDK packages is very close to zero. NuGet's DevDiv partners are very important, but if the rule helps improve average package quality for everyone else, it may be worthwhile to keep. |
You don't need to break anything to fix the representation foR support. Just add an explicit representation of support by evaluating where the package will install. This isn't so hard to do.
That's a bug. NuGet shouldn't permit install of a package if it has applicable assets on some framework but no applicable assets on the target framework and no indication of support via placeholders/empty groups. I'm pretty sure some previous version of NuGet would fail install of this package. I actually tried this using packages.config and it fails: If I use packagereference it succeeds. That definitely feels like a bug to me. Agreed that data can help you decide here. I believe you will find lots of community packages that are just build and/or content. Agreed I don't expect there to be a lot of SDKs since SDKs aren't easily acquireable. |
It's unfortunate, and in retrospect probably a bug, but at the time the compatibility checking for PR was implemented, this was treated as by design. See #6908 (comment) |
|
A docs PR is in flight to cover NU5128: NuGet/docs.microsoft.com-nuget#1677 |
A new NuGet warning was introduced that requires a workaround see NuGet/Home#8583
* Update dependencies from https://github.com/dotnet/arcade build 20190918.2 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19468.2 * Update dependencies from https://github.com/dotnet/arcade build 20190919.4 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19469.4 * Update dependencies from https://github.com/dotnet/arcade build 20190919.8 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19469.8 * Update dependencies from https://github.com/dotnet/arcade build 20190920.9 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19470.9 * Update dependencies from https://github.com/dotnet/arcade build 20190923.5 - Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19473.5 * Upgrade msbuild-xcopy version to 16.3 * Also specify 16.3 as the required VS version * Update contributor docs to link .NET Core 3 as the build SDK * Update packages to fix NuGet warnings A new NuGet warning was introduced that requires a workaround see NuGet/Home#8583 * Embed VB runtime by default
|
A similar package type issue was observed here: dotnet/arcade#4337. |
Ignoring NU5128 as per https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5128 and NuGet/Home#8583.
|
I have a package that is hitting this warning, and for me it's just noise I have to disable because I very intentionally want the behavior this is warning about. The package uses I've previously requested a change to packing because of this package in #7154, but that was left rather inconclusive. I currently have this package targeting |
|
We've also got a package which raises NU5128. It's a Tool package so it doesn't contain Generated NuSpec (i've edited out description only properties like release notes, tags,... ): Which is generated from running Note: |
nkolev92
added
Product:dotnet.exe
Area:ErrorHandling
|
@nkolev92 - nope, that's not what the example had. I figured that out later for a different package once I finally hacked the nuspec.xsd so I could get intellisense support for nuspec and saw what it wanted. Its been a few months since I've had to make a package and was unpleasantly surprised by this extra bit of complexity and ongoing maintenance for a simple package. It used to be dead simple to make a package, sometimes it would even work without a nuspec. It was an easy add on to primary development work. Now it's a whole rigamarole that needs to be budgeted time in an estimate, etc. There's only one framework version mentioned in the nuspec, and that was enough. Now I get an error (treat warnings as errors) or warning if I don't do some extra work just to appease someone's engineering perfectionism. |
|
The example maybe can be improved but it's recommending removing the dependencies element and only using the dependencyGroup element.
It's effectively saying do: <dependencies>
<group targetFramework=".NETFramework4.6.2">
<dependency id="Castle.Core" version="4.2.0" />
<dependency id="Castle.WcfIntegrationFacility" version="4.1.0" />
<dependency id="Castle.Windsor" version="4.1.0" />
<dependency id="log4net" version="2.0.8" />
<dependency id="Newtonsoft.Json" version="9.0.1" />
</group>
</dependencies>Created a docs issue: https://github.com/NuGet/docs.microsoft.com-nuget/issues/2005 |
Some developers like to know what TFMs a package supports, so they have an idea about whether it's going to be compatible with their project before they try to use the package. Although it's not 100% accurate for reasons I won't go into here, the best way (pretty much the only way currently) they can make that decision is by looking at the dependency groups, which both nuget.org and Visual Studio's package manager UI show. To these customers it doesn't matter if the package only supports a single TFM, if all supported TFM have the same dependencies, or the package doesn't have dependencies. When the package doesn't list TFMs it supports as dependency groups, they don't have information they're interested in. So, this rule is aimed at improving the ecosystem, so that NuGet works for more developers the way they want it to. Claiming it's just to appease someone's engineering perfectionism is a strawman argument. For what it's worth, NuGet does raise NU5128 as a warning, so if it's reported as an error, it's because the project opted into warnings as errors. |
**In this PR:** - Add metapackage (no dll, just dependencies) to ease upgrade for users already using storage extensions. **Testing** I was running locally: ``` dotnet pack eng/service.proj -warnaserror /p:ServiceDirectory=storage /p:IncludeTests=false /p:PublicSign=false /p:OfficialBuildId=20201021.6 /p:SkipDevBuildNumber=false /p:Configuration=Release /p:CommitSHA=8b061b75d76cce3f93bb3741d622b26605ea36f0 /p:ArtifactsPackagesDir=C:\tmp\webjobs ``` The outcome is:  **Note** I was running into NuGet/Home#8583 while doing that. Hence the workaround I've copied from one of linked PRs in the csproj file.
|
Note: NU5017, which is an error, not a warning is also in play when using Specifically, I'm trying to create an SDK package. But when I set Note that the error is misleading; where it says "content" it means lib/ref assemblies; content files are not good enough. |
@zivkan -The <files>
<file src="bin\$Configuration$\MyAssembly.*" target="lib\net452"/>
</files>If there was a dependency declared, and that dependency targeted net452 (or not, idk, idc), it would be installed. That would be the complete effort needed for someone making a private package. Now we have another structure built to replace it that requires more time and effort* for a simple package, all to make it dependency declaration more overt and complete (the two qualities we typically glom into the idea of perfect). Circling back to the original topic though, I'd suggest anyone wanting to make a tools only package or that wants to not leave it up to nuget.exe/dotnet.exe to guess correctly as to what belongs in the package should really be looking to pack against a nuspec and not a proj file. Proj file based pack has always been a problem with how it chooses files to include or not. It would be nice if someone from the Nuget team could explain what the difference between Project Output and Build Output are in general and in relation to this topic. *we still do not have intellisense support for nuspec creation - why is that still a thing? its an XSD file that needs to be registered in VS |
The list of files in the package is not part of the "NuGet protocol" (the json that gets sent between Package Manager UI and NuGet sources for search results). We could add it to the protocol and get nuget.org to implement it, but there are a whole lot of existing NuGet servers (like Azure Artifacts, MyGet, NuGet feeds built into products like TeamCity, Artifactory, Nexus, and more) which would also need to be updated. Finally, if Visual Studio was updated to use the package's file list, only customers using the newest Visual Studio, and customers opening nuget.org in their web browser, would benefit. By having a pack warning that encourages package authors to create packages in a way that works with all existing NuGet server implementations and all previous versions of Visual Studio, the benefit to the ecosystem is maximised, not to mention it was significantly less effort that changing the protocol and Package Manager UI, allowing us to work on other features instead. NU5128 is a warning, so package authors have the choice of suppressing the warning. Most packages do not fall into the scenario where suppressing NU5128 is necessary or desirable.
We get feedback in many ways. Occasional surveys in Visual Studio, occasional surveys on nuget.org, interviews with customers over video, interviews with customers at and after conferences (not in 2020), interviews with Microsoft MVPs, in addition to feedback on developercommunity.visualstudio.com and github, monitoring social media like twitter, stackoverflow, and I'm sure others. The PMs periodically summarize their customer outreach to the engineering team, and I don't remember nuspec intellisense ever being a top issue. In fact, I believe that we get more positive feedback about packing SDK style csproj with msbuild/dotnet cli, than we get negative feedback about limitations in it compared to hand editing nuspec files. I hear you that it's something you are interested in. But we take into consideration a number of factors when deciding what work to prioritize, and adding intellisense for nuspec files has low customer engagement. |
|
TL;DR - Rather than rely on a command option that's not performing as expected, OP can probably work around this option behavior/problem by using a nuspec, This arguably also makes it easier for the next Long form @zivkan - There is probably some difference in understanding for all parties between project output and build output. From my time in working with windows installer technology, project output is the compiled output of the project and nothing else. No content or config files, just a DLL or EXE. I'd assume build output includes these things and anything else destined for the output folder. If that assumption is correct then what does this flag possibly do for a nuget pack operation against a csproj?
I dont get where you are going with this one because It worked before you added targets to dependency groups. The files in the package declare the framework target(s) already, and the information is already used by VS to block the installation of some packages when installing them into an inappropriate target project (a net46 package into a net45 project for example), and VS (or some subsystem) needs to read the nuspec in order to install the package. This is an extra step required now when I just want to reliably create and use packages without a lot of extra effort in my development workflow. That newly added feature code led to unplanned productivity delays for your customers. Hasn't that protocol been needing replacement or update for years now? (at least I think that was the reason given for why the PM UI couldnt be made better WRT search execution and performance in general )
All I really wanted for the nuspec.xsd was a legitimate xsd file. I dont know what the point of talking about an xsd in any of the documentation is if you dont actually have a working one available. It just needs to have the header namespaces set with actual values and not left as unfilled string interpolations. There is another issue open for that here already to continue any discussion on. Defects in programs (or documentation), feature requests/enhancements, and feedback are three distinctly separate things. This GH issue - like many of them - is a potential defect. It may turn into other defects and feature request, but its never going to be the same as a customer survey or casual conversation (feedback). |
When we do surveys, one of the top complaints from customers is the lack of ability to determine whether a package is compatible with their project before they attempt to install the package. They don't like attempting to install, only to get an error. I don't remember which version of Visual Studio first started showing dependency groups in PM UI, I don't think VS2010 did, so I'll guess VS2012, but let's say VS2012 to VS2019 only show dependency groups. They don't use the list of files in the package to show in the UI what TFMs the package is compatible with. Therefore, by encouraging package authors to ensure their nuspec's dependency groups match what NuGet uses for actual package compatibility, all customers, using either the website or any version of VS (excluding VS2010) will have "accurate" information to make decisions on. That's what our intern who worked on this expressed in the background section of the spec for this pack validation rule. This is the intent of the validation rule. To help customers understand package compatibility before attempting to install the package, using the only information currently available on nuget.org and PM UI so that it benefits the maximum number of customers.
No, but even if it did, it wouldn't help with the goal of the pack validation that raises NU5128, specifically the goal of maximizing the number of package consumers who will benefit from the change.
I wasn't aware that the docs referred to it. We can take a look at what context the xsd is being mentioned, and update the docs accordingly. Perhaps even delete the mention. |
@zivkan - There are packages that show up as "Blocked" and then packages that have dependency groups defined correctly that still only report incompatibility at install time (meaning the thing users wanted was not fixex)....
Nuget team has complained in the recent past about the protocol being slow and problematic as a reason why the search is crummy. Deleting the link to the XSD (from what is more or less "page 1") instead of fixing the namespaces at the top that are currently set to |
|
I'm not disputing there are bugs, I'm trying to help give an understanding why NU5128 was added in the first place. |
https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5128 NuGet/Home#8583 Cannot repro locally. C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj] C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: - Add a dependency group for UAP10.0 to the nuspec [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj]
https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5128 NuGet/Home#8583 Cannot repro locally. C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj] C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: - Add a dependency group for UAP10.0 to the nuspec [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj]
https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5128 NuGet/Home#8583 Cannot repro locally. C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj] C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: - Add a dependency group for UAP10.0 to the nuspec [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj]
https://docs.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5128 NuGet/Home#8583 Cannot repro locally. C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: Some target frameworks declared in the dependencies group of the nuspec and the lib/ref folder do not have exact matches in the other location. Consult the list of actions below: [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj] C:\hostedtoolcache\windows\dotnet\sdk\5.0.101\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5128: - Add a dependency group for UAP10.0 to the nuspec [D:\a\1\s\src\LibVLCSharp\LibVLCSharp.csproj]
**In this PR:** - Add metapackage (no dll, just dependencies) to ease upgrade for users already using storage extensions. **Testing** I was running locally: ``` dotnet pack eng/service.proj -warnaserror /p:ServiceDirectory=storage /p:IncludeTests=false /p:PublicSign=false /p:OfficialBuildId=20201021.6 /p:SkipDevBuildNumber=false /p:Configuration=Release /p:CommitSHA=8b061b75d76cce3f93bb3741d622b26605ea36f0 /p:ArtifactsPackagesDir=C:\tmp\webjobs ``` The outcome is:  **Note** I was running into NuGet/Home#8583 while doing that. Hence the workaround I've copied from one of linked PRs in the csproj file.
One approach is to wrap your <dependencies> in the .nuspec file with: <group targetFramework=".NETFramework4.6.2"> The other approach is <NoWarn>$(NoWarn);NU5128</NoWarn> Since I didn't get anywhere with the first approach, I went with the 2nd. Refs: NuGet/docs.microsoft.com-nuget#1677 NuGet/Home#8583 (comment)
One approach is to wrap your <dependencies> in the .nuspec file with: <group targetFramework=".NETFramework4.6.2"> The other approach is <NoWarn>$(NoWarn);NU5128</NoWarn> Since I didn't get anywhere with the first approach, I went with the 2nd. Refs: NuGet/docs.microsoft.com-nuget#1677 NuGet/Home#8583 (comment)
Packages that just deploy native runtime files. Combo of |
Details about Problem
Many projects set IncludeBuildOutput=false, then manually define the layout of their package. In case they are defining content other than lib/ref (eg: build/*, tools, or SDK) they are likely to hit NU5218, since the project's dependencies will make it into the package: IncludeBuildOutput does not control the inclusion of dependencies.
NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe):
dotnet.exe
NuGet version (x.x.x.xxx):
5.3.0.6192
dotnet.exe --version (if appropriate):
3.0.0-rc1-19455-02
VS version (if appropriate):
N/A
OS version (i.e. win10 v1607 (14393.321)):
N/A
Worked before? If so, with which NuGet version:
Yes, dotnet 2.x, past previews of 3.0
Detailed repro steps so we can see the same problem
dotnet packagainst the following project:Expect: no build errors
Actual: build errors due to warnings as errors for NU5128.
Workaround is to set
<SuppressDependenciesWhenPacking>true</SuppressDependenciesWhenPacking>as may be the intent for a project setting<IncludeBuildOutput>false</IncludeBuildOutput>. I don't think this is always the intent, so I don't think this property should be derived fromIncludeBuildOutput.Other suggested things
Issue is originally discussed here: dotnet/arcade#3861 (comment)
Related: #8576
Verbose Logs
https://github.com/NuGet/Home/files/3602557/msbuild.binlog.zip
Sample Project
See above
The text was updated successfully, but these errors were encountered: