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

FELIX-6708 upgrade bnd to 7.0.0 #322

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

paulrutter
Copy link
Contributor

@paulrutter paulrutter commented May 25, 2024

See https://issues.apache.org/jira/browse/FELIX-6708 and bndtools/bnd#2227 (comment)

To be able to handle Multi-Release jars as dependencies in an OSGi bundle.
This prevents exceptions like:

build	24-May-2024 18:40:01	[ERROR] Bundle com.abc:jersey-bundle:bundle:55.0-SNAPSHOT : Classes found in the wrong directory: {META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport$NonLoomishExecutors.class=org.glassfish.jersey.innate.VirtualThreadSupport$NonLoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/virtual/LoomishExecutors.class=org.glassfish.jersey.innate.virtual.LoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport$Java21LoomishExecutors.class=org.glassfish.jersey.innate.VirtualThreadSupport$Java21LoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport.class=org.glassfish.jersey.innate.VirtualThreadSupport}

To be able to handle Multi-Release jars as dependencies in an OSGi bundle.
This prevents exceptions like:

```
build	24-May-2024 18:40:01	[ERROR] Bundle com.abc:jersey-bundle:bundle:55.0-SNAPSHOT : Classes found in the wrong directory: {META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport$NonLoomishExecutors.class=org.glassfish.jersey.innate.VirtualThreadSupport$NonLoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/virtual/LoomishExecutors.class=org.glassfish.jersey.innate.virtual.LoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport$Java21LoomishExecutors.class=org.glassfish.jersey.innate.VirtualThreadSupport$Java21LoomishExecutors, META-INF/versions/21/org/glassfish/jersey/innate/VirtualThreadSupport.class=org.glassfish.jersey.innate.VirtualThreadSupport}
```
Update to java 17 as thats required by bnd
Revert unneeded change
@paulrutter paulrutter changed the title FELIX-6708 upgrade bnd FELIX-6708 upgrade bnd to 7.0.0 May 25, 2024
@paulrutter
Copy link
Contributor Author

paulrutter commented May 25, 2024

Probably this code can be removed now as well, as bnd can handle it.

protected static void includeJava9Fixups(MavenProject currentProject, Analyzer analyzer)

It suppresses the errors as warnings via fixupmessages.
This didn’t work for me due to having this line in a parent pom.xml:

