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

Added code changes to pom-versioned.xml to support osgi bundling for kryo5. #818

Merged
merged 4 commits into from
Apr 6, 2021

Conversation

Shradha-git
Copy link
Contributor

No description provided.

@theigl
Copy link
Collaborator

theigl commented Apr 5, 2021

@Shradha-git: Thank you!

Is it really necessary to specify all sub-packages like this:

com.esotericsoftware.kryo.kryo${kryo.major.version}.objenesis.instantiator,
com.esotericsoftware.kryo.kryo${kryo.major.version}.objenesis.strategy,

Is this not enough?

com.esotericsoftware.kryo.kryo${kryo.major.version}.*

Listing all those packages is quite brittle. If Kryo or any of the dependencies adds a new package, we have to make sure to add it here as well. If possible, I'd prefer a solution that does not break that easily.

sun.nio.ch;resolution:=optional
]]>
</Import-Package>
<Export-Package>com.esotericsoftware.kryo.kryo${kryo.major.version},com.esotericsoftware.kryo.kryo${kryo.major.version}.asm,com.esotericsoftware.kryo.kryo${kryo.major.version}.io,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we really need all the separate packages listed here, please add a newline after each package to make this easier to read.

Copy link
Contributor Author

@Shradha-git Shradha-git Apr 6, 2021

Choose a reason for hiding this comment

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

@theigl Initially I had tried adding the same like com.esotericsoftware.kryo.kryo${kryo.major.version}.*
But this change didn't work and will not create any export package entry in MANIFEST.MF file
Reason : There is no source java files in this versioned project, and hence it doesn't find anything to export when using the "*" config. So we need to manually list out each package. I can add a new line after each package.
cc @vikinghawk

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Shradha-git: I tried to find a better solution for this but the shade and bundle plugin don't play well together:

https://stackoverflow.com/questions/18280608/using-maven-bundle-plugin-with-the-maven-shade-plugin.

So it seems we really have to list all export packages, but I think we might be able to get rid of some of the exports. The Export-Package definition for the un-shaded JAR only lists the following two packages in addition to Kryo's own packages:

org.objenesis.instantiator,
org.objenesis.strategy

All other packages do not need to be exposed to consumers of the bundle. Could you please try this simplified export configuration and check if it works:

<Export-Package>
    com.esotericsoftware.kryo.kryo${kryo.major.version},
    com.esotericsoftware.kryo.kryo${kryo.major.version}.io,			
    com.esotericsoftware.kryo.kryo${kryo.major.version}.serializers,
    com.esotericsoftware.kryo.kryo${kryo.major.version}.unsafe,
    com.esotericsoftware.kryo.kryo${kryo.major.version}.util,		
    com.esotericsoftware.kryo.kryo${kryo.major.version}.objenesis,
    com.esotericsoftware.kryo.kryo${kryo.major.version}.objenesis.instantiator,
    com.esotericsoftware.kryo.kryo${kryo.major.version}.objenesis.strategy		
</Export-Package>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theigl Tried with above simplified list of Export-Package and it is getting resolved. I will update the Export-Package list with above one.

MANIFEST.MF generated:
Manifest-Version: 1.0
Automatic-Module-Name: com.esotericsoftware.kryo.kryo5
Bnd-LastModified: 1617716489841
Build-Jdk: 10.0.2
Built-By: SR068348
Bundle-Description: Fast, efficient Java serialization. This is the ve
rsion specific Kryo artifact.
Bundle-License: https://opensource.org/licenses/BSD-3-Clause
Bundle-ManifestVersion: 2
Bundle-Name: Kryo 5
Bundle-SymbolicName: com.esotericsoftware.kryo.5
Bundle-Version: 5.0.4
Created-By: Apache Maven Bundle Plugin
Export-Package: com.esotericsoftware.kryo.kryo5;version="5.0.4",com.es
otericsoftware.kryo.kryo5.io;version="5.0.4",com.esotericsoftware.kry
o.kryo5.serializers;version="5.0.4",com.esotericsoftware.kryo.kryo5.u
nsafe;version="5.0.4",com.esotericsoftware.kryo.kryo5.util;version="5
.0.4",com.esotericsoftware.kryo.kryo5.objenesis;version="5.0.4",com.e
sotericsoftware.kryo.kryo5.objenesis.instantiator;version="5.0.4",com
.esotericsoftware.kryo.kryo5.objenesis.strategy;version="5.0.4"
Import-Package: sun.misc;resolution:=optional,sun.nio.ch;resolution:=o
ptional,sun.reflect;resolution:=optional
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
Tool: Bnd-4.2.0.201903051501

@Shradha-git Shradha-git requested a review from theigl April 6, 2021 05:22
@theigl theigl linked an issue Apr 6, 2021 that may be closed by this pull request
@theigl
Copy link
Collaborator

theigl commented Apr 6, 2021

Thanks @Shradha-git! There are still some warnings by the shade and bundle plugins. I'll try to get rid of them after merging.

@theigl theigl merged commit a227885 into EsotericSoftware:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Request: OSGI compatibility for the versioned kryo5 jar
2 participants