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 auto-generated OSGi metadata #934

Merged
merged 1 commit into from Nov 25, 2022
Merged

Added auto-generated OSGi metadata #934

merged 1 commit into from Nov 25, 2022

Conversation

Mailaender
Copy link
Contributor

I want to get rid of net.openchrom.thirdpartylibraries.cdk and this seems to be a requirement.

@johnmay
Copy link
Member

johnmay commented Nov 24, 2022

More info needed. What are you doing and why?

By get rid of you means push the osgi in the main repo?

I’ve never seen a strong argument for OSGI but providing the build and deployment (central upload) size doesn’t change I have no strong objection.

@Mailaender
Copy link
Contributor Author

Mailaender commented Nov 24, 2022

Yes, I can then fetch CDK via Maven (instead of rebundling .jars) and integrate it into the target platform. Compare the MANIFEST.MF before and after to see what this actually does. See Apache Felix Maven Bundle Plugin for documentation.

@egonw
Copy link
Member

egonw commented Nov 25, 2022

OSGi-bundles for the CDK modules would help me too. E.g. Cytoscape and PathVisio have plugins that use the CDK and need OSGi bundles.

I am not Maven/OSGi expert (never had enough time to understand all the complexities, tho I got the basics). Are there any downsides of marking each module a "bundle"?

@Mailaender
Copy link
Contributor Author

A bundle is a jar with OSGi metadata according to https://stackoverflow.com/a/20260459 so I hope this does not change any existing build workflows.

@johnmay
Copy link
Member

johnmay commented Nov 25, 2022

Done some tests locally and this seems to double the installation size. It's already much bigger than I would like to some recent third-party updates (#911) and I'm hesitant to double that.

find . -name '*.jar' | xargs du -sch | sort -h

before:

...
176K  ./descriptor/fingerprint/target/cdk-fingerprint-2.9-SNAPSHOT.jar
192K  ./display/renderbasic/target/cdk-renderbasic-2.9-SNAPSHOT.jar
212K  ./misc/extra/target/cdk-extra-2.9-SNAPSHOT.jar
240K  ./descriptor/qsarmolecular/target/cdk-qsarmolecular-2.9-SNAPSHOT.jar
244K  ./tool/sdg/target/cdk-sdg-2.9-SNAPSHOT.jar
308K  ./base/core/target/cdk-core-2.9-SNAPSHOT.jar
324K  ./base/standard/target/cdk-standard-2.9-SNAPSHOT.jar
440K  ./storage/io/target/cdk-io-2.9-SNAPSHOT.jar
560K  ./storage/pdb/target/cdk-pdb-2.9-SNAPSHOT.jar
600K  ./legacy/target/cdk-legacy-2.9-SNAPSHOT.jar
2.2M  ./tool/builder3d/target/cdk-builder3d-2.9-SNAPSHOT.jar
 46M  ./bundle/target/cdk-2.9-SNAPSHOT.jar
 54M  total

after:

...
2.2M  ./storage/pdb/target/cdk-pdb-2.9-SNAPSHOT.jar
2.3M  ./app/depict/target/cdk-depict-2.9-SNAPSHOT.jar
2.3M  ./storage/pdbcml/target/cdk-pdbcml-2.9-SNAPSHOT.jar
2.9M  ./legacy/target/cdk-legacy-2.9-SNAPSHOT.jar
4.7M  ./tool/builder3d/target/cdk-builder3d-2.9-SNAPSHOT.jar
 45M  ./bundle/target/cdk-2.9-SNAPSHOT.jar
106M  total

It looks like what it does is pull in all the dependant files of the project.

Before:

[john@sentinel:cdk]% tar tvf ./storage/pdb/target/cdk-pdb-2.9-SNAPSHOT.jar | wc -l
      25

After:

[john@sentinel:cdk]% tar tvf ./storage/pdb/target/cdk-pdb-2.9-SNAPSHOT.jar | wc -l
     810

This might be better once we are more modularised move to Java modules but for now I think this is possible a no go unfortunately.

@Mailaender
Copy link
Contributor Author

I see. Modified it according to https://bnd.bndtools.org/tools/felix-maven.html so that the packaging type stays .jar using the “Maven first” approach.

@egonw
Copy link
Member

egonw commented Nov 25, 2022

Done some tests locally and this seems to double the installation size.

Okay, that's not what I had in mind or expected. That does not make sense to me either. As far as I know it should only update the MANIFEST.MF file. I cannot imagine a >1MB manifest file. The increase in number of lines is also weird.

Are we sure this is not an interaction effect and that it makes every module a "with dependencies" now?

@johnmay
Copy link
Member

johnmay commented Nov 25, 2022

I see. Modified it according to https://bnd.bndtools.org/tools/felix-maven.html so that the packaging type stays .jar using the “Maven first” approach.

Much better, thanks just test and all looks ok.

@johnmay johnmay merged commit 6c40e31 into cdk:main Nov 25, 2022
@johnmay
Copy link
Member

johnmay commented Nov 25, 2022

Yep @egonw now just the manifest updated:

[john@sentinel:cdk]% unzip -p ./storage/pdb/target/cdk-pdb-2.9-SNAPSHOT.jar META-INF/MANIFEST.MF
Manifest-Version: 1.0
Created-By: Apache Maven Bundle Plugin 5.1.8
Build-Jdk-Spec: 17
Specification-Title: cdk-pdb
Specification-Version: 2.9
Implementation-Title: cdk-pdb
Implementation-Version: 2.9-SNAPSHOT
Bnd-LastModified: 1669393139106
Bundle-Description: Modular library for Cheminformatics
Bundle-License: http://www.gnu.org/licenses/lgpl.html
Bundle-ManifestVersion: 2
Bundle-Name: cdk-pdb
Bundle-SymbolicName: org.openscience.cdk.pdb
Bundle-Version: 2.9.0.SNAPSHOT
Export-Package: org.openscience.cdk;version="2.9.0";uses:="org.openscien
 ce.cdk.interfaces",org.openscience.cdk.io;version="2.9.0";uses:="org.op
 enscience.cdk.exception,org.openscience.cdk.interfaces,org.openscience.
 cdk.io.formats",org.openscience.cdk.protein.data;version="2.9.0";uses:=
 "org.openscience.cdk,org.openscience.cdk.interfaces",org.openscience.cd
 k.templates;version="2.9.0";uses:="org.openscience.cdk,org.openscience.
 cdk.interfaces",org.openscience.cdk.templates.data;version="2.9.0",org.
 openscience.cdk.tools;version="2.9.0";uses:="org.openscience.cdk.except
 ion,org.openscience.cdk.interfaces"
Import-Package: javax.vecmath,org.openscience.cdk;version="[2.9,3)",org.
 openscience.cdk.aromaticity;version="[2.9,3)",org.openscience.cdk.confi
 g;version="[2.9,3)",org.openscience.cdk.dict;version="[2.9,3)",org.open
 science.cdk.exception;version="[2.9,3)",org.openscience.cdk.geometry;ve
 rsion="[2.9,3)",org.openscience.cdk.graph;version="[2.9,3)",org.opensci
 ence.cdk.graph.rebond;version="[2.9,3)",org.openscience.cdk.interfaces;
 version="[2.9,3)",org.openscience.cdk.io;version="[2.9,3)",org.openscie
 nce.cdk.io.formats;version="[2.9,3)",org.openscience.cdk.io.setting;ver
 sion="[2.9,3)",org.openscience.cdk.protein.data;version="[2.9,3)",org.o
 penscience.cdk.templates;version="[2.9,3)",org.openscience.cdk.tools;ve
 rsion="[2.9,3)",org.openscience.cdk.tools.manipulator;version="[2.9,3)"
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
Tool: Bnd-6.3.1.202206071316

@Mailaender Mailaender deleted the osgi branch November 25, 2022 17:42
@johnmay
Copy link
Member

johnmay commented Nov 25, 2022

Note to future self.

The build was made a little slower (+18% user time ~49s => ~59s, real time ~19s => ~20.4s M1 Pro). OK but careful of "death by a thousand paper cuts". Likely improved by less modules.

@egonw
Copy link
Member

egonw commented Nov 26, 2022

An alternative is to have the extra manifest information in a separate file (if not mistaken), and have the plugin use that to create the OSGi bundle. Less build time, more admin.

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.

None yet

3 participants