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

Fully automate OSGi metadata creation and fix broken OSGi-metadata in slf4j-api #330

Merged
merged 2 commits into from Mar 15, 2023

Conversation

HannesWell
Copy link
Contributor

@HannesWell HannesWell commented Dec 19, 2022

This is a follow-up on #324 and a reduced version of #327.

It would be possible to further reduce the number of changes by using the maven-bundle-plugin and its ability to merge the generated OSGi Manifest content with a already existing one. But spliting the OSGi metadata in an automatically generated part controlled in the pom and a static part controlled in MANIFEST.MF files IMHO makes it eventually harder to understand.
Nevertheless if you insist to make this change as minimalist as possible I can change this PR accordingly.

What this PR is doing is, to only use the bnd-maven-plugin to generate all OSGi metadata into slf4j's Manifest.MF files. This unifies the resulting MANIFEST.MFs that are currently partly generated and partly statically defined and fixes the following aspects:

  • Restore the Bundle-SymbolicName from SLF4J-2.0.5 and before
  • Removes Export-Package: META-INF.versions.9
  • Restricts the slf4j self import versions to [,3)
  • Fix the package-export of slf4j-jdk-platform-logging (from 'slf4j.jdk.platform.logging' to 'org.slf4j.jdk.platform.logging')
  • Import missing packages for slf4j-reload4j and log4j-over-slf4j
  • Adds the 'Main-Class' header for the migrator artifact. Because the Manifest.MF did not have a terminating new-line it was invalid and its content was ignored.
  • Replace the deprecated 'Bundle-RequiredExecutionEnvironment' by a
    corresponding required 'osgi.ee' capability

@HannesWell
Copy link
Contributor Author

It would be possible to further reduce the number of changes by using the maven-bundle-plugin and its ability to merge the generated OSGi Manifest content with a already existing one. But spliting the OSGi metadata in an automatically generated part controlled in the pom and a static part controlled in MANIFEST.MF files IMHO makes it eventually harder to understand. Nevertheless if you insist to make this change as minimalist as possible I can change this PR accordingly.

I did that now to further reduce the number of changes and removed some restriction (that where applied to make the generated metadata as similar as before), which mainly add some before missing metadata. I have not tired all bridges in an OSGi environment, but for some I suspect that they are currently not working in an OSGi runtime, due to that missing Package imports.

@HannesWell
Copy link
Contributor Author

@ceki could you please review this change?
Together with #331 this blocks the migration of Eclipse to slf4j-2 (see eclipse-platform/eclipse.platform.releng.aggregator#588).

If it makes the review simpler for you I can further reduce this to only adjust the root pom.xml and the slf4j-api module and do the other changes in a subsequent PR.

@HannesWell HannesWell force-pushed the automateOSGiMetadata_2 branch 2 times, most recently from 4711a8b to 317b6db Compare January 12, 2023 22:27
@HannesWell
Copy link
Contributor Author

If it makes the review simpler for you I can further reduce this to only adjust the root pom.xml and the slf4j-api module and do the other changes in a subsequent PR.

Did that now, as you requested in #327 (comment).

Once this is merged I can create a follow-up with the remaining modules. If you prefer I can split that in smaller chunks as well.

@HannesWell
Copy link
Contributor Author

@ceki can you please have a look at this?

@HannesWell
Copy link
Contributor Author

HannesWell commented Feb 7, 2023

@ceki is there any chance that this and #331 will land soon?
It would be very helpful for the spring release of Eclipse. Without these two PRs adopting slf4j-2 will be much harder for the Eclipse eco-system.

@HannesWell
Copy link
Contributor Author

@ceki Is there anything I can do to speed up the completion of this PR?
@jonahgraham can you tell me what you did to get your PR completed quicker?

@ceki
Copy link
Member

ceki commented Feb 24, 2023

@HannesWell I will look into this issue in the next few days. Maybe we can have a chat on Google meet or similar?

@jonahgraham
Copy link
Contributor

@jonahgraham can you tell me what you did to get your PR completed quicker?

I see that @ceki has already responded, but I didn't do anything. My guess as a fellow FOSS maintainer is that I got lucky and caught @ceki on a quiet day (or one when they wanted a distraction). Sometimes I manage to review incoming PRs within hours, and sometimes it takes weeks.

@ceki
Copy link
Member

ceki commented Feb 24, 2023

I think PRs consisting for baby steps also helps.

@HannesWell
Copy link
Contributor Author

@HannesWell I will look into this issue in the next few days. Maybe we can have a chat on Google meet or similar?

Great, thank you in advance. I send you a message on the mail-address you use in your slf4j-commits.

I think PRs consisting for baby steps also helps.