<_fixupmessages>^Classes found in the wrong directory: \{META-INF/versions/(9|10|11|12)/</_fixupmessages>

It didn’t cover java 21. Still, it would be nice to move to 7.0.0 as it claims to support multi release jars properly.

@paulrutter
Copy link
Contributor Author

paulrutter commented May 30, 2024

Two integration tests fail still, but one of these also failed on 6.3.1.

[INFO] Building: existing-metadata-no-update\pom.xml
[INFO] run pre-build script prebuild.groovy
[INFO] run post-build script verify.groovy
[INFO]   The post-build script did not succeed. assert manifest.isEmpty()
       |        |
       |        false
       Manifest-Version: 1.0
       Bnd-LastModified: 1717071194581
       Build-Jdk-Spec: 21
       Bundle-Description: This project has been built before (i.e. a cache fil
        e is there), the current pom.xml has not been touched, nor any of the b
        uilt files
       Bundle-DocURL: https://wcm.io
       Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
       Bundle-ManifestVersion: 2
       Bundle-Name: existing-metadata-no-update
       Bundle-SymbolicName: org.apache.felix.bundleits.existing-metadata-no-upd
        ate
       Bundle-Vendor: wcm.io
       Bundle-Version: 1.0.0.SNAPSHOT
       Created-By: Apache Maven Bundle Plugin 5.1.10-SNAPSHOT
       Tool: Bnd-7.0.0.202310060912
[INFO]           existing-metadata-no-update\pom.xml .............. FAILED (28.9 s)
[INFO] Building: no-test-scoped-imports\pom.xml
[INFO] run post-build script verify.groovy
[INFO]   The post-build script did not succeed. Import-Package instruction contains version number from test dependency
[INFO]           no-test-scoped-imports\pom.xml ................... FAILED (8.2 s)

@paulrutter
Copy link
Contributor Author

@hboutemy @rotty3000 any feedback on this pr, as you both worked on upgrading bnd last time it was upgraded.

Copy link
Contributor

@rotty3000 rotty3000 left a comment

Choose a reason for hiding this comment

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

LGTM

@paulrutter
Copy link
Contributor Author

What do you think about the failing Integration tests?

@rotty3000
Copy link
Contributor

@paulrutter do this to fix the test:

diff --git a/tools/maven-bundle-plugin/src/it/no-test-scoped-imports/pom.xml b/tools/maven-bundle-plugin/src/it/no-test-scoped-imports/pom.xml
index 3a15568ced..2eedf2350c 100644
--- a/tools/maven-bundle-plugin/src/it/no-test-scoped-imports/pom.xml
+++ b/tools/maven-bundle-plugin/src/it/no-test-scoped-imports/pom.xml
@@ -53,6 +53,11 @@ under the License.
                 <artifactId>maven-bundle-plugin</artifactId>
                 <version>@project.version@</version>
                 <extensions>true</extensions>
+                <configuration>
+                    <instructions>
+                        <_noimportjava>true</_noimportjava>
+                    </instructions>
+                </configuration>
             </plugin>
         </plugins>
     </build>

@rotty3000
Copy link
Contributor

In fact, @paulrutter if you want to preserve the original behaviour of maven-bundle-plugin not to import java.* you may wish to set that bnd flag default to true such that users opt-in by setting it to false.

ref: https://bnd.bndtools.org/instructions/noimportjava.html

@laeubi
Copy link

laeubi commented Jun 3, 2024

I think java packages should simply be always imported, I don't see a reason to disable that, especially as its the default for bnd.

@rotty3000
Copy link
Contributor

whatever you prefer! 🤷🏻

@paulrutter
Copy link
Contributor Author

Great, i will adjust the PR and rerun the tests with that instruction.

+                <configuration>
+                    <instructions>
+                        <_noimportjava>true</_noimportjava>
+                    </instructions>
+                </configuration>
@paulrutter
Copy link
Contributor Author

The only IT still failing was also failing with bnd 6.3.1.

[INFO] --- invoker:3.2.1:run (integration-test) @ maven-bundle-plugin ---
[INFO] Building: archiver\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           archiver\pom.xml ................................. SUCCESS (13.3 s)
[INFO] Building: dep-reduced\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           dep-reduced\pom.xml .............................. SUCCESS (4.9 s)
[INFO] Building: embed-multiple-artifacts\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           embed-multiple-artifacts\pom.xml ................. SUCCESS (4.4 s)
[INFO] Building: existing-metadata-no-update\pom.xml
[INFO] run pre-build script prebuild.groovy
[INFO] run post-build script verify.groovy
[INFO]   The post-build script did not succeed. assert manifest.isEmpty()
       |        |
       |        false
       Manifest-Version: 1.0
       Bnd-LastModified: 1717484827643
       Build-Jdk-Spec: 21
       Bundle-Description: This project has been built before (i.e. a cache fil
        e is there), the current pom.xml has not been touched, nor any of the b
        uilt files
       Bundle-DocURL: https://wcm.io
       Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
       Bundle-ManifestVersion: 2
       Bundle-Name: existing-metadata-no-update
       Bundle-SymbolicName: org.apache.felix.bundleits.existing-metadata-no-upd
        ate
       Bundle-Vendor: wcm.io
       Bundle-Version: 1.0.0.SNAPSHOT
       Created-By: Apache Maven Bundle Plugin 5.1.10-SNAPSHOT
       Tool: Bnd-7.0.0.202310060912
[INFO]           existing-metadata-no-update\pom.xml .............. FAILED (28.2 s)
[INFO] Building: no-test-scoped-imports\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           no-test-scoped-imports\pom.xml ................... SUCCESS (7.9 s)
[INFO] Building: reproducible\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           reproducible\pom.xml ............................. SUCCESS (2.9 s)
[INFO] Building: reproducible-manifest\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           reproducible-manifest\pom.xml .................... SUCCESS (5.7 s)

@paulrutter
Copy link
Contributor Author

I think we should update the major version when we release this, due to the minimum jvm version updated to 17.

@paulrutter paulrutter requested a review from rotty3000 June 4, 2024 08:51
@laeubi
Copy link

laeubi commented Jun 4, 2024

I think we should update the major version when we release this, due to the minimum jvm version updated to 17.

bnd major update > felix major update regardless of java version (but plugin minium java version must be set to 17 then)

@rotty3000
Copy link
Contributor

The only IT still failing was also failing with bnd 6.3.1.

[INFO] --- invoker:3.2.1:run (integration-test) @ maven-bundle-plugin ---
[INFO] Building: archiver\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           archiver\pom.xml ................................. SUCCESS (13.3 s)
[INFO] Building: dep-reduced\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           dep-reduced\pom.xml .............................. SUCCESS (4.9 s)
[INFO] Building: embed-multiple-artifacts\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           embed-multiple-artifacts\pom.xml ................. SUCCESS (4.4 s)
[INFO] Building: existing-metadata-no-update\pom.xml
[INFO] run pre-build script prebuild.groovy
[INFO] run post-build script verify.groovy
[INFO]   The post-build script did not succeed. assert manifest.isEmpty()
       |        |
       |        false
       Manifest-Version: 1.0
       Bnd-LastModified: 1717484827643
       Build-Jdk-Spec: 21
       Bundle-Description: This project has been built before (i.e. a cache fil
        e is there), the current pom.xml has not been touched, nor any of the b
        uilt files
       Bundle-DocURL: https://wcm.io
       Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
       Bundle-ManifestVersion: 2
       Bundle-Name: existing-metadata-no-update
       Bundle-SymbolicName: org.apache.felix.bundleits.existing-metadata-no-upd
        ate
       Bundle-Vendor: wcm.io
       Bundle-Version: 1.0.0.SNAPSHOT
       Created-By: Apache Maven Bundle Plugin 5.1.10-SNAPSHOT
       Tool: Bnd-7.0.0.202310060912
[INFO]           existing-metadata-no-update\pom.xml .............. FAILED (28.2 s)
[INFO] Building: no-test-scoped-imports\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           no-test-scoped-imports\pom.xml ................... SUCCESS (7.9 s)
[INFO] Building: reproducible\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           reproducible\pom.xml ............................. SUCCESS (2.9 s)
[INFO] Building: reproducible-manifest\pom.xml
[INFO] run post-build script verify.groovy
[INFO]           reproducible-manifest\pom.xml .................... SUCCESS (5.7 s)

I did not get this failure when I locally ran the IT tests on your branch yesterday. Could someone else maybe confirm?

@paulrutter
Copy link
Contributor Author

Maybe it's platform related? I'm running on a Windows 11 based machine.

@rotty3000
Copy link
Contributor

Maybe it's platform related? I'm running on a Windows 11 based machine.

.. and I'm running on Ubuntu Linux. Bnd has often had weird windows issues because it's the least tests platform.

@paulrutter
Copy link
Contributor Author

If you don't see the failing test, then i'd say we're good to go.

@rotty3000
Copy link
Contributor

Perhaps you can confirm that src/it/existing-metadata-no-update/prebuild.groovy is actually updating the lastmodified time in Windows. (setLastModified returns a boolean). It could be a windows file locking thing since the file was just written from src to target for the sake of the tests.

@paulrutter
Copy link
Contributor Author

I printed the return value of setLastModified and it returns true:

Running pre-build script: C:\git\felix-dev\tools\maven-bundle-plugin\target\it\existing-metadata-no-update\prebuild.groovy
true

@rotty3000
Copy link
Contributor

.. just a hunch. There are any number of issues it might be. but I just ran again and didn't see that failure. It would be nice if someone else confirmed one way or the other. Barring that, I think you could merge.

@paulrutter
Copy link
Contributor Author

paulrutter commented Jun 4, 2024

I don't have the power to merge though ;)
And someone should create a release for this, updating the major version as well.

@paulrutter
Copy link
Contributor Author

@tjwatson @cziegeler any feedback on this PR?

@paulrutter
Copy link
Contributor Author

@cziegeler i have been running with 7.0.0 for a whole release cycle now and haven't run into any issues. Is there a chance this can be merged and released?

@stbischof
Copy link
Contributor

So we for everyone using felix-bundle-plugin to use java17.

Would update the felix.Framework to java17?

@paulrutter
Copy link
Contributor Author

paulrutter commented Sep 3, 2024

Since it would be a major version upgrade for the plugin (6.0.0?), i don't see a problem with requiring 17.
Also, since the bundle plugin can be used apart from felix itself, we probably don't need to enforce 17 as a baseline for felix framework itself.

@cziegeler
Copy link
Contributor

I agree that updating to bnd 7 and requiring java 17 for the tool chain is probably not a huge problem. However, I'm too far away from the bundle-plugin code to make a call here.

@paulrutter
Copy link
Contributor Author

@rotty3000 maybe?

@rotty3000
Copy link
Contributor

man... I've not looked at this for such a long time I would be remise to make a recommendation such out of date info. Perhaps @jbonofre would have better insight?

@stbischof
Copy link
Contributor

@paulrutter since this plugin is justr a wrapper around bnd why not using bnd-maven-plugin?

@paulrutter
Copy link
Contributor Author

paulrutter commented Sep 4, 2024

I looked at bnd-maven-plugin and although that would probably work, i find the configuration (in a CDATA tag in the pom.xml) less convenient/readable than the maven-bundle-plugin.
Also, we have a lot of OSGi bundles (40+) which would be quite some rework (with regression risk).

So ideally, i would like to have an updated maven-bundle-plugin that uses bnd 7.0.0.

@laeubi
Copy link

laeubi commented Sep 4, 2024

Sad to say but bnd-maven plugin even misses some features of felix-maven-plugin, beside that +1 from me to simply upgrade (with or without major version increment) it all looks fine and sane and should has no impact to the rest of felix.

@paulrutter
Copy link
Contributor Author

paulrutter commented Sep 23, 2024

Who could give the final go for this to get merged and released? Would be a shame to leave it on the shelve unused, but maybe I'm just too impatient 😉

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.

5 participants