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

Don't lock in a specific version of Lucene in the help feature #230

Closed
merks opened this issue May 19, 2023 · 24 comments
Closed

Don't lock in a specific version of Lucene in the help feature #230

merks opened this issue May 19, 2023 · 24 comments

Comments

@merks
Copy link
Contributor

merks commented May 19, 2023

The help feature includes Lucene bundles.

<plugin
id="org.apache.lucene.analysis-common"
download-size="0"
install-size="0"
version="0.0.0"
unpack="false"/>
<plugin
id="org.apache.lucene.core"
download-size="0"
install-size="0"
version="0.0.0"
unpack="false"/>
<plugin
id="org.apache.lucene.analysis-smartcn"
download-size="0"
install-size="0"
version="0.0.0"
unpack="false"/>

This locks in a very specific version. The only bundle that actually requires Lucene allows a range of versions:

image

Does anything speak against removing the includes?

@akurtakov
Copy link
Member

Go for it for next M1

@merks
Copy link
Contributor Author

merks commented May 19, 2023

The problem is that we've been on the "everyone use the latest" path and now people want to to use 9.5.0, e.g., Mylyn wants that. Of course people also want the Platform in their target platform and they want to be able to launch a self-hosted IDE, which means they have multiple versions of Lucene in their target platform, which is a complete nightmare mess...

Anyway, let's first see if this breaks anything...

@laeubi
Copy link

laeubi commented May 19, 2023

I might be wrong but last time I suggested such changes it was argued that:

  1. source bundles might not be included
  2. bundle might not be included in update-sites

but if this is no longer an issue I think actually any third party dependency can be removed from platform features making them much more flexible (less version bumps and so on...)

@merks
Copy link
Contributor Author

merks commented May 19, 2023

Includes have been removed all over the place. That's been the general trend. It's even been suggested that all 3rd party dependencies be removed from includes in general. In other projects I've created a "dependencies"feature and put 3rd party bundles there to include them easily in the update site without including them in any user-installed feature...

@merks
Copy link
Contributor Author

merks commented May 19, 2023

FYI, in this issue we are discussing the details around using an m2e target location to create wrappers for Lucene 9.5.0...

merks/simrel-maven#5

Will the Platform be able/willing to upgrade to such a new version of Lucene?

@akurtakov
Copy link
Member

Lucene upstream jars are not osgi so still Orbit wrappers are used and I've been the one doing the last few. So if there are already created wrappes that would be more than welcome.

@laeubi
Copy link

laeubi commented May 19, 2023

Is orbit on github already? If not won't it be better to move there, enable dependabot, getting dependency updates notification, and so on? Or is it now the goal to replace orbit with some "oomph maven wrapped bundles" service?

@merks
Copy link
Contributor Author

merks commented May 19, 2023

I'm waiting for these reviews to complete, though the Platform doesn't use these specifically:

https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/8488
https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/8486

But yes, I already have drop-in replacement wrappers for Lucene 9.5.0 that I've been testing with Mylyn and with DataTools.

I'm testing with a self hosted launch with these new Lucene wrappers, but there are other problems that seem unrelated:

image

The Help view appears to work, but I don't know if that tells me anything...

@merks
Copy link
Contributor Author

merks commented May 19, 2023

Orbit is here:

https://github.com/eclipse/orbit

But they want to use this organization:

https://github.com/eclipse-orbit

Let's discuss goals for Orbit elsewhere. Here are their issues:

https://github.com/eclipse/orbit/issues

@laeubi
Copy link

laeubi commented May 19, 2023

I don't think this is an orbit issue in general, you asked if platform can use the "whatever named new service" and platform already using orbit, so why not update the orbit versions so it seems not really we need a decision then?

@merks
Copy link
Contributor Author

merks commented May 19, 2023

It sounds like you want to discuss eclipse-orbit/orbit#8 here where few will see it so I suggest you discuss Orbit's plans on that Orbit issue where many will see the discussion.

@HannesWell
Copy link
Member

I also think we should remove the lucense bundles from the feature.

Lucene upstream jars are not osgi so still Orbit wrappers are used and I've been the one doing the last few. So if there are already created wrappes that would be more than welcome.

The issue to request that is now on GH: apache/lucene#8573

It looks like lucene removed the split-packages with version 9:
https://github.com/apache/lucene/blob/main/lucene/MIGRATE.md#migration-from-lucene-8x-to-lucene-90
This would make upstream metadata much simpler.
From my preliminary analysis the only difficulty left is the use of ServiceLoader, but this can be tackeled with the usual ServierLoader Medaitor approach. Or one could use fragments, but I have not yet checked if that fits well into the existing bundle structure.
The biggest problem I have, is that they use Gradle and I don't know Gradle. Does anybody of you know it and can help to configure the bnd-gradle-plugin?

@merks
Copy link
Contributor Author

merks commented May 24, 2023

This locking in of a specific gson version is also annoying:

image

This is made worse by the fact that the metadata of the direct-from-maven bundle is different from that of the orbit wrapped bundle:

image

And lsp4j appears to be relying on the internal packages. But that's another problem...

@laeubi
Copy link

laeubi commented May 24, 2023

This locking in of a specific gson version is also annoying:

Also I'm quite sure you are aware of it I just wanted to note for others that no one is really "locking" a version in a feature but this is an intrinsic feature of how feature.xml works, what is that included plugins/features are resolved (and replaced) by the version of the resolved build time artifact if not already specify a hard version requirement.

