search

eclipse / elk

Eclipse Layout Kernel - Automatic layout for Java applications.
https://www.eclipse.org/elk/
Other
258 stars 86 forks source link

Remove re-exports in MANIFEST.MF and use Import-Package where suitable #1074

Closed HannesWell closed 1 month ago

HannesWell commented 2 months ago

Re-exporting required bundles is convenient in the first place but can cause a lot of trouble on the long run. When reexporting a required bundle that bundle effectively becomes part of the exporting bundle's API with all the consequences regarding versioning. Additionally in an OSGi runtime it can cause resolution errors if multiple versions are available. Because of all that using reexports is not recommended and should be avoided.

This PR suggests to do that for ELK and removes all reexports and adds the therefore necessary explicit requirements instead.

soerendomroes commented 2 months ago

Thank you for your PR. I have no time to look into this now, but would happily help with this PR in October. And I have to check our own upstream dependencies.

It seems that the removed reexports now result in missing dependencies.

HannesWell commented 2 months ago

It seems that the removed reexports now result in missing dependencies.

Unfortunately yes. But I did some experiments locally and it looks like that it works without version-ranges. But since having them is actually a good thing and recent versions of Tycho support that very well I think a Tycho update should fix that. Is there a specific reason Tycho is not updated for some time or is it just because nobody did it yet? If you are interested in that I can look into it too.

soerendomroes commented 2 months ago

Is there a specific reason Tycho is not updated for some time or is it just because nobody did it yet? If you are interested in that I can look into it too.

None I can think of right now. Maybe we just did not do it yet. So we are interested if you can look into this.

soerendomroes commented 2 months ago

On a second thought we typically wait for Xtext to change they tycho version to be compatible with what they are doing.

HannesWell commented 1 month ago

I have now found the cause for the build failure: In the current Target-Platform you are using the old Guice 5.1 artifact whoose metadata are generated from the old Eclipse Orbit. And Orbit generated the versions of the exported packages in version 5, matching the bundle-version, while the original Guice artifacts from Maven-Central export them with version 1.4 (and follow Semantic-Versioning).

Since in more recent releases the original artifacts are used, I have now reverted the change to use Import-Package instead of require bundle for Guice. It can be done later, when updating the TP.

This now only removes the re-exports and the build now passes.

On a second thought we typically wait for Xtext to change they tycho version to be compatible with what they are doing.

Xtext uses Tycho 4.0.9 at the moment, https://github.com/eclipse/xtext/blob/76cf6452ffb640a5faf5f2ed3980d88776618f89/pom.xml#L128

, but you are using on older Version of Xtext. And I haven't checked the Tycho version it uses.

Btw. with that and the first point, are you interested in a TP update? To the latest 2024-09 SimRel?

soerendomroes commented 1 month ago

, but you are using on older Version of Xtext. And I haven't checked the Tycho version it uses.

We will update to a newer version together with the TP.

Btw. with that and the first point, are you interested in a TP update? To the latest 2024-09 SimRel?

We are interested in a TP update, however, we currently do not have time to test for issues with our upstream dependencies. Maybe this can be a separate PR that could be merged once we had time to check this out?

HannesWell commented 1 month ago

, but you are using on older Version of Xtext. And I haven't checked the Tycho version it uses.

We will update to a newer version together with the TP.

Btw. with that and the first point, are you interested in a TP update? To the latest 2024-09 SimRel?

We are interested in a TP update, however, we currently do not have time to test for issues with our upstream dependencies. Maybe this can be a separate PR that could be merged once we had time to check this out?

Sounds both good. 👍🏽 I can create a separate PR for updating the TP and Tycho to the latest versions. I already looked into it a bit and it shouldn't be a big problem.

From my side this PR is ready for submission. Please let me know if you need any adaptions.