-
Notifications
You must be signed in to change notification settings - Fork 112
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
Correct required Apache Lucene bundles version ranges #1435
Conversation
Since commit 5f4a41c (<eclipse-platform@5f4a41c>) the class `org.apache.lucene.index.StoredFields` is used which does not exist in Apache Lucene 9.4.2: https://lucene.apache.org/core/9_4_2/core/org/apache/lucene/index/package-summary.html but only since Apache Lucene 9.5.0: https://lucene.apache.org/core/9_5_0/core/org/apache/lucene/index/StoredFields.html#document(int) So the minimal Apache Lucene version is 9.5.0.
Thanks for catching this up! |
In my platform I see |
If I read the first comment carefully l, think it’s setting the minimum valid minimum not simply setting the minimum to the current version. |
Yes, but why should we require old one if there is already new one available & working (at least in my installation)? |
Allow is not the same as require. In general we do not update lower bounds every time we update the version in the target platform. In general lower bounds are supposed to be as low as correct but no lower. So the opposite question: why require 9.11 when we know 9.10 works because we shipped that for the 4.32 release. And if there is a good reason, should we apply it consistently in all cases, not just this case? |
Simply because every 3rd party dependency might have security or other issues etc that could have been fixed in latest versions? So yes, in general, to keep platform up-to-date shouldn't we proactively update all dependencies? (Of course if that doesn't break anyone else). |
We do update dependencies and ship those updated dependencies. That’s a different issue from changing lower bounds. Lower bounds should be about correctly and accurately constraining the version range based on semantics and compatibility. This reminds me of the attempt to increase EMF’s version range on the platform core runtime to require the one with security fixes. I refused to do that because it breaks my consumers. Anyway, I’m not sure who wants to create and implement lower bound updates. I have more than enough to do keeping the dependencies updated without pawing through hundreds of bundles looking forward lower bounds in need of increase. |
Just as a note: In my experience deriving a valid minimum is a hard task. This issue found that 9.4.2 is not valid because it doesn't contain a method that's currently called. However, that does not mean 9.5 is good enough, but only that nobody has found a method being called in the current platform code which doesn't exist in 9.5 yet. I'm not aware of another method than creating a target platform that really contains the minimum versions of all dependencies (which is even unsolvable, if different minimum versions of the same dependency are required by 2 components). That is not meant to question any current handling of version numbers in the platform. Basically just saying you get some drawback with either choice of minimum version strategy... |
There is definitely some merit in increasing lower bounds to match the current “context”. That applies not just to the 3rd party dependencies but also the cross dependencies within the platform which tend to be neglected and actually wrong. Every change should consider, in which version is the thing I’m starting to use first available. But no one does that consistently always. And no tool helps solve that problem. (That being said, when we publish the platform to maven central, using the cbi aggregator, we do exactly this step of mapping dependencies to the actual current version.) Speaking of tools, Orbit has tools to automate updates to 3rd party dependencies. That is what I use to keep Orbit and the Platform updated. And that’s what makes the activity scalable. But there is no tool, yet, to update the bounds in bundles and features. I will definitely not tackle that task as a manual effort. It’s a task for a monkey 🐵 no insult to the monkey intended. 😝 So my suggestion would be to merge this change and separately discuss how best to manage lower bounds in general. We should define a policy along with the reasoning for the policy. And we will need tools to implement the policy because skilled monkeys are in short supply. Or we can pretend the light did not shine in this dark corner, live in ignorant bliss, and work on other cool things or other problems, neither of which will ever be exhausted. |
I just wanted to note that the problem actually is solvable (whatever we choose) I just haven't worked on that with higher priority because (until now) because it seems more like a personal preference than a wider required feature.
|
As this PR already fixes invalid metadata and the rest of the discussion is about further improvement to the processes I'm merging this one. Please move further discussions to dedicated issue/PR. |
Thanks for applying the pull request and all the comments! The wrong minimal version has not caused any problems for me nor, as far as I know, for anyone else. I just stumbled across it while I was looking at something else. By the way, creating this pull request via GitHub's web interface was quick and easy. The one I looked into was the upper bound of the version range. Apache Lucene does not offer OSGi bundles directly, so I assume there is no guarantee of backwards compatibility if the minor version number is increased, which means that the assumption that Larger version ranges offer more flexibility to combine bundles without having to modify them. I also appreciate the ability to use bundles of different versions in OSGi, which is not possible with a flat classpath. There's a trade-off between the the effort to maintain a larger version range and the flexibility. Automatically checking the version ranges would be a great improvement to capture the corner cases that are otherwise will be missed like in this case. |
Since 5f4a41c the class
org.apache.lucene.index.StoredFields
is used which does not exist in Apache Lucene 9.4.2, but only since Apache Lucene 9.5.0So the minimal Apache Lucene version has to be 9.5.0, not 9.4.2.