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

Require-Bundle javax.inject #2176

Closed
jukzi opened this issue Mar 24, 2023 · 25 comments
Closed

Require-Bundle javax.inject #2176

jukzi opened this issue Mar 24, 2023 · 25 comments

Comments

@jukzi
Copy link

jukzi commented Mar 24, 2023

Instead of using "Require-Bundle" could you please use "Import-Package" for javax.inject

We currently try to use a Non-OSGi java where libraries should not conflict but
bundle javax.inject conflicts with jakarta.inject which is normally used for that package. As far as i know org.eclipse.xtext.util is the only lib that explicitly requests javax.inject.

https://github.com/eclipse/xtext-core/blob/0ca980145ddb04780e6e898a023458782e734977/org.eclipse.xtext.util/META-INF/MANIFEST.MF#L48

@cdietrich
Copy link
Member

cdietrich commented Mar 24, 2023

we currently use the google inject annotation and injector class at 1 billion places. so the require wont be sufficient
do i overlook something? the question is why we use require bundle for it.

beside util its also used in test bundles as import package

@szarnekow
Copy link
Contributor

It's worse: javax.inject;bundle-version="1.0.0";resolution:=optional; is a bad joke. It's not optional at all. Replacing the require-bundly by import-package would likely be an incompatible change since we are currently re-exporting javax.inject.

I think this needs some more analysis.

@cdietrich
Copy link
Member

cdietrich commented Mar 24, 2023

@jukzi @szarnekow can package import be reexported?
so this would break a ton of xtext plugin and all xtext languages out there. e.g. mwe

@szarnekow
Copy link
Contributor

A bundle can export it's imported packages, even though the UI does not allow it. The OSGI spec has rules how that situation will be resolved, though. I'm not terribly sure about that approach though, since it's certainly prone to confusion.

@LorenzoBettini
Copy link
Contributor

Instead of using "Require-Bundle" could you please use "Import-Package" for javax.inject

We currently try to use a Non-OSGi java where libraries should not conflict but bundle javax.inject conflicts with jakarta.inject which is normally used for that package. As far as i know org.eclipse.xtext.util is the only lib that explicitly requests javax.inject.

https://github.com/eclipse/xtext-core/blob/0ca980145ddb04780e6e898a023458782e734977/org.eclipse.xtext.util/META-INF/MANIFEST.MF#L48

by non-OSGI do you mean plain Maven/Gradle project? In that case, you can exclude the dependency javax.inject when specifying the org.xtext.util dependency.

@jukzi
Copy link
Author

jukzi commented Mar 24, 2023

sadly it's not that easy for us.
best would be to not reexport but package import in all depended bundles. yes it's not compatible but straight forward.

@cdietrich cdietrich transferred this issue from eclipse/xtext-core Apr 3, 2023
@HannesWell
Copy link
Member

@jukzi @szarnekow can package import be reexported? so this would break a ton of xtext plugin and all xtext languages out there. e.g. mwe

Not as far as I know.

A bundle can export it's imported packages, even though the UI does not allow it. The OSGI spec has rules how that situation will be resolved, though. I'm not terribly sure about that approach though, since it's certainly prone to confusion.

It is more the other way round, a bundle can import the packages it exports. In general a Bundle can only export what is contained in the bundle.

so this would break a ton of xtext plugin and all xtext languages out there. e.g. mwe

As far as I can tell there will be no way around that, if that issue should be addressed. This is one of the main the reason why reexported require-bundle is strongly discouraged and require bundle is also discouraged, although it might seem to be more convenient at first sight.

@HannesWell
Copy link
Member

best would be to not reexport but package import in all depended bundles. yes it's not compatible but straight forward.

I suggest when Xtext has migrated to jakarata.inject/annotation annotations to remove the reexported bundles and then only import jakarta.inject/annotation.

For platform I started the migration, first only to the javax-annotations provided by jakarta and eventually to jakarta-annotations: eclipse-platform/eclipse.platform.releng.aggregator#1056
If time permits I will finish this for the 2023-06 release.

And when Xtext requires that release IMHO it would be a good point to make the switch.

@cdietrich
Copy link
Member

cdietrich commented May 2, 2023

we still need javax.inject for 2023-06 as i dont think we get this updated so soon on our with the capacity i see.
but i assume we do also just repackage the orbit one on https://github.com/eclipse/xtext/blob/main/org.eclipse.xtext.p2repository/category.xml
so the consumer can take ours and we are independent from upstream

@HannesWell
Copy link
Member

we still need javax.inject for 2023-06 as i dont think we get this updated so soon on our with the capacity i see.

