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

Remove NuGet dependencies version upper limits #4642

Merged

Conversation

paulomorgado
Copy link
Contributor

@paulomorgado paulomorgado commented Dec 12, 2023

Some packages have upper version limits on some of the dependencies which result in NU1608 warnings.

Since this does not prevent the usage of upper version packages, this PR removes that limitation.

Closes #4638

@olegd-superoffice
Copy link
Contributor

olegd-superoffice commented Dec 12, 2023

Upper limits for Asp.Net Core 2.x dependencies should not be removed. 2.2 is deprecated while 2.1 is the only supported version which will receive security updates. Upper limits are introduced in #4561 for this reason.
Probably worth adding a comment about why those upper limit exist in NSwag.Commands.csproj

@paulomorgado
Copy link
Contributor Author

Upper limits for Asp.Net Core 2.x dependencies should not be removed. 2.2 is deprecated while 2.1 is the only supported version which will receive security updates. Upper limits are introduced in #4561 for this reason. Probably worth adding a comment about why those upper limit exist in NSwag.Commands.csproj

@olegd-superoffice, #4561 does nothing to prevent the user from targeting those versions. It can still be forced.

The same would apply to .NET 3.1 and .NET 5.0.

@olegd-superoffice
Copy link
Contributor

Preventing the user of using anything was not the purpose of that PR. The purpose was to change versions of Microsoft.AspNetCore.*.dll bundled inside NSwag.MSBuild package (in tools\Win folder). Without upper limit the package included deprecated 2.2.x versions which are in fact older than 2.1.x versions of the same dlls and are flagged by various security scanners.

@paulomorgado
Copy link
Contributor Author

Preventing the user of using anything was not the purpose of that PR. The purpose was to change versions of Microsoft.AspNetCore.*.dll bundled inside NSwag.MSBuild package (in tools\Win folder). Without upper limit the package included deprecated 2.2.x versions which are in fact older than 2.1.x versions of the same dlls and are flagged by various security scanners.

The effective change to the dependency was changing 2.2.0 to 2.1.x and not setting an upper limit.

If that could be a really issue, [2.1.x] could be used instead of just 2.1.x.

@olegd-superoffice
Copy link
Contributor

Having upper limit just makes it explicit, Either way comment in NSwag.Commands.csproj is required to explain why versions 2.1 are used and why those references should not be updated to 2.2

@paulomorgado
Copy link
Contributor Author

Having upper limit just makes it explicit, Either way comment in NSwag.Commands.csproj is required to explain why versions 2.1 are used and why those references should not be updated to 2.2

It does nothing of the sort. The source code will only use one specific version and it can't target a incompatible runtime.

The end user can still force it in a number of ways.

@olegd-superoffice
Copy link
Contributor

Nothing of which sort? The end user cannot force anything about dlls bundled inside NSwag.MSBuild package.

@paulomorgado
Copy link
Contributor Author

Nothing of which sort? The end user cannot force anything about dlls bundled inside NSwag.MSBuild package.

And nothing on the dependency version declaration changes that.

But that's not the case with NSwag.Commands. The package does not ship with the dependencies. Only declares the references.

@olegd-superoffice
Copy link
Contributor

So what does removing upper limit achieve here?

As you said, end user can override it if they want. But having upper limit prevents end user from using unsupported library versions accidentally. Especially for people who are using paket for dependency management which would just update everything to the latest version on paket update.

@paulomorgado
Copy link
Contributor Author

So what does removing upper limit achieve here?

As you said, end user can override it if they want. But having upper limit prevents end user from using unsupported library versions accidentally. Especially for people who are using paket for dependency management which would just update everything to the latest version on paket update.

Because I don't use paket I forgot about that. But I expect this package to not be the only one to cause issues.

I'd need a binlog to analyze that.

@Lyra2108
Copy link

I would also highly welcome this change. We updated to net8 and this package is now causing build warnings for our build.

e.g.

