Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Not entirely sure about Bundle-SymbolicName

Do we want:
a) Bundle-SymbolicName: org.jspecify.jspecify
or
b) Bundle-SymbolicName: org.jspecify.annotations

a) is currently implemented. It is based on the maven artifact GAV (org.jspecify:jspecify:jar:0.3.0 e.g. see here

As a ignorant developer supporting a library with OSGi users, I found adding a unit test helpful comfort, wdyt?
(example using pax-exam)

This project might want reproducible jars? I don't believe bnd does that by default (my build snippet if helpful).

As a ignorant developer supporting a library with OSGi users, I found adding a unit test helpful comfort, wdyt?
(example using pax-exam)

I'm going to steal that 😄 for my own project.

Also I think making this project reproducible is more important than OSGi so hopefully that will be the case. I didn't see it in the list: https://github.com/jvm-repo-rebuild/reproducible-central#readme

@ben-manes do you know if it is currently reproducible (jspecify jars)?

@ben-manes thanks a lot for the pointers. I tried adding your suggestions about reproducible builds - although I must admit I have no experience with it, so could you double check it?

Regarding unit test: generally good idea. What would you test for here?

Last point:

As a ignorant developer

Sorry, german here, don't get it ;) What's is with this "ignorant"?

do you know if it is currently reproducible (jspecify jars)?

Prior to @chrisrueger's change, those jar look non-reproducible because it included the timestamps (those gradle flags were not set).

$ unzip -l jspecify-0.3.0.jar
Archive:  jspecify-0.3.0.jar
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  12-13-2022 12:34   META-INF/
       46  12-13-2022 12:34   META-INF/MANIFEST.MF
       ...

When set it would look more like,

unzip -l caffeine-3.1.8.jar
Archive:  caffeine-3.1.8.jar
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  02-01-1980 00:00   META-INF/
      792  02-01-1980 00:00   META-INF/MANIFEST.MF
    11358  02-01-1980 00:00   META-INF/LICENSE
    ...

What would you test for here?

I suppose maybe a reflection query if the annotation is found on a dummy test class? I believe since it runs in a custom classloader the Caffeine test is just ensuring that its library can be loaded by the runtime.

What's is with this "ignorant"?

haha, I just meant that I have not actually ever used OSGi and only know of it as some magic flags that I set to satisfy other developers. I don't trust myself to not break them by an innocent refactor, so I can stay naive but more assured.

could you double check it?

lgtm (but just an observer)

➜  Downloads gh repo clone jspecify/jspecify
fatal: destination path 'jspecify' already exists and is not an empty directory.
failed to run git: exit status 128
➜  Downloads cd jspecify
➜  jspecify git:(add-osgi-metadata) gh pr checkout 428
From https://github.com/jspecify/jspecify
 * branch            refs/pull/428/head -> FETCH_HEAD
Already up to date.
➜  jspecify git:(add-osgi-metadata) ./gradlew -q jar
➜  jspecify git:(add-osgi-metadata) cd build/libs
➜  libs git:(add-osgi-metadata) unzip -l jspecify-0.0.0-SNAPSHOT.jar
Archive:  jspecify-0.0.0-SNAPSHOT.jar
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  02-01-1980 00:00   META-INF/
      626  02-01-1980 00:00   META-INF/MANIFEST.MF
        0  02-01-1980 00:00   META-INF/versions/
        0  02-01-1980 00:00   META-INF/versions/9/
        0  02-01-1980 00:00   META-INF/versions/9/OSGI-INF/
      145  02-01-1980 00:00   META-INF/versions/9/OSGI-INF/MANIFEST.MF
      187  02-01-1980 00:00   META-INF/versions/9/module-info.class
        0  02-01-1980 00:00   org/
        0  02-01-1980 00:00   org/jspecify/
        0  02-01-1980 00:00   org/jspecify/annotations/
      434  02-01-1980 00:00   org/jspecify/annotations/NonNull.class
      498  02-01-1980 00:00   org/jspecify/annotations/NullMarked.class
      488  02-01-1980 00:00   org/jspecify/annotations/NullUnmarked.class
      436  02-01-1980 00:00   org/jspecify/annotations/Nullable.class
---------                     -------
     2814                     14 files
➜  libs git:(add-osgi-metadata) unzip jspecify-0.0.0-SNAPSHOT.jar
Archive:  jspecify-0.0.0-SNAPSHOT.jar
   creating: META-INF/
  inflating: META-INF/MANIFEST.MF
   creating: META-INF/versions/
   creating: META-INF/versions/9/
   creating: META-INF/versions/9/OSGI-INF/
  inflating: META-INF/versions/9/OSGI-INF/MANIFEST.MF
  inflating: META-INF/versions/9/module-info.class
   creating: org/
   creating: org/jspecify/
   creating: org/jspecify/annotations/
  inflating: org/jspecify/annotations/NonNull.class
  inflating: org/jspecify/annotations/NullMarked.class
  inflating: org/jspecify/annotations/NullUnmarked.class
  inflating: org/jspecify/annotations/Nullable.class
➜  libs git:(add-osgi-metadata) cat META-INF/MANIFEST.MF
Manifest-Version: 1.0
Bundle-Description: An artifact of well-specified annotations to power
  static analysis checks and JVM language interop.
Bundle-DocURL: https://jspecify.dev/docs/start-here
Bundle-License: https://www.apache.org/licenses/LICENSE-2.0
Bundle-ManifestVersion: 2
Bundle-Name: jspecify
Bundle-SymbolicName: org.jspecify.jspecify
Bundle-Vendor: jspecify.dev
Bundle-Version: 0.0.0.SNAPSHOT
Export-Package: org.jspecify.annotations;version="0.0.0.SNAPSHOT"
Import-Package: java.lang,java.lang.annotation
Multi-Release: true
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

lgtm (but just an observer)

Thanks a lot.

@ben-manes Sorry I didn't mean for you to go actually check 😄 just if you knew offhand. Thanks for checking though!

The reason is because it is missing from : https://github.com/jvm-repo-rebuild/reproducible-central#readme

I will see if I can add it to the list today. I think you can add an artifact without being a project owner.

I will say checking the timestamp in the zip contents sometimes isn't enough because some Maven plugins have (had) bugs like Moditect that accidentally put something like timestamp somewhere in the zip that for some reason isn't visible in the zip manifest (e.g. checksum changes on each build). However this project uses Gradle (and I guess Blaze internally) so probably not a problem.

Regarding failing JDK 11 build

I forgot that bnd 7.0.0 requires JDK 17 as a minimum.
I'll later decrease bnd to 6.4.0 which should also work and requires JDK 8 asa minimum.

I'll later decrease bnd to 6.4.0 which should also work and requires JDK 8 asa minimum.

@cpovirk could you trigger a rebuild to check if JDK11 build now runs?

Done. Looks like it's unhappy with the module-info under the multi-release jar root. If that's a significant obstacle, feel free to move it back to the main root (#484).

Done. Looks like it's unhappy with the module-info under the multi-release jar root. If that's a significant obstacle, feel free to move it back to the main root (#484).

Thanks. I have applied a fix.
it is kind of a dilemma: unfortunatelly bnd in 6.4.0 does not support multi-release jars. It starts being support in bnd 7.0.0 but this requires min. JDK 17.
So the only way is to suppress the warning via -fixupmessage (see bndtools/bnd#3514 and https://bnd.bndtools.org/instructions/fixupmessages.html)

I try to investigate if there are other options like doing different things depending on JDK version.

You can run gradle on 17 independently from the execution tasks. In the latest version, you can add the gradle-daemon-jvm.properties tool version and use bnd, but build on a lower jdk using the java.toolchain.languageVersion. Does that help?

You can run gradle on 17 independently from the execution tasks. In the latest version, you can add the gradle-daemon-jvm.properties tool version and use bnd, but build on a lower jdk using the java.toolchain.languageVersion. Does that help?

Thanks. Not sure.

My first thought was to try something like this: https://discuss.gradle.org/t/conditional-dependency-based-on-toolchains-version/46369

if (JavaVersion.current().isJava17Compatible()) {
    id "biz.aQute.bnd.builder" version "7.0.0"
  }
else {
    id "biz.aQute.bnd.builder" version "6.4.0"
  }

Is this what you mean?

Nope. Bnd only needs to run in gradle so as long as the daemon is 17+ then everything else will work. You can now force that with a daemon toolchain.

The Java version for tasks like compile, test, exec are from the language toolchain. Instead of inferring it by the daemon jvm, it should be set explicitly. An external env variable can be used, which then lets you cross build. The jdk release flag is also useful as it will ensure that the output matches a desired version as well.

Caffeine’s build uses this, but it’s a few parts to set up.

(I'd be happy for someone to make that happen, especially after the fun in #517. I don't expect to get to it myself on any particular timeline, unfortunately.)

Thanks for the pointers @ben-manes
I reverted my changes back to bnd 7.0.0 because this would be the right thing once JDK 17 is used to support Multi-Release jars too.

In the latest version, you can add the gradle-daemon-jvm.properties tool version and use bnd, but build on a lower jdk using the java.toolchain.languageVersion.

It seems that it is currently in (unreleased) Gradle 8.8.-rc-1 and I first need to get familiar with the whole toolchain stuff. No gradle expert.
So eventually somebody more familiar with the matter can take over or someday I'll find the time to wrap my head around it.

OK, so the failure when building under Java 11 is what's expected from the bnd version, as discussed above. (Sorry, I have half-watched this develop but not retained much information.)

I don't know that anyone will have time to sort out toolchains in the immediate future (though of course we've had opportunities dating back some time now...), so this might well miss the 1.0.0 release. But we can at least try to get back to it sometime afterward, and of course everyone is welcome to continue to rescue us by handing us a tweaked PR that passes while still including testing under the currently tested JDKs.

so the failure when building under Java 11 is what's expected from the bnd version, as discussed above

Yes that is expected. min. JDK17 for bnd >= 7.0.0

this might well miss the 1.0.0 release. But we can at least try to get back to it sometime afterward, and of course everyone is welcome to continue to rescue us by handing us a tweaked PR that passes while still including testing under the currently tested JDKs.

Ok understood. Thanks for considering it later. Keeping my fingers crossed somebody with deep gradle toolchain knowledge could help.

@wmdietl or anyone else, any other concerns?

You and me both :) Thanks.

(Actually, the discussion here has given me plenty of reason to feel comfortable with merging this. I just don't think anyone will confuse me with someone who has knowledge of OSGi anytime soon....)

And thank you and kudos to @ben-manes for all the help 👍

For the more OSGi-literate, here's what the 1.0.0 data is shaping up to look like:

META-INF/MANIFEST.MF

Manifest-Version: 1.0
Bundle-Description: An artifact of well-specified annotations to power
  static analysis checks and JVM language interop.
Bundle-DocURL: https://jspecify.dev/docs/start-here
Bundle-License: https://www.apache.org/licenses/LICENSE-2.0
Bundle-ManifestVersion: 2
Bundle-Name: JSpecify annotations
Bundle-SymbolicName: org.jspecify.jspecify
Bundle-Vendor: The JSpecify Authors
Bundle-Version: 1.0.0
Export-Package: org.jspecify.annotations;version="1.0.0"
Implementation-Version: 1.0.0
Import-Package: java.lang,java.lang.annotation
Multi-Release: true
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

META-INF/versions/9/OSGI-INF/MANIFEST.MF

Manifest-Version: 1.0
Import-Package: java.lang,java.lang.annotation
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

haha, I think @chrisrueger is the only OSGi literate one here. If you want to validate then you could write a unit test (example) to have some assurance that things were okay.

Thanks @cpovirk for the update.
I also re-tested locally (with the 0.0.0.SNAPSHOT) version how it looks, when I used the JSpecify OSGi bundle in our application.
Summary: It works fine, from my OSGi user perspective. Also the Eclipse PDE / JDT people should be able to work with it I guess (@bjmi ?).

For documentation purposes here are some screenshots of how the bndtools Eclipse Plugin shows the bundle when I import it as a dependency. This time I added it to a local file repo, but 1.0.0 will e.g. come from Maven Central then.

I also started the bundle for the sake of testing with our app and verified with the OSGi console that the bundle is successfully started and in an Active state:

lb jspec
START LEVEL 1
   ID|State      |Level|Name
  162|Active     |    1|JSpecify annotations (0.0.0.SNAPSHOT)|0.0.0.SNAPSHOT

For sake of completeness the pretty-printed output of the bndtools' JARViewer:

[MANIFEST]

Bundle-Description                      An artifact of well-specified annotations to power static analysis checks and JVM language interop.
Bundle-DocURL                           https://jspecify.dev/docs/start-here
Bundle-License                          https://www.apache.org/licenses/LICENSE-2.0
Bundle-ManifestVersion                  2
Bundle-Name                             JSpecify annotations
Bundle-SymbolicName                     org.jspecify.jspecify
Bundle-Vendor                           The JSpecify Authors
Bundle-Version                          0.0.0.SNAPSHOT
Export-Package                          org.jspecify.annotations;version="0.0.0.SNAPSHOT"
Implementation-Version                  0.0.0-SNAPSHOT
Import-Package                          java.lang
                                        java.lang.annotation
Manifest-Version                        1.0
Multi-Release                           true
Require-Capability                      osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"


[IMPEXP]

Import-Package
  java.lang                              
  java.lang.annotation                   

Export-Package
  org.jspecify.annotations               {version=0.0.0.SNAPSHOT}

[CAPABILITIES]

Require-Capability
  osgi.ee                                {filter:=(&(osgi.ee=JavaSE)(version=1.8))}

[COMPONENTS]



[METATYPE]



[API USES]


[USES]

org.jspecify.annotations                


[USEDBY]



[LIST]

META-INF
  MANIFEST.MF
META-INF/versions
META-INF/versions <no contents>
META-INF/versions/9
  module-info.class
META-INF/versions/9/OSGI-INF
  MANIFEST.MF
org
org <no contents>
org/jspecify
org/jspecify <no contents>
org/jspecify/annotations
  NonNull.class
  NullMarked.class
  NullUnmarked.class
  Nullable.class

Thanks. Real-world usage is more reassuring than a test, but a test also adds reassurance. I cobbled together something based on Caffeine and ancient Google Groups postings in 3dd3700. It passes, and if I revert the commit from this PR, then it fails with java.lang.ClassNotFoundException: org.jspecify.annotations.Nullable not found by PAXEXAM-PROBE-4d05fdef-9dab-45b4-a779-2808c46ee9f8 [16]. I'm not sure if I'll ever clean it up for submission or not.

First of all, I would like to thank you for your efforts that have led to a solution.

Now for two irregularities that I noticed.

If both MANIFEST.MF files don't differ in Import-Package and Require-Capability then why not omit META-INF/versions/9/OSGI-INF/MANIFEST.MF?
Or alternatively specify Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=9))" for the latter.
META-INF/MANIFEST.MF

Import-Package: java.lang,java.lang.annotation
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

META-INF/versions/9/OSGI-INF/MANIFEST.MF

Import-Package: java.lang,java.lang.annotation
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"

I'm not convince of BSN org.jspecify.jspecify. The second jspecify is superfluous. Since Maven uses two parts to identify an artifact by groupId and artifactId OSGI uses a single identifier the BSN only.
E.g. com.acme:acme-api -> com.acme.api or org.foo:foo -> org.foo as decribed in Apache Felix Maven Bundle Plugin (BND) doc
What's next with jspecify? Further annotations in the current bundle then org.jspecify.annotations is a good candidate. Or something like an uber jar for all kinds of things then org.jspecify is fine.

I'm not convince of BSN org.jspecify.jspecify. The second jspecify is superfluous.

Feeling the same a bit after some distance now.

What's next with jspecify? Further annotations in the current bundle then org.jspecify.annotations is a good candidate. Or something like an uber jar for all kinds of things then org.jspecify is fine.

I like org.jspecify.annotations

It transports the intention of the bundle in its current state - which is about "annotations".
I created PR #569 for this. @cpovirk

Regarding the other thing, the redundancy of the META-INF/versions/9/OSGI-INF/MANIFEST.MF

This was auto-generated by bnd and I am not familiar with Multi-Release-Jars (MRJ) / JPMS.

Only thing I can point out regarding the version range:

• (version=1.8) means version >= 1.8 (see ) which is sometimes confusing, since it "looks" like a fixed version, but it is actually a range.
• since MRJ were introduced with JDK 9 I would argue that version >= 1.8 is not wrong. Only a JDK >= 9 would inspect the META-INF/versions/ folder