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

Show extra validation information during pack to allow for more effective means of communicating best practices such as README #12070

Open
nkolev92 opened this issue Sep 1, 2022 · 13 comments

Comments

@nkolev92
Copy link
Member

nkolev92 commented Sep 1, 2022

Showing additional pack validation information

Summary

Package authoring best practices are an ever evolving set of rules and guidelines. With the addition of new features, there are new recommended practices.
Historically warnings have been the only way the tooling can communicate that certain metadata or package structure is suggested.
Warnings can be disruptive and not every new ideas warrants that alert level.
Adding a non-breaking validation message would allow NuGet to make soft recommendations for scenarios such as README.

Motivation

For some of the package authoring best practices, such as embedded licenses, a warning was added to alert the user.
TreatWarningsAsError isn't an uncommon thing, so warnings in these cases can be disruptive.
Furthermore, the warning itself wasn't really saying use an embedded license, but rather saying don't use licenseurl, use an embedded license instead, thus elevating the urgency.

Showing extra validation information during pack to allow for more effective means of communicating best practices such as READMEs.

Explanation

Functional explanation

Currently when you pack the package, you get the following output.

  Successfully created package 'C:\Code\Temp\Pack\bin\Debug\Pack.1.0.0.nupkg'.

When every package is create, 2 sets of validations are run:

  • Mandatory validation, valid package id, valid version
  • A set of warning validation rules is run and each one is usually associated with a warning code.
  • This proposes a new set of rules. These rules are going to be independent from the other sets of rules. A violation of any of these rules will lead to an information message indicating that the metadata can be specified.
    The idea is that this feature will be completed by the package scoring.

Currently if you have a warning, you get the following:

C:\Program Files\dotnet\sdk\6.0.400\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(221,5): warning NU5125: The 'licenseUrl' element will be deprecated. Consider using the 'license' element instead. [C:\Code\Temp\Pack\P
ack.csproj]
  Successfully created package 'C:\Code\Temp\Pack\bin\Debug\Pack.1.0.0.nupkg'.

The proposal is to add the following on normal verbosity.

The created package is missing a readme. Go to https://aka.ms/nugetpackreadme to learn why package readmes are important.
The created package does not specify a license. Go to https://aka.ms/nugetpackagelicense to learn why package licenses are important.
ack.csproj]
  Successfully created package 'C:\Code\Temp\Pack\bin\Debug\Pack.1.0.0.nupkg'.

The validations can be added on minimal or normal verbosity level.
msbuild /t:pack and nuget.exe pack default to normal verbosity, while dotnet.exe pack defaults to minimal verbosity.

It is considered a good practice to have 1 information output per project and per operation. As such, these messages will be added on normal verbosity.

Technical explanation

By default these validations are not going to raise any warnings or errors.
These validations will not something that can be disabled early on.
These validations cannot be elevated to warnings at this point.

Drawbacks

  • Pack command noise. These best practices are not relevant for every package created. Some packages are easy.
  • Not super visible. Non-breaking output from commandline commands is not always gonna be noticed by package authors.

Rationale and alternatives

  • By going with the current minimal approach we allow ourselves the freedom to experiment and get this new set of validations correct.
  • Do nothing.
  • Add a warning. This is specific to a single rule, but we want to create a concept that would allow us to add non-breaking validations.

Prior Art

  • Pack Validation rules are similar, but they are not a perfect paralell as they do not really require you to take action.

Unresolved Questions

  • Is normal verbosity good enough?
  • Is it ok to ship without a means to upgrade these validations to warnings.

Future Possibilities

  • Add a means to upgrade these validations to warnings/errors.
    • We might need to define nuget log codes for each validation. Alternatively, we can use 1 warning code for all information validations that get upgraded to errors.
@heng-liu heng-liu added internal-debugging For bot related testing and removed internal-debugging For bot related testing labels Sep 1, 2022
@nkolev92
Copy link
Member Author

nkolev92 commented Sep 8, 2022

I've added some quick design ideas in the issue body.
The exact text in the output is up for discussion.

@zivkan
Copy link
Member

zivkan commented Sep 9, 2022

while dotnet.exe pack defaults to minimal verbosity
Is normal verbosity good enough?

In my opinion no, normal verbosity is not good enough given dotnet pack defaults to minimal. Most customers will never have the messages output to their console or CI logs, so never even have an opportunity to see the message. That's before we consider the points you raised about customers ignoring anything that's not an error.

@heng-liu
Copy link
Contributor

As the set of rules and guidelines keep evolving, people use different versions of NuGet client tooling will get different info. Will this cause confusion? Or shall we show something when it's not checking against latest rules?

@jwfx
Copy link

jwfx commented Jun 22, 2023

I am being spammed by
"The package xxxxxx is missing a readme. Go to https://aka.ms/nuget/authoring-best-practices/readme to learn why package readmes are important."

How can I get rid of all these educational warnings permanently?

@JonDouglas
Copy link
Contributor

@jwfx add a readme or suppress the warning code

@jwfx
Copy link

jwfx commented Jun 22, 2023

@JonDouglas Where can I find the warning codes for those particular messages? They are not printed out in the log.

@nkolev92
Copy link
Member Author

There's no warning code for that particular message, it's an info level message that'll always be there unless you have a readme.

@jwfx
Copy link

jwfx commented Jun 22, 2023

That is very unfortunate to hear.

I hope you guys can revisit this whole approach with those educational messages.

As it stands right now this giving me a pretty bad user experience.

I try to keep all our CI logs as clean as possible, so issues are easy to spot. Now they are spammed with warnings that have literally no use for me and my colleagues as we're using a private NuGet server that does not even support rendering readmes. While we do have a readme in our repository, it is not always something that we would want to pack into the resulting NuGet package.

There really needs to be a way these type of warnings can be disabled or they need to be way less intrusive.

@ali50m
Copy link

ali50m commented Jun 29, 2023

@JonDouglas Could you let us know to suppress the warning code?

@jeffska
Copy link

jeffska commented Jul 15, 2023

Not every NuGet package is going to be exposed publicly/externally, and therefore missing a README isn't a huge deal. It would be nice to have some flag to suppress the message (exposed in project files), it's distracting to see in what should be clean build output. Adding a dummy readme to 100's of internal packages isn't a worthwhile exercise.

Seems like this would be something that would be better suited to be shown when pushing to a public feed.

@NickCraver
Copy link

@nkolev92 Please add a way to disable this rather spammy message. I have no reason to package one for many private packages and in other cases no desire to package an immutable README in the package and this is something we've discussed in the past just like release notes. In both cases, a README was purposely left off, so a message spamming to the console as we pack dozens of packages in a build (causing roughly half the output) helps nothing and adds hindrance to identifying actual issues. We need a way to disable this.

@JonDouglas
Copy link
Contributor

Hi folks, this was an under sight by implementation. We are working to code every message here to ensure you can ignore/disable them. Apologies in advance as I thought they were coded and they are not as @nkolev92 mentioned. Additionally, a way to opt-out of any "best practices" messages entirely to come.

@jwfx
Copy link

jwfx commented Jun 7, 2024

@JonDouglas
Is there any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants