Home: Suppress the <requireLicenseAcceptance> element when packing a project

[jnm2]

When I pack my project using msbuild /t:pack, the generated nuspec contains this redundant element with the default value:

    <requireLicenseAcceptance>false</requireLicenseAcceptance>

To me it makes sense to remove it if the csproj does not contain a <PackageRequireLicenseAcceptance> element.

[anangaur]

@jnm2 we do not plan to fix it in near future due to other higher priority items. Would you like to take a crack at fixing this and raising a PR?

[jnm2]

I would! I'll get started soon. Is there a pull request that would have been similar that I could glean quick information from, like test setup?

[nkolev92]

An example is https://github.com/NuGet/NuGet.Client/commit/3c9b61826814dc800a2a295f0985ce4f295483d6,

It might be tricky to honor whether the prop is set or not because the type is a boolean so you can't really express 3 different states with that (true, false, not specified).

[joelverhagen]

Moved from https://github.com/NuGet/NuGet.Client/pull/3240#issuecomment-587878680.

This seems like a great change, but there are some concerns I have that should be investigated/mitigated/dismissed:

1. The default is not documented as far as I know: requireLicenseAcceptance
   
   
   
   • At the very least, we should document the default so docs match implementation.
   
   
2. Was the default _always_ false? For example, in NuGet 2.x/3.x/4.x was it like this?
3. What do other tools do? Do they use the same default?
   
   
   
   • nuget.org
   
   
   • NuGet Package Explorer
   
   
   • others?
   
   

Maybe we can consider an announcement about this clarification as well so if someone comes out of the woodwork after we make this change saying their code has a different interpretation of a default we can point them to a published explanation.

[jnm2]

Oh, my bad! I was sure it was documented to be false. It looks like only the .csproj property default is documented to be false.

Changing the builder and reader types from bool to bool? would be a breaking change for anything compiled against this library, so I'm assuming you wouldn't do that? If you decide to roundtrip a three-state value (true/false/no element), the only alternative then is to add something like a new builder/reader property to supplement or replace it.

[dominoFire]

One concern is that you're making NuGet clients 'interpret' what it means to not to have the requireLicenseAcceptance attribute.

Let's say that we make all the changes to not to ask for a license acceptance when the attribute is not set. Later, we decide that all NuGet clients requires the license acceptance. In that case we just broke a lot of packages.

I'm not that confident of making this change. Thoughts? @anangaur @joelverhagen @nkolev92 @jnm2 @chgill-MSFT