Warning NU1608 : Detected package version outside of dependency constraint: NSwag.Generation.AspNetCore 13.20.0 requires Microsoft.Extensions.DependencyInjection.Abstractions (>= 7.0.0 && < 8.0.0) but version Microsoft.Extensions.DependencyInjection.Abstractions 8.0.0 was resolved.
Warning NU1608 : Detected package version outside of dependency constraint: NSwag.Generation.AspNetCore 13.20.0 requires Microsoft.Extensions.Options (>= 7.0.0 && < 8.0.0) but version Microsoft.Extensions.Options 8.0.0 was resolved.

I see the point that you want to prevent accidental issues with too new versions. But like @paulomorgado mentioned, it is also the only package for us which does this and causes the issue.
Which now leaves me with the pain to either void the 0 build warnings policy I bound myself to (It is way simpler to notice new bugs if the build is not flooded with warnings) or ignore the warning which as far as I know I can only do in general so it would leave me blind for other packages issues :/

Also with the paket update: I have not experienced so far that this is a real use case long run in a production environment. Any package update I do, I need to test. Even if I blindly update all of them, I still need to go back and check for the updates done. Usually it is not the dependency tree which causes the issues, especially for the MS libs but more changes within the lib.

@olegd-superoffice
Copy link
Contributor

@Lyra2108 I'm only talking about Microsoft.AspNetCore.* packages and only for .Net Framework 4. Do you have use case where having upper limit on these particular packages causes you any pain?

@paulomorgado
Copy link
Contributor Author

@olegd-superoffice, can you provide a minimal exemple of how this is an issue for you?

@olegd-superoffice
Copy link
Contributor

It is not an issue for me. Because I'm already very familiar with the versioning mess created by Microsoft with Microsoft.AspNetCore.* packages on .NET 4. The reality of this is that lower (2.1.x) versions of packages are newer and supported while higher versions of packages (2.2.x) are older, insecure and not supported. This is exception and do not apply to other packages. In this reality having explicit upper limit on these versions is helping users to not shoot themselves in the foot is a good thing.

I have already described how it can be an issue for someone using paket. Here's a simple script to reproduce it:

dotnet new classlib -f netstandard2.0
dotnet new tool-manifest
dotnet tool install paket
dotnet tool restore
dotnet paket init
echo nuget NSwag.Commands >> paket.dependencies
dotnet paket update | find "AspNetCore"

This will use current release version of NSwag.Commands (13.20) which doesn't have this upper limit. Observe that versions 2.2.x were installed by paket.

@paulomorgado
Copy link
Contributor Author

paulomorgado commented Dec 13, 2023

The paket.dependencies I ended up with is:

source https://api.nuget.org/v3/index.json

storage: none
nuget
NSwag.Commands

And got this error:

dotnet paket update
Paket version 8.0.0+6bcb14ec191f11e984ff0e58016f5987a5cfa8f6
Total time taken: 0 milliseconds
Paket failed with
-> Error in paket.dependencies line 4
     could not retrieve NuGet package from

I tried with

dotnet new classlib -f netstandard2.0
dotnet add package NSwag.Commands
dotnet build

And got a dependency of Microsoft.AspNetCore/2.1.7:

sls -path .\bin\Debug\netstandard2.0\nuget.deps.json -s -patt '"Microsoft.AspNetCore/'

D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:34:      "Microsoft.AspNetCore/2.1.7": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2034:    "Microsoft.AspNetCore/2.1.7": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2038:      "path": "microsoft.aspnetcore/2.1.7",

However I can force it to reference Microsoft.AspNetCore/2.2.0, even though it's outside the interval:

dotnet add package Microsoft.AspNetCore --version 2.2.0
dotnet build

Now it uses 2.2.0:

sls -path .\bin\Debug\netstandard2.0\nuget.deps.json -s -patt '"Microsoft.AspNetCore/'

