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

curations: Update curations for Nuget packages #250

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mnonnenmacher
Copy link
Member

Update the curations by running the generation tasks in tools/curations.

Update the curations by running the generation tasks in
`tools/curations`.

Signed-off-by: Martin Nonnenmacher <[email protected]>
@mnonnenmacher mnonnenmacher requested a review from a team as a code owner November 6, 2024 10:09
@mnonnenmacher mnonnenmacher enabled auto-merge (rebase) November 6, 2024 10:11
@sschuberth
Copy link
Member

IIRC, @fviernau did some corrections to these generated curation in #225. I hope these do not get overwritten now.

@mnonnenmacher
Copy link
Member Author

IIRC, @fviernau did some corrections to these generated curation in #225. I hope these do not get overwritten now.

That's really unfortunate, why fix the generated code instead of the generator? Also, it appears to me that the "fixes" did not fix any real issues as curations for non-published packages do not do any harm except for wasting a little space.
I won't have time to go through the >200 files anytime soon.

@sschuberth
Copy link
Member

curations for non-published packages do not do any harm

Then maybe I'm mistaken, as I recall during the review that (some of) these packages are published. @fviernau can probably tell better.

@mnonnenmacher
Copy link
Member Author

IIRC, @fviernau did some corrections to these generated curation in #225. I hope these do not get overwritten now.

That's really unfortunate, why fix the generated code instead of the generator? Also, it appears to me that the "fixes" did not fix any real issues as curations for non-published packages do not do any harm except for wasting a little space. I won't have time to go through the >200 files anytime soon.

I had a look at one single package (NuGet:Microsoft.AspNetCore:Authentication) and it appears there is some problem with the generator. However, I still don't understand why this was not addressed by fixing the generator, the manual fixes probably missed many packages?

@mnonnenmacher mnonnenmacher marked this pull request as draft November 6, 2024 10:40
@fviernau
Copy link
Member

fviernau commented Nov 7, 2024

@mnonnenmacher I have a hunch you do not really understand the fixes I've made, so
why judging about the approach without asking the question why? e.g.:

That's really unfortunate, why fix the generated code instead of the generator?

The effort it took to investigate the issues caused by all these wrong curations was that huge,
that I would have probably been faster if they were not existent at all.

Also, it appears to me that the "fixes" did not fix any real issues as curations for non-published packages do not do any harm except for wasting a little space.

Each of these fixes did fix any real issues, in contrast to the generated curations, where many of them seems to
have been introduced for non-real issues.

However, I still don't understand why this was not addressed by fixing the generator, the manual fixes probably missed many packages?

Because the approach how these are generated does not work, and coming up with a better approach would mean redo it from scratch.

Please, let's avoid generating more broken curations with this task.
I could help with pointing out the problems with the existing approach, if someone is willing to put effort into implemting a fix.

(A short summary of some of the fixes: Some packages have been released from multiple repositories. The fixes ensure that the vcs URL points to the repo the release has been created from, so that the SHA1 in the metadata
can be found in the repository. The generated ones did not always accomplish that).

curations:
comment: "Set the VCS path of the module inside the multi-module repository."
vcs:
path: "src/Http/Http/src"
- id: "NuGet:Microsoft.AspNetCore:Http:(2.1.34,3.0.0["
- id: "NuGet:Microsoft.AspNetCore:Http:[2.1.22,2.1.34]"
Copy link
Member

Choose a reason for hiding this comment

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

Please do not revert my fixes.

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.

4 participants