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

Make JSON imports optional in OSGi environments #3971

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rombert
Copy link

@rombert rombert commented Sep 27, 2024

When deploying in OSGi containers the Jedis bundle has two hard requirements on having the GSON and JSON bundles around. Since these depenendencies are optional we mark the imports as such in the OSGi manifest.

If these packages are not exported the bundle will be installed and will be unable to make use of the JSON functionality. When they are present they will be used.

Additionally, allow newer version of the org.json bundle to be used without requiring a new release of Jedis.

Fixes #3956 .

When deploying in OSGi containers the Jedis bundle has two hard requirements on having
the GSON and JSON bundles around. Since these depenendencies are optional we mark
the imports as such in the OSGi manifest.

If these packages are not exported the bundle will be installed and will be unable to
make use of the JSON functionality. When they are present they will be used.

Additionally, allow newer version of the org.json bundle to be used without requiring
a new release of Jedis.
@rombert rombert force-pushed the feature/optional-json-imports-osgi branch from 81d4678 to e4d5aa4 Compare September 27, 2024 12:50
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rombert Thank you for the PR. We're checking about this change. It may take some time. In the mean time, I have a question:

pom.xml Show resolved Hide resolved
@tishun
Copy link

tishun commented Oct 10, 2024

I was wondering: if we are able to use the Jedis driver without these two - shouldn't we mark them as optional in the Maven dependency tree too. If we do then the maven-bundle-plugin would pick this up and generate the same result in the MANIFEST.MF, if I am not mistaken.

@rombert
Copy link
Author

rombert commented Oct 10, 2024

I was wondering: if we are able to use the Jedis driver without these two - shouldn't we mark them as optional in the Maven dependency tree too. If we do then the maven-bundle-plugin would pick this up and generate the same result in the MANIFEST.MF, if I am not mistaken.

Yes, if the Maven dependencies are marked as optional the maven-bundle-plugin should also make the imports optional. The version pinning for the org.json import, if deemed acceptable, still needs to stay.

@sazzad16
Copy link
Collaborator

I was wondering: if we are able to use the Jedis driver without these two - shouldn't we mark them as optional in the Maven dependency tree too. If we do then the maven-bundle-plugin would pick this up and generate the same result in the MANIFEST.MF, if I am not mistaken.

Yes, but this is a breaking change and has been considered as unfavorable #3278

@sazzad16 sazzad16 added the dependencies Pull request that updates a dependency label Oct 10, 2024
@tishun
Copy link

tishun commented Oct 11, 2024

I was wondering: if we are able to use the Jedis driver without these two - shouldn't we mark them as optional in the Maven dependency tree too. If we do then the maven-bundle-plugin would pick this up and generate the same result in the MANIFEST.MF, if I am not mistaken.

Yes, but this is a breaking change and has been considered as unfavorable #3278

Apologies, was not aware of #3278, makes sense.

In this case, for OSGi specifically, this seems a reasonable improvement. AFAIK most (all?) OSGi containers depend on the user to deploy the artefacts (e.g. by using a WAR file or a nested JAR file or some other way) and the MANIFEST.MF only serves to mandate what should be loaded, so we should not have the same issues as the ones described in #3278

@rombert
Copy link
Author

rombert commented Oct 11, 2024

OSGi containers depend on the user to deploy the artefacts (e.g. by using a WAR file or a nested JAR file or some other way) and the MANIFEST.MF only serves to mandate what should be loaded

Yes, that is correct. In the current form of Jedis the two JSON bundles must be deployed, even if they are not used by the application.

@rombert rombert requested a review from sazzad16 November 22, 2024 15:29
@sazzad16
Copy link
Collaborator

@rombert Apologies. We're still wait for a resolution from the higher-ups.

FYI, making the concerned dependencies totally optional in next major release is being considered.

@rombert
Copy link
Author

rombert commented Nov 25, 2024

Ack, thanks @sazzad16 . Let me know if there is information/testing I can provide in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull request that updates a dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine OSGi metadata for JSON dependencies
3 participants