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 re-exports in MANIFEST.MF and use Import-Package where suitable #1074

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

HannesWell
Copy link
Contributor

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
Copy link
Contributor

soerendomroes commented Sep 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

soerendomroes commented Sep 23, 2024

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
Copy link
Contributor

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

@HannesWell
Copy link
Contributor Author

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
Copy link
Contributor

, 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
Copy link
Contributor Author

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

@soerendomroes soerendomroes merged commit b6ed84a into eclipse-elk:master Oct 17, 2024
7 checks passed
@HannesWell HannesWell deleted the remove-reexports branch October 17, 2024 08:16
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.

2 participants