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

Prevent generation of Substitution Packages (Import-Package) without version #6267

Closed
chrisrueger opened this issue Sep 13, 2024 · 4 comments · Fixed by #6270
Closed

Prevent generation of Substitution Packages (Import-Package) without version #6267

chrisrueger opened this issue Sep 13, 2024 · 4 comments · Fixed by #6270

Comments

@chrisrueger
Copy link
Contributor

chrisrueger commented Sep 13, 2024

Problem

In #6220 a problem was reported that bnd has generated an Import-Package header for

# .bnd

                Import-Package: sun.misc;resolution:=optional, *

                -exportcontents:\
                    com.google.gson,\
                    com.google.gson.annotations,\
                    com.google.gson.reflect,\
                    com.google.gson.stream

created an Import-Package for com.google.gson.annotations without a version range :

# generated Manifest.mf

Export-Package                          com.google.gson.annotations;version="2.11.0"
                                        com.google.gson.reflect;version="2.11.0"
                                        com.google.gson.stream;version="2.11.0"
                                        com.google.gson;uses:="com.google.gson.reflect,com.google.gson.stream";version="2.11.0"
Import-Package                          com.google.gson.annotations
                                        sun.misc;resolution:=optional

Note: the version on the Export-Packages (2.11.0) was taken from the Bundle-Version header, which is some kind of last-resort when no version is provided on the exports (not good OSGi practise but very common in the wild for library bundles who "just" want to add OSGi meta data via bnd)

Expected result

As discussed in the call today with @pkriens it was disucssed that in the case above

com.google.gson.annotations should NOT appear in the Import-Package because it has no version.

Related OSGi spec sections

@chrisrueger
Copy link
Contributor Author

chrisrueger commented Sep 13, 2024

I got something working to remove substitution packages (Imports) without versions but then the following testcase fails BuilderTest#testNoImportForUsedExport_971 (originated from #971):

https:/bndtools/bnd/blob/master/biz.aQute.bndlib.tests/test/test/BuilderTest.java#L454

This testcase currently produces and expects the following Manifest:

Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Created-By: 21.0.3 (Eclipse Adoptium)
Tool: Bnd-7.1.0
Bnd-LastModified: 1726266483369
Export-Package: test.missingimports_971.p1;version="0.0.0",test.missingi
 mports_971.p2;version="0.0.0",test.missingimports_971.p4;version="0.0.0
 "
Import-Package: java.lang,test.missingimports_971.p1,test.missingimports
 _971.p2
Private-Package: test.missingimports_971.p3
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=17))"
Bundle-SymbolicName: biz.aQute.bndlib.tests
Bundle-Name: biz.aQute.bndlib.tests
Bundle-Version: 0

The question is: what can we do? @pkriens

UPDATE: I updated the testcase to include a version in the exports. So this testcase passes again. I added another testcase which checks the missing version case too then.
see PR #6270

@chrisrueger
Copy link
Contributor Author

chrisrueger commented Sep 14, 2024

@laeubi would be interested in your opinion too #6270

@laeubi
Copy link
Contributor

laeubi commented Sep 14, 2024

  1. If it derives the export-package version from the bundle version the version is actually known so it looks like a bug to claim it is not known for the import (that is obviously generated by bnd so user can't be blamed here).
  2. Substitution package without a version (range) is not valid as per spec (emphasis by me) so adding check in bnd itself seems good.

Importing an exported package must use a version range according to its compatibility requirements, being either a consumer or a provider of that API

In general if no version range can ultimately derived for whatever reason, then it should be an error / warning and not be imported at all.

Given that the test-case you presented makes wrong assumption, the resulting manifest is wrong (in terms of the spec) as it export and imports test.missingimports_971.p1 and test.missingimports _971.p2 but misses a version range in the import.

@chrisrueger
Copy link
Contributor Author

  1. If it derives the export-package version from the bundle version the version is actually known so it looks like a bug to claim it is not known for the import

I don't know what to say here, but I just remember this statement for a PR which was rejected.

2. Substitution package without a version (range) is not valid as per spec (emphasis by me) so adding check in bnd itself seems good.

Good point. This underlines the intent of this issue and also that the testcase should be adjusted as I have done in PR #6270

then it should be an error / warning

Where would you want to see that warning / error?

and not be imported at all.

this is done in #6270

Could you maybe review PR #6270 too?
Would be good to have more pairs of eyes.

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 a pull request may close this issue.

2 participants