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

Remove SLF4J Plugin from all features #61

Closed
wants to merge 1 commit into from
Closed

Remove SLF4J Plugin from all features #61

wants to merge 1 commit into from

Conversation

HannesWell
Copy link

If a Feature includes a Plugin, it usually includes it with a specific name and the version that was in the TP when the feature was build. This prevents consumers from using a slf4j bundle with different symbolic-name or different version in their TP or product.

This is part of eclipse-platform/eclipse.platform.releng.aggregator#588

@eclipse-rap-bot
Copy link

Can one of the admins verify this patch?

@ifurnadjiev
Copy link
Contributor

ok to test

@ifurnadjiev
Copy link
Contributor

SLF4J is needed by Jetty. Removing the bundle makes impossible to start Jetty with RAP target platform. @mknauer probably we can use the new bundle symbolic name slf4j-api in the features without version constraint?!

@HannesWell
Copy link
Author

HannesWell commented Oct 4, 2022

SLF4J is needed by Jetty. Removing the bundle makes impossible to start Jetty with RAP target platform. @mknauer probably we can use the new bundle symbolic name slf4j-api in the features without version constraint?!

Even if you specify 0.0.0 as a version, Tycho will insert the version that is in the Target-Platform at build time. You could make slf4j a dependency of the feature, but that will bind it again to a specific bundle-SymbolicName.
On the other hand if a slf4j package is imported by a Plugin/Bundle it can be satisfied by any source (that is in the specified version range) and Tycho/P2/PDE will pull in a matching version of sl4fj into a Product or Target-Platform in the workspace.

If you want to build a self contained p2-repo for rap you could use includeAllDependencies of the tycho-p2-repository-plugin. Or if that is not suitable (e.g. because you want to split that over multiple repos), you could consider to simply not contribute the target-feature to SimRel. The SimRel repo is self-contained and during its creation missing requirements of Plug-ins will be pulled in from any repository available to SimRel. If multiple sources are available the usual selection rules are applied, the number of bundles is minimized and the latest version is selected.

@mknauer
Copy link
Contributor

mknauer commented Oct 5, 2022

We'll leave it for 3.23M1 as it is and will consider it again for M2/M3.
At the moment, we are discussing several changes in that area, one of them is replacing the way we consume the Jetty bundles.

@HannesWell
Copy link
Author

We'll leave it for 3.23M1 as it is and will consider it again for M2/M3. At the moment, we are discussing several changes in that area, one of them is replacing the way we consume the Jetty bundles.

I would appreciate if this is considered until M2. Then we have more time to address problems until M3.
If you want to keep slf4j in the rap repo for now, I can simply add it to the corresponding repo/category.xml file directly. Then it is removed from the feature but remains in the repo and can be fetched by consumers that need it from the rap repo.

If a Feature includes a Plugin, it usually includes it with a specific
name and the version that was in the TP when the feature was build.
This prevents consumers from using a slf4j bundle with different
symbolic-name or different version in their TP or product.

This is part of
eclipse-platform/eclipse.platform.releng.aggregator#588
@HannesWell
Copy link
Author

Any updates in this regard?

@mknauer mknauer self-assigned this Nov 15, 2022
@mknauer
Copy link
Contributor

mknauer commented Nov 15, 2022

Maybe it helps to look at the idea behind the "RAP Basic Equinox Target" feature.

It has been created and is still maintained as a feature that a developer can use as a minimal set of meaningful bundles for developing RAP applications. It includes the RAP Runtime, Jetty for handling all the http stuff, and all of that is bound together by Equinox as OSGI runtime (speaking of that, we used to have other variants based on Apache Karaf, Felix, etc.). It lives in a separate Simultaneous Release validation set in order to minimise the interference of some RAP implementations with their RCP counterparts, and it happily redistributes the exact same versions of bundles that are used in the Eclipse Platform and in the Simultaneous Release.

Therefore it is not meant to create a self contained p2-repo, but its goal is to offer developers a self contained feature, and that requires adding the slf4j.api bundle to the feature, and removing the old one from Orbit, of course.

I will update the commit of this PR accordingly, including changes in various other files, e.g. launch configurations. And I will update RAP Tools in the same way.

@mknauer mknauer changed the title Remove SLF4J Plugin from all features Synchronise SLF4J plugin to Eclipse Platform Nov 15, 2022
@mknauer mknauer changed the title Synchronise SLF4J plugin to Eclipse Platform Remove SLF4J Plugin from all features Nov 15, 2022
@mknauer
Copy link
Contributor

mknauer commented Nov 15, 2022

To confuse everyone as much as I can... no, sorry for that, but I didn't find a better way in GitHub as to create a new separate pull request that should replace this one.

In fact, there are two pull requests, one for each project:

These pull request contain the change of the bundle symbolic name.

@HannesWell
Copy link
Author

Understand. Thank you for the elaboration. An Installable-Unit location in a Target-Definition can be configured to include (transitive) dependencies of all specified Plugins and Features and I assume that for example Jetty requires slf4j so it will be pulled into a TP anyway. But you probably know that and want to have a self-contained Feature nonetheless.

Updating the Features to include the new slf4j bundle is also fine since it prevents contributing the old one to SimRel.
Thank you for creating the PRs to do the other necessary change.
This PR can then be closed. I will link your two new PRs instead in the umbrealla issue where the SLF4J migration is tracked.

@HannesWell HannesWell closed this Nov 15, 2022
@HannesWell
Copy link
Author

And since you mentioned that you have to adjust launch configurations, you can also configure them to include required Features and Plugins automatically upon launch:
https://www.eclipse.org/eclipse/news/4.24/pde.php#auto-add-requirements-launches

Maybe that is interesting for you as well.

@mknauer
Copy link
Contributor

mknauer commented Nov 15, 2022

Thank you!

And thank you for bringing up the last idea about the launch configurations, we may consider this. Although we tried our best in the past to keep them all in sync from release to release, they deviated sometimes from reality, and ticking that checkbox would prevent most of these cases.

@HannesWell
Copy link
Author

HannesWell commented Nov 15, 2022

And thank you for bringing up the last idea about the launch configurations, we may consider this. Although we tried our best in the past to keep them all in sync from release to release, they deviated sometimes from reality, and ticking that checkbox would prevent most of these cases.

You're welcome. :)
Yes syncing them manually is quite a lot a tedious work. Therefore I try to reduce my products and similar the (associated) launch configs down to the roots of the dependency tree, i.e. to the Features/Plugins I really want in my Product or launch and let PDE/P2 add all required dependencies on demand when launching or assembling them. This way a launch/product is always up to date. And that works very well as long as due to Inversion-Of-Control something needed is contributed 'from outside'. Such IUs then have to be added as well.

@HannesWell HannesWell deleted the removeSLF4JFromFeatures branch August 19, 2023 10:55
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