Fully understand. I already made this change much smaller compared to the initial PR, which covered all slf4j modules.
Theoretically I could even split this PR into two, one covering the changes in the root pom and one that covers the changes in the slf4j-api module. So just let me know if you prefer it that way.

@jonahgraham can you tell me what you did to get your PR completed quicker?

I see that @ceki has already responded, but I didn't do anything. My guess as a fellow FOSS maintainer is that I got lucky and caught @ceki on a quiet day (or one when they wanted a distraction). Sometimes I manage to review incoming PRs within hours, and sometimes it takes weeks.

Yes, I know. I was just a bit desperate, hoping that this PR is not forgotten and that I don't annoy with my gentle reminders.

@HannesWell HannesWell changed the title Fully automate OSGi metadata creation and fix brocken OSGi-metadata Fully automate OSGi metadata creation and fix brocken OSGi-metadata in slf4j-api Feb 24, 2023
Enhance the generated OSGi metadata for all slf4j-modules in the
following ways:
- Restore the Bundle-SymbolicName, Bundle-Name, Bundle-Vendor and
Bundle-DocURL from SLF4J-2.0.5 and before
- Removes Export-Package: META-INF.versions.9
- Removes self-Imports of exported package (has to be added again in a
follow up, where desired)
- Remove unnecessary BND-internal headers

Additionally move the maven-bundle-plugin configuration from the
execution configuration up into the configuration of the plugin so that
it can be easier overwritten in child-modules.

Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
Only use the maven-bundle-plugin to generate all OSGi metadata into
slf4j-api's Manifest.MF file. This unifies the resulting MANIFEST.MFs
that are currently partly generated and partly statically defined.

Furthermore it fixes the following flaws in the currently generated OSGi
metadata of slf4j-api:
- Import only the exported org.slf4j.spi package (like in slf4j 2.0.5
and before)
- Replace the deprecated 'Bundle-RequiredExecutionEnvironment' header by
a corresponding required 'osgi.ee' capability

Signed-off-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
@HannesWell
Copy link
Contributor Author

HannesWell commented Feb 24, 2023

I think PRs consisting for baby steps also helps.

Fully understand. I already made this change much smaller compared to the initial PR, which covered all slf4j modules. Theoretically I could even split this PR into two, one covering the changes in the root pom and one that covers the changes in the slf4j-api module. So just let me know if you prefer it that way.

Already split this PR into two commits that I also can submit in two PRs. @ceki just let me know if you prefer to have the second one in a second PR.

@HannesWell
Copy link
Contributor Author

@HannesWell I will look into this issue in the next few days. Maybe we can have a chat on Google meet or similar?

@ceki did you have some time to look into this?

@ceki ceki merged commit 2235d3c into qos-ch:master Mar 15, 2023
0 of 9 checks passed
@ceki ceki changed the title Fully automate OSGi metadata creation and fix brocken OSGi-metadata in slf4j-api Fully automate OSGi metadata creation and fix broken OSGi-metadata in slf4j-api Mar 15, 2023
@HannesWell HannesWell deleted the automateOSGiMetadata_2 branch March 15, 2023 21:29
@HannesWell
Copy link
Contributor Author

Thank you @ceki for your review and merging.
Could you be so kind and also have a look at #331, which would also be of great interest.

@ceki
Copy link
Member

ceki commented Mar 16, 2023

@HannesWell There are several directives in the Felix Bundle Plug-in that are included in this PR that I do not understand.

For example, what does "<_exportcontents>!META-INF.versions.9,*;-noimport:=true</_exportcontents>" do?

@HannesWell
Copy link
Contributor Author

For example, what does "<_exportcontents>!META-INF.versions.9,*;-noimport:=true</_exportcontents>" do?

The value !META-INF.versions.9 prevents that a package named META-INF.versions.9 is exported for those modules that have a src/main/java9/module-info.java file. The maven-bundle-plugin respectively the underlying bnd-tools library unfortunately adds said package by default. But that's not a package that contains java classes.
You can see that for example in the META-INF/MANIFEST.MF in
https://repo1.maven.org/maven2/org/slf4j/slf4j-api/2.0.6/slf4j-api-2.0.6.jar

On the other hand *;-noimport:=true exports all packages found in a artifact (that are not META-INF.versions.9), but prevents the BND-Tool from importing not import them.
From https://bnd.bndtools.org/heads/export_package.html

Exports are automatically imported. This features can be disabled with a special directive on the export instruction: -noimport:=true. For example:

Export-Package= com.acme.impl.*;-noimport:=true, *

The reason is that the self-import of packages exported by a bundle can occasional cause problems and for API artifacts like slf4j it is actually not really useful. Furthermore that was the structure of the package exports/imports before #324.

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

3 participants