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

Use BND for both OSGI and JPMS #1819

Merged
merged 22 commits into from
Oct 4, 2023
Merged

Use BND for both OSGI and JPMS #1819

merged 22 commits into from
Oct 4, 2023

Conversation

ppkarwasz
Copy link
Contributor

@ppkarwasz ppkarwasz commented Sep 26, 2023

This PR is a work in progress and replaces #1815.

It replaces maven-bundle-plugin with bnd-maven-plugin, which:

  • is maintained by the same project that releases the common backend BND, so our build system depends on one less project (Apache Felix),
  • is released at each upgrade of the BND backend.

The introduction of JPMS support in BND allows us:

  • to have real JPMS module descriptors that are verified to be complete at a bytecode level (even Class.forName calls are supported by BND).

To help in the review process, BND configurations for various modules will have their own PRs and this one will be complete when modifications to all modules will be accepted:

@ppkarwasz
Copy link
Contributor Author

Using BND we still need to manually overwrite the detected (or undetected) module name of multi-release dependency JARs. Theoretically this was fixed in bndtools/bnd#5327, but I can't get it to work. The module name of MRJs with a module descriptor in their META-INF/versions/9 folder isn't

@HannesWell, do you know what am I missing?

The following packages are no longer exported:

 *  org.apache.logging.log4j.core.layout.internal
 *  org.apache.logging.log4j.core.message
 *  org.apache.logging.log4j.core.time.internal
 *  org.apache.logging.log4j.core.tools.picocli
 *  org.apache.logging.log4j.core.util.internal

A new JPMS module is generated, which sets the following Java modules as
optional:
 *  java.compiler,
 *  java.logging,
 *  java.naming,
 *  java.sql.
Differences in the module descriptor:
 * `java.management` is added to the optional requires.

Differences in the OSGi descriptor:
 * the import statement on `sun.reflect` disappears,
 * an import statement on `org.apache.logging.log4j.internal` appears.
@ppkarwasz
Copy link
Contributor Author

Apparently the BND (partial) fix is only in the 7.1.0-SNAPSHOT branch.

@ppkarwasz
Copy link
Contributor Author

Closes #1830.

JARs without either an `Automatic-Module-Name` header nor a module
descriptor MUST be optional.
@ppkarwasz ppkarwasz merged commit c4e6d29 into 2.x Oct 4, 2023
7 checks passed
@ppkarwasz ppkarwasz deleted the bnd branch October 4, 2023 13:31
@HannesWell
Copy link
Contributor

Sorry for the late and after the fact reply. I just checked the following slf4j bundles in more detail and everything looks good and worked well in my testing in our two OSGi applications, from which one uses log4j-core as back-end and redirects every other API to it and the other one uses logback-classic and redirects all APIs to that:

  • log4j-api
  • log4j-core
  • log4j-slf4j2-impl
  • log4j-slf4j-impl
  • log4j-to-slf4j (Especially the extended version range in Import-Package: org.slf4j;version="[1.7,3)",org.slf4j.spi;version="[1. 7,3)", ... is important)

I noted that for some bundles the symbolic name changed e.g. from o.a.l.log4j.slf4j2-impl to o.a.l.log4j.slf4j2.impl. In some cases this requires adjustments, e.g. if one requires the bundle instead of importing the contained packages. But since the former is discouraged anyways it shouldn't be a blocker and is IMHO fine.

If you want to keep the MANIFEST.MF a little bit cleaner you could remove the Private-Package, which is only for internal use of BND and could be removed with the instruction -removeheaders: Private-Package.

Thank you @ppkarwasz for this work!

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Oct 5, 2023

@HannesWell,

That is great news, so I believe we have a green light to release 2.21.0, unless you have an idea what is going on in #1741.

Regarding the bundle names, we chose to prefer consistency (use bundle names that can be module names) over backward compatibility. If you can think of a place where bundle names are used (build scripts?), we can revert them to the original form.

Now we are just waiting for INFRA-25032.

@HannesWell
Copy link
Contributor

That is great news, so I believe we have a green light to release 2.21.0, unless you have an idea what is going on in #1741.

From my point of view yes 🚀🙂

Regarding the bundle names, we chose to prefer consistency (use bundle names that can be module names) over backward compatibility. If you can think of a place where bundle names are used (build scripts?), we can revert them to the original form.

That's a reasonable choice. I'm know cases that depend on the bundle-name, for example if another Bundle requires a log4j bundle using OSGi's Require-Bundle (but that's discouraged/deprecated and as far as I know mainly used in the Eclipse world only. And since you provide good OSGi metadata, i.e. versioned packages, there is no reason to not used Import-Package instead, which only depends on the package name). Or if the bundles are part of an Eclipse Feature (which is basically a group of Bundles that form a 'feature' of an Eclipse Product like the IDE), but there names can be changed too and usually a feature once build is pinned to a specific version of the Bundle so there is no compatibility problem.
So IMHO the name change is fine, personally I find bundle-names with a dash a bit odd, but it should be mentioned in the release notes so that there is a change to simplify migration.

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

2 participants