Last time I'd attempted to change that it resulted in a lot of complaints and was claimed that no one needs this and it behaves like every one wants it, so its a bit strange to complain now about this, the only solution would be to strip the gson feature completely...

@merks
Copy link
Contributor Author

merks commented May 24, 2023

Yes indeed, features are doing exactly what they are intended to do. I don't think we should change that because that's how an include of a bundle in a features is expected to work and how it's supposed to work.

In this issue I'm not complaining about the behavior---I'm not even complaining---I'm suggesting to remove the include, and asking what speaks against that.

It's been suggested in general to remove includes of 3rd party bundle includes from features:

eclipse-orbit/orbit#8 (comment)

As you've noted previously, now that Tycho supports many more useful things such as automatically including source bundles for otherwise included non-source bundles there seems to be less that speaks against this approach. (I suppose there might remain a concern about whether specific sources are installed in the Eclipse SDK, but no one has mentioned that.) Often other projects want specific 3rd party dependencies in their update site, but that can be accomplished by specifying the bundles directly in the category.xml or by creating a dependencies feature (one that the user does not install), including the bundles there, and including that in the category.xml. I have taken these approaches with several other projects, and specifically recently with Mylyn...

But I fully understand @akurtakov concern that it's rather late in the cycle to alter this without risk.

@mickaelistria
Copy link
Contributor

I'm suggesting to remove the include, and asking what speaks against that.

I support this approach. Inclusion in features now seems like a pre-p2 utility when features where distributed as zips to unpack in the eclipse installation; but with p2 fetching transitive deps and Platform building its repo with includeAllDependencies to include all these transitive deps, feature inclusions are for Platform most often a source of problems and extra-work than a help.
Removing the problem is 10x more valuable than trying to workaround it.

@akurtakov
Copy link
Member

For the record, my main concern is not the general one about removal of Lucene from feature.xml but rather the fact about opening Help system to get new Lucene versions updated - Lucene is very strict with index versions (every minor version gets new version), Help system supports single version only and if new Lucene version is pushed by another plugin that would render would all pre-built help indexes useless causing user visible warnings about that, projects having to rebuild their documentation bundles and etc.
With all that said - Lucene is pretty fast at indexing already so these pre-build indexes served their point so a good solution might be to stop shipping pre-build indexes and let them be indexed on first usage.

@laeubi
Copy link

laeubi commented May 24, 2023

I'm suggesting to remove the include, and asking what speaks against that.

Nothing special, but if you remove the gson bundle from the gson feature whats left there? :-)

@merks
Copy link
Contributor Author

merks commented May 24, 2023

@mickaelistria

Indeed, I've grown tried of doing qualifier bundles when updating the *.target!

@laeubi

There is no gson feature there is a tips feature:

image

@akurtakov

Yes, I wondered about the indexing, because I've seen issues about such warnings. Consider this though:

image

Just because the feature requires to install a specific version does not mean that the bundle itself will be wired to that version. It's happy with either version. So yes, I think it would be better to stop pre-indexing even if we didn't remove the include...

@akurtakov
Copy link
Member

Yes, you're correct about the resolution of lucene to newer version if available, still I find it a bit risky to do now as the Help system is not fun to debug and fix at all if smth breaks.
My vote goes for stop pre-indexing, deprecate the extension point and remove Lucene from the feature.xmls for 2023-09.

@laeubi
Copy link

laeubi commented May 24, 2023

Alright I was confused because it uses an icon very similar to the feature icon here:
grafik

@merks
Copy link
Contributor Author

merks commented May 24, 2023

@laeubi

No problem! There's so much detail!!

@akurtakov

I agree. We've lived with Lucene duplicates for a long time, so it's safest to wait for the next release rather to change things during RC1.

merks added a commit to merks/eclipse.platform.releng that referenced this issue Jun 14, 2023
merks added a commit to merks/eclipse.platform.ua that referenced this issue Jun 14, 2023
The  org.eclipse.tips.json bundle allows a range:

Import-Package: com.google.gson;version="[2.8.6,3.0.0)"

eclipse-platform/eclipse.platform.releng#230
merks added a commit to eclipse-platform/eclipse.platform.ua that referenced this issue Jun 14, 2023
The  org.eclipse.tips.json bundle allows a range:

Import-Package: com.google.gson;version="[2.8.6,3.0.0)"

eclipse-platform/eclipse.platform.releng#230
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jun 14, 2023
merks added a commit to merks/eclipse.platform.releng that referenced this issue Jun 14, 2023
@merks
Copy link
Contributor Author

merks commented Jun 14, 2023

FYI,

I will meet with @jonahgraham next week to discuss Orbit. Then we can hopefully build new versions of Lucene 9.6 which we can contribute to the target platform...

@merks merks closed this as completed Jun 14, 2023
@HannesWell
Copy link
Member

I will meet with @jonahgraham next week to discuss Orbit. Then we can hopefully build new versions of Lucene 9.6 which we can contribute to the target platform...

Sounds good, I assume this also includes your new Maven-SimRel-Orbit and the general handling of Maven-targets?
Looking forward to the results. I already thought about, if it would be helpful to provide a little presentation/demo on how to set up/use Maven target properly in an Eclipse project that contributes to the SimRel.
Maybe this would convince more projects to use Maven-Targets directly.

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

No branches or pull requests

5 participants