If you are willing to potentially break down stream projects it would be sufficient to change the Require-bundle javax.inject to Import-Package javax.inject. That can be done quickly.
What could be tried out if it helps to make the re-exported javax.inject required bundle optional. Maybe this would make the re-export available if the bundle is present but would not force it's presence.

When Xtext migrates to jakarta annotations that should not be another break for down-stream consumers, since it is an internal change.

Btw. if Xtext migrates to jakarta.inject/annotations I would also migrate the the google.inject annotations to jakarta (although they are used very much).

@cdietrich
Copy link
Member

cdietrich commented May 2, 2023

i fear this will also break all internal projects
we also have dedicated code that checks if guice provider is used and not javax.
have no insights in the why there
maybe @szarnekow has

@HannesWell
Copy link
Member

Understand.

What could be tried out if it helps to make the re-exported javax.inject required bundle optional. Maybe this would make the re-export available if the bundle is present but would not force it's presence.

Regarding this suggestion, coincidentally we had a bug-report just this evening that shows that it would probably work (see eclipse-platform/eclipse.platform.ua#122).
If we mark all reexported requirements of javax.inject as optional those that depend on it can just add javax.inject to their product (e.g. from an old Orbit release repo) to make it work as before but at the same time those that don't need it are not forced to use it. And if javax.inject is not directly available but it is clearly documented (e.g. in the release notes) how to get it, everybody should notice it that one day they should migrate, but are not forced to do it immediately without previous notice.
Then after your deprecation period is over the reexported require bundle could be removed.

@cdietrich
Copy link
Member

for that we need to check what works. at some places we reject usage of the javax annotation

@LorenzoBettini
Copy link
Contributor

Btw. if Xtext migrates to jakarta.inject/annotations I would also migrate the the google.inject annotations to jakarta (although they are used very much).

The two Inject annotations are not completely interchangeable: https://github.com/google/guice/wiki/JSR330

@cdietrich
Copy link
Member

yes this is what i saw some special threatment for

cdietrich added a commit that referenced this issue May 5, 2023
…inject to jakarta.inject in guice

Signed-off-by: Christian Dietrich <christian.dietrich@itemis.de>
cdietrich added a commit that referenced this issue May 5, 2023
[#2176] use guice namespace annotations to prepare switch from javax.inject to jakarta.inject in guice
@cdietrich
Copy link
Member

for now i changed the code to use guices annotations consistently. so it will work with and without javax.annotation present

@HannesWell
Copy link
Member

for now i changed the code to use guices annotations consistently. so it will work with and without javax.annotation present

Since the only Require-Bundle header that mentions javax.inject marks the requirement as optional (as mentioned in #2176 (comment)) I think that change resolves this issue.
If the javax.inject bundle is not present org.eclipse.xtext.util should still resolve and work well.

If somebody relies on the reexport just adding javax.inject to the TP and runtime is sufficient.

@jukzi can you test a current nightly?

If that requirement should be deprecated one day PDE could probably help in the future: eclipse-pde/eclipse.pde#406

@jukzi
Copy link
Author

jukzi commented May 16, 2023

@jukzi can you test a current nightly?

I don't see what changed for us. As long as there is an javax.inject import our build system will add it :-(

@szarnekow
Copy link
Contributor

I don't see what changed for us. As long as there is an javax.inject import our build system will add it :-(

What do you refer to by import? A Java import? Or even an optional require-bundle in a manifest?

@jukzi
Copy link
Author

jukzi commented May 16, 2023

sorry i meant "Require-Bundle:" even if optional

@szarnekow
Copy link
Contributor

In that case, I'm afraid there's not much we can do within the boundaries of Xtext's backward compatibility guarantees.

Your tool is not using a regular OSGI resolver, I guess? Or it is resorting to greedy resolution of dependencies?

@HannesWell
Copy link
Member

HannesWell commented May 16, 2023

@jukzi can you test a current nightly?

I don't see what changed for us. As long as there is an javax.inject import our build system will add it :-(

What build system do you use? Maven+Tycho? If yes, we could consider to not fail the resolution if a optional dependency is missing. This would also make #2219 work.
See also eclipse-tycho/tycho#2309.

@jukzi
Copy link
Author

jukzi commented May 17, 2023

I highly appreciate your offer to help, but currently for us there is no simple way to migrate towards any ordinary build tool.
But i got it: xtext doesn't need java.inject but will keep requiring it for backward compatibility. I hoped xtext would
cut off old braids but we can work around.

@cdietrich
Copy link
Member

I would like to discuss the compatibility thing together with the guice and guava update. Right now we have 3 discussion in parallel

@cdietrich
Copy link
Member

obsolete with guice and guava update in 2.32

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