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

Correct required Apache Lucene bundles version ranges #1435

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

howlger
Copy link
Contributor

@howlger howlger commented Jun 18, 2024

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.0

So the minimal Apache Lucene version has to be 9.5.0, not 9.4.2.

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.
@akurtakov
Copy link
Member

Thanks for catching this up!

Copy link
Contributor

Test Results

 1 734 files  ±0   1 734 suites  ±0   1h 25m 1s ⏱️ -37s
 3 970 tests ±0   3 948 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 507 runs  ±0  12 346 ✅ ±0  161 💤 ±0  0 ❌ ±0 

Results for commit c876d3c. ± Comparison against base commit a5bbdf4.

@iloveeclipse
Copy link
Member

In my platform I see org.apache.lucene.core 9.11.0, so why only 9.5.0?

@merks
Copy link
Contributor

merks commented Jun 18, 2024

In my platform I see org.apache.lucene.core 9.11.0, so why only 9.5.0?

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.

@iloveeclipse
Copy link
Member

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)?

@merks
Copy link
Contributor

merks commented Jun 18, 2024

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?

@iloveeclipse
Copy link
Member

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).

@merks
Copy link
Contributor

merks commented Jun 18, 2024

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.

@Bananeweizen
Copy link
Contributor

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).
So the only easy way out is updating all minimum versions in all manifests whenever an upgrade of a dependency happens. This may break compatibility with other plugins of course. However, in my experience it's not that much of a problem in practice, if you either rely on Orbit and the release train (where dependencies are unified), or if you include all the dependencies (as latest version) that you require in your own code (what we do for our custom products).

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...

@merks
Copy link
Contributor

merks commented Jun 18, 2024

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.

@laeubi
Copy link
Contributor

laeubi commented Jun 19, 2024

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.

  1. Tycho computes a target platform for each project separately, so it is possible that two bundles are using a different "lower" bound
  2. Tycho can (and do) compute different dependency sets so it would be possible to compile against the lowest bound (and either warn, fail, ... if not found) and run (e.g. tests) against the latest, that usually gives a quite good compromise.
  3. If one wants, it would even be possible to compile against all matching versions (if it is a maven dependency we can even determine all version automatically, for P2 one would need a composite repo), or use bytecode inspection like it is already done by e.g. API tools (just that we API check the dependencies).
  4. here are some relevant Tycho issues:

@akurtakov
Copy link
Member

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.

@akurtakov akurtakov merged commit a7b2918 into eclipse-platform:master Jun 19, 2024
16 checks passed
@howlger
Copy link
Contributor Author

howlger commented Jun 19, 2024

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 org.eclipse.help.base will work with all Apache Lucene 9.x versions equal to or greater than 9.5.0 might be too optimistic. Therefore, I looked at whether backwards compatibility has been broken in the past, assuming that what has happened in the past is more likely to happen in the future again. But I have not found any API breakage. Also, the Apache Lucene change log says that things have been marked as deprecated, but not that deprecated things have been removed in the 9.x stream yet. Everything looks fine to me.

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.

@howlger howlger deleted the patch-2 branch June 19, 2024 10:25
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.

6 participants