Follow ups regarding the Spring package curation provider

[sschuberth]

Please have a look at the individual commit messages for the details.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.39%. Comparing base (be610e3) to head (c4b0275).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #11108   +/-   ##
=========================================
  Coverage     57.38%   57.39%           
+ Complexity     1703     1701    -2     
=========================================
  Files           346      346           
  Lines         12825    12826    +1     
  Branches       1214     1214           
=========================================
+ Hits           7360     7361    +1     
  Misses         4997     4997           
  Partials        468      468           

Flag	Coverage Δ
funTest-no-external-tools	31.06% <0.00%> (-0.11%)	⬇️
test-ubuntu-24.04	42.46% <100.00%> (+<0.01%)	⬆️
test-windows-2025	42.44% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fviernau]

Why should this be enabled by default?

This changes the standard workflow to on the fly generated curations, which might break some workflows such as replacing curations in an ORT result and makes it harder to figure out where some curation came from. So, I wonder if it's better to leave this disabled?

[MarcelBochtler]

Why should this be enabled by default?

The functionality which was moved to this plugin, was enabled by default before this change.
IMO it should be enabled by default, so provide a seamless upgrade for users.

[sschuberth]

Why should this be enabled by default?

As written in the commit message, because it would be a breaking change / change in behavior otherwise. The plugin now contains functionality that we previously hard-coded in the package managers, so the plugin needs to be enabled by default to maintain the same behavior.

which might break some workflows such as replacing curations in an ORT result

How exactly would that workflow be broken?

makes it harder to figure out where some curation came from

As the provider of the package curation is by now recorded as part of the resolved configuration in an ORT result, I do not think this is an issue.

[fviernau]

It was clear to me that the functionality was there before. But not exactly, as it now generates curations on-the-fly. So, I was after a rationale beyond "because it was there before" (because again, curations actually weren't generated before, or?).

[sschuberth]

because again, curations actually weren't generated before, or?

No, they weren't. The "because it was there before" rationale referred to commit 3bc2369 only.

I don't really see a risk in 9154423 now also making the "curations on-the-fly" feature enabled by default. I guess if someone wants Spring curations, that someone would want all of it, and not just parts of it.

In the worst case, we could still hide the "curations on-the-fly" feature behind a plugin option.

[sschuberth]

@fviernau, you were only asking (rhetoric?) questions, but not requesting any actual changes. So was your intention really to submit the review with "Request changes", or should it have been "Comment" only?

[fviernau]

So was your intention really to submit the review with "Request changes", or should it have been "Comment" only?

My intention was to clarify, whether we really want to start generating curations on-the-fly before merging this PR.

[sschuberth]

So was your intention really to submit the review with "Request changes", or should it have been "Comment" only?

My intention was to clarify, whether we really want to start generating curations on-the-fly before merging this PR.

And to block the PR until your questions are answered? I'm asking because, since we cannot make any assumptions about your availability, it might take some time for you to dismiss your review in this case after your questions have been answered, and during that time the merge would still be blocked.

So unless you really want to block a PR until your questions are answered, I recommend to submit a review with "Comment" only.

[fviernau]

And to block the PR until your questions are answered? I'm asking because, since we cannot make any assumptions about your availability, it might take some time for you to dismiss your review in this case after your questions have been answered, and during that time the merge would still be blocked.

So unless you really want to block a PR until your questions are answered, I recommend to submit a review with "Comment" only.

1. Regarding my availability and whether I should "comment only" on PRs, I don'T feel this is the right place to discuss. Please raise it in a meeting.
2. Regarding the "on-the-fly" approach, I felt this should have some consensus amongst core devs, and I saw prev PR was merged relatively quickly.

If feel my change request is offending you somehow, but I do not understand why. Important to also note, so far there has not been any issue with any delay here whatsoever. All in all, I feel above comments are the exact opposite of being appreciative of my reviews.

[MarcelBochtler]

So was your intention really to submit the review with "Request changes", or should it have been "Comment" only?

My intention was to clarify, whether we really want to start generating curations on-the-fly before merging this PR.

The functionality to do this was already merged in #11102.
The change in this PR would avoid that ORT users are negatively affected by an oversight in #11102.

How about discussing reverting #11102 at a later point but still merge this PR to not negatively affect our users?

[fviernau]

And to block the PR until your questions are answered? I'm asking because, since we cannot make any assumptions about your availability, it might take some time for you to dismiss your review in this case after your questions have been answered, and during that time the merge would still be blocked.

Yes, of course I wanted to have it be blocked until clarified.

So unless you really want to block a PR until your questions are answered, I recommend to submit a review with "Comment" only.

This is clear - We usually have blocking change requests for typos in commit messages, grammer mistakes and other nits, so why shouldn't we have them for introducing the concept of on-the-fly generated curations (by default). Isn't this a bit double standards.

[fviernau]

@fviernau, you were only asking (rhetoric?) questions, but not requesting any actual changes. So was your intention really to submit the review with "Request changes", or should it have been "Comment" only?

From my observations in ORT community, asking a question with a "change request" is a commonly used practice. Also you do this IIUC.

I'm discouraged to continue contributing here.

[sschuberth]

whether I should "comment only" on PRs, I don'T feel this is the right place to discuss. Please raise it in a meeting.

I've put it on the agenda for the next TSC meeting on 2025-12-02.

[sschuberth]

From my observations in ORT community, asking a question with a "change request" is a commonly used practice.

I don't share that observation.

Also you do this IIUC.

In my view, if the only thing I do is asking questions in a review, then I'm not requesting a change, and if I anyway selected "Request changes" when submitting a review with only questions, then that's a mistake on my side, and I'm happy to get hinted at that so I can correct it.

[sschuberth]

Merging despite the unrelated test failure(s).