NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

Improve warning message when using licenseUrl #8000

Open zivkan opened 5 years ago

zivkan commented 5 years ago

Some customers do not find it obvious how to act on the warning. For example this VS feedback: https://developercommunity.visualstudio.com/content/problem/504915/nuget-package-license-warning-not-resolvable.html

It's easy to understand why customers seeing the following warning message:

warning NU5125: The ‘licenseUrl’ element will be deprecated. Consider using the ‘license’ element instead.

believe it's possible to simply rename the licenseUrl element to license. The NU5125 docs aren't particularly good either. It doesn't contain any actionable information on the page itself, and just contains a link to a github issue that has even more links and a discussion. The point is a customer who wants to do the right thing has to make an unreasonable amount of effort just to find examples on how to use the new embedded license feature when they see the NU5125 warning.

I suggest we change the URL in the NU5125 docs to a page that has clear, concise instruction on how to use embedded licenses, and we should also include the URL to those docs in the warning message for anyone using nuget.exe or the dotnet cli, since they won't have a clickable link like in the Visual Studio error list.

nkolev92 commented 5 years ago

You want the warning message to contain a link to the warning docs?

I have a philosophical problem with that. We should not have hard links from the product. If you are suggesting aka.ms link, I'd say we should limit our exposure to potential docs site change root causes.

I'd be open to enhancing the warning message without any hard links. Improving the dosc should be a fast & easy win.

nkolev92 commented 5 years ago

Milestoned items don't need priority & backlog items have a priority.

zivkan commented 5 years ago

At the very least, the message shouldn't make it sound like it's possible to rename licenseUrl to license. It's easy to understand how some customers may have that impression, even if that's not what it actually says.

nkolev92 commented 5 years ago

I'd imagine it's a tiny minority that'd interpret it as such, but yeah we should do our best to improve it.

how about;

The ‘licenseUrl’ element is deprecated. The licenses should be self-contained within the package. Please use the ‘license’ element instead, which allows you to specify a standard license name or include a license file.

//cc @karann-msft

karann-msft commented 5 years ago

warning NU5125: The ‘licenseUrl’ element is deprecated. Consider using the ‘license’ element instead. It allows you to specify a standard license expression or include a license file as part of the package,

zivkan commented 5 years ago

My suggestion:

warning NU5125: The ‘licenseUrl’ element is deprecated. It is possible to specify a license expression or include a license file in a package using the ‘license’ element.

I also have issues with the NU5125 docs, but I think it's easier/quicker for me to create a PR rather than explain it and wait for someone else to work on it. Basically, imagine you're a customer who saw the warning message and wants to migrate to the new, supported feature. The current docs require at least 2 more clicks to find actionable information, but there are 6 links and only 2 seem to have actionable information, so there's basically a 1/3 chance they'll find the info in 2 clicks.

rconard commented 4 years ago

For those finding this issue via a search, the fix for this issue can be found here: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packing-a-license-expression-or-a-license-file

The deprecation warning should be modified to suggest the <PackageLicenseExpression> syntax instead of "license."

richlander commented 4 years ago

Related: I don't think <PackageRequireLicenseAcceptance>false</PackageRequireLicenseAcceptance> is honored in the dotnet pack scenario.

nkolev92 commented 4 years ago

Bumping to backlog + QW due to the linked issue upvotes.

michael-hawker commented 3 years ago

So, we just updated to remove the licenseUrl in favor of license as per the warning message, but then I was checking in Visual Studio (16.8.5) and it doesn't even properly accept the new format:

image

michael-hawker commented 3 years ago

Apparently using both like the icon property is hard blocked ☹:

C:\Program Files\dotnet\sdk\5.0.103\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(207,5): error NU5035: The PackageLicenseUrl is being deprecated and cannot be used in conjunction with the PackageLicenseFile or PackageLicenseExpression. [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls\Microsoft.Toolkit.Uwp.UI.Controls.csproj]
nkolev92 commented 3 years ago

So, we just updated to remove the licenseUrl in favor of license as per the warning message, but then I was checking in Visual Studio (16.8.5) and it doesn't even properly accept the new format:

image

Where was the package published? Nuget.og ingests and provides a better link. Azure DevOps doesn't do that.

michael-hawker commented 3 years ago

@nkolev92 this was me trying to test from our Azure DevOps Artifacts feed to understand how this would look before we push to NuGet.

nkolev92 commented 3 years ago

That's expected in that case. If you follow the link it talks about potential limitations of the server or the client. Given that you are on the latest client, in your case it's the server.