D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:34:      "Microsoft.AspNetCore/2.2.0": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2034:    "Microsoft.AspNetCore/2.2.0": {
D:\Temp\Projects\nswag#4642\nuget\bin\Debug\netstandard2.0\nuget.deps.json:2038:      "path": "microsoft.aspnetcore/2.2.0",

paket chooses to make its own interpretation of how NuGet should work and it's paket's responsability to make it work.

@olegd-superoffice
Copy link
Contributor

Are you getting new line in paket.dependencies between nuget and NSwag.Commands? It should look like this (nuget NSwag.Commands in the same line):

source https://api.nuget.org/v3/index.json

storage: none
nuget NSwag.Commands 

Also, you can replace target framework in generated .csproj file with net48, I just used netstandard2.0 because it is supported by classlib template.

@paulomorgado
Copy link
Contributor Author

Are you getting new line in paket.dependencies between nuget and NSwag.Commands? It should look like this (nuget NSwag.Commands in the same line):

source https://api.nuget.org/v3/index.json

storage: none
nuget NSwag.Commands 

That's what echo nuget NSwag.Commands does on PowerShell.

Also, you can replace target framework in generated .csproj file with net48, I just used netstandard2.0 because it is supported by classlib template.

That will cause errors with dotnet paket update

You can use dotnet paket add Microsoft.AspNetCore --version 2.1.7 to pin the version of Microsoft.AspNetCore.

@Sonic198
Copy link

Hi, any idea when this fix could be released?

@olegd-superoffice
Copy link
Contributor

That will cause errors with dotnet paket update

No, I just tested it. No errors here.

You can use dotnet paket add Microsoft.AspNetCore --version 2.1.7 to pin the version of Microsoft.AspNetCore.

Thanks, I already know how to pin dependencies versions, but my point is that the defaults should be safe to use and that's what upper limit provides here. But can you provide a minimal example of how this existing upper limit is an issue for you?

@paulomorgado
Copy link
Contributor Author

Setting upper version limits is for when the referencing library does not work with versions above a specific version, which is not the case. It has nothing to do with supportability of the dependencies.

As far as I know, the whole NuGet system was built on the premise that each package should reference the lowest dependencies that match the usage. Upper limits are in place because there might be breaking changes in the dependencies.

Paket, on the other end, chose to opt for the highiest version, knowing the issues that would bring.

I'll leave the final decision to @RicoSuter .

@olegd-superoffice
Copy link
Contributor

Setting upper version limits is for when the referencing library does not work with versions above a specific version, which is not the case. It has nothing to do with supportability of the dependencies.

This is just your opinion, but there are other opinions what upper limit is for. For example Microsoft Kiota developers use different approach.
Also in case of Microsoft.Extensions.* packages it is actually the case. See the answer here - versions of these packages should match the major/minor of the ASP.NET Core version you are running.

Anyway, you cannot provide any example of how this limit on .NET 4 is an issue for you?

@olegd-superoffice
Copy link
Contributor

@Lyra2108 BTW, can you try the latest preview of NSwag v14 (14.0.0-preview012 as of today) and see if you still have this warning?

@Lyra2108
Copy link

Well... Yes, the warnings are fixed with the latest preview...

grafik

But I need to do a lot of commented out lines to not have build errors any more and for some e.g. the TypeMappers I could not see quick replacements. So I will need an update guide and quite some time to really use the v14. Therefore, an update for the v13 would be preferred.

@RicoSuter
Copy link
Owner

@Lyra2108 all these settings are now under SchemaSettings and not on the generator settings itself anymore, should be quick to fix.

@Lyra2108
Copy link

Thanks, that was simpler and faster to get running then expected. :) I am happy to wait for the v14 Release.

@RicoSuter RicoSuter merged commit 08ad638 into RicoSuter:master Dec 15, 2023
1 check passed
RicoSuter added a commit that referenced this pull request Dec 15, 2023
@paulomorgado
Copy link
Contributor Author

@RicoSuter, because it's a, somewhat, unexpected breaking change, this could be attributed as obsolete with warning and proxied for the new API.

In a few releases, it should be an error and removed only on version 15.

I can submit a PR for that.

@RicoSuter
Copy link
Owner

@paulomorgado what do you mean?
Can you create a PR?

@paulomorgado
Copy link
Contributor Author

@RicoSuter, I just created #4648 for discussion.

@paulomorgado paulomorgado deleted the remove-nuget-version-upper-limit branch December 19, 2023 17:42
lahma pushed a commit to lahma/NSwag that referenced this pull request Jan 20, 2024
lahma pushed a commit to lahma/NSwag that referenced this pull request Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NSwag.AspNetCore has an upper version limit for Microsoft.Extensions.FileProviders.Embedded
5 participants