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

NuGet curation fixes #225

Merged
merged 14 commits into from
Aug 28, 2024
Merged

NuGet curation fixes #225

merged 14 commits into from
Aug 28, 2024

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Aug 1, 2024

No description provided.

@fviernau fviernau marked this pull request as ready for review August 1, 2024 15:03
@fviernau fviernau requested a review from a team as a code owner August 1, 2024 15:03
@fviernau fviernau changed the title Further NuGet curation fixes NuGet curation fixes Aug 1, 2024
@fviernau fviernau mentioned this pull request Aug 1, 2024
@@ -1,5 +1,10 @@
- id: "NuGet:Microsoft.Net:Http.Headers:[2.1.7,)"
- id: "NuGet:Microsoft.Net:Http.Headers:[3.0.0,)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the ranges here and in line 6 need to be swapped to keep path: "src/Http/Headers/src" for versions < 3.0.0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 3.0.0 and above the path should be fine, see https://github.com/dotnet/aspnetcore/tree/v3.0.0/src/Http/Headers.

For below 3.0.0 releases come from different repo, see https://github.com/aspnet/HttpAbstractions/tree/2.2.0/src/Microsoft.Net.Http.Headers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused. The existing curation sets the path to "src/Http/Headers/src" for version 2.1.7 and higher. I'm assuming that curation to be correct, at least exactly for version 2.1.7. But your new curation sets the path to "src/Microsoft.Net.Http.Headers" for version 3.0.0 and lower, including version 2.1.7. So is the existing curation wrong for version 2.1.7?

Copy link
Member Author

@fviernau fviernau Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! There is something wrong with the curations.
Version 2.1.7 does not even exist, but I believe I got it now.
It helps to look at the list of available versions and release dates here [1].

Releases are as follows:

I've updated the curation accordingly. Looks plausible to me now...

[1] https://www.nuget.org/packages/Microsoft.Net.Http.Headers/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To double-check, I've looked at the following link which have the package information in the lower left:

So that curation now looks good to me as well.

curations/NuGet/System.Drawing/Common.yml Outdated Show resolved Hide resolved
@fviernau fviernau force-pushed the nuget-curation-fixes-2 branch 2 times, most recently from d40bd8c to f9520c7 Compare August 27, 2024 06:47
Copy link
Member

@sschuberth sschuberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about splitting you all pure formatting commits out to a separate PR until those version 2.1.7 path issues are sorted out for all affected packages?

comment: "Set the VCS path of the module inside the multi-module repository."
vcs:
path: "src/Security/Authentication/Core/src"
path: "src/Microsoft.AspNetCore.Authentication"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again the same problem with version 2.1.7?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought it's the same problem again as from a pure text-diff perspective of a reviewer the path curation for version 2.1.7 is now different than before. The commit message says "Several targeted versions do not exist", but I would have wished this to be more explicit and name the non-existing versions.

Several targeted versions do not exist, see [1]. Note that versions
lower or equal to 2.2.0 have been released from [2], and later one from
[3], with the exception that 2.1.11 through 2.1.31 have been release
from [3].

[1] https://www.nuget.org/packages/Microsoft.AspNetCore.Mvc.Abstractions
[2] https://github.com/aspnet/Mvc/tree
[3] https://github.com/dotnet/aspnetcore

Signed-off-by: Frank Viernau <[email protected]>
@fviernau
Copy link
Member Author

How about splitting you all pure formatting commits out to a separate PR until those version 2.1.7 path issues are sorted out for all affected packages?

I went through all curations and found 2 more "version 2.1.17 like" issue which I fixed. I believe the curations are fine now.

comment: "Set the VCS path of the module inside the multi-module repository."
vcs:
path: "src/Security/Authentication/Core/src"
path: "src/Microsoft.AspNetCore.Authentication"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought it's the same problem again as from a pure text-diff perspective of a reviewer the path curation for version 2.1.7 is now different than before. The commit message says "Several targeted versions do not exist", but I would have wished this to be more explicit and name the non-existing versions.

@sschuberth sschuberth merged commit 391acb1 into main Aug 28, 2024
2 checks passed
@sschuberth sschuberth deleted the nuget-curation-fixes-2 branch August 28, 2024 08:58
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.

3 participants