Skip to content

Added auto-generated OSGi metadata - #934

Merged
johnmay merged 1 commit into
cdk:mainfrom
Mailaender:osgi
Nov 25, 2022
Merged

Added auto-generated OSGi metadata#934
johnmay merged 1 commit into
cdk:mainfrom
Mailaender:osgi

Conversation

@Mailaender

@Mailaender Mailaender commented Nov 24, 2022

Copy link
Copy Markdown
Contributor

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

@johnmay

johnmay commented Nov 24, 2022

Copy link
Copy Markdown
Member

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

Mailaender commented Nov 24, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

egonw commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

Mailaender commented Nov 25, 2022

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

johnmay commented Nov 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

Mailaender commented Nov 25, 2022

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

egonw commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

johnmay commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

johnmay commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

johnmay commented Nov 25, 2022

Copy link
Copy Markdown
Member

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

egonw commented Nov 26, 2022

Copy link
Copy Markdown
Member

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.

@egonw egonw mentioned this pull request Nov 30, 2022
Closed
@Mailaender Mailaender mentioned this pull request Aug 29, 2023
Closed
@Mailaender Mailaender mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Mailaender @johnmay @egonw