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

Fix OSGi manifest for no_aop: remove unnecessary aopalliance package #1355

Closed

Conversation

ctranxuan
Copy link

The MANIFEST.MF of the no_aop jar file shouldn't declare
an OSGi import package of org.aopalliance.intercept since no aop
is used.
The issue comes from the fact the maven-bundle-plugin generates
the MANIFEST.MF from the orginal source files and not the munged
source files for the 'guice.with.no_aop' profile.
This commit fixes the issue by:

  • defining the proper source location for the maven-bundle-plugin
    for the "aop" and "no_aop" profile
  • changing the phase of munge-maven-plugin to "generate-sources":
    this ensures the munge-maven-plugin is called before the
    maven-bundle-plugin, so that this latter one can use the proper
    source files to generate the appropriate MANIFEST.MF

By removing this import, using the guice_no_aop.jar within an OSGi
container does no more require to deploy an OSGi bundle of the aopalliance.

@googlebot
Copy link

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

core/pom.xml Outdated
@@ -114,6 +118,7 @@
<groupId>org.apache.felix</groupId>
<artifactId>maven-bundle-plugin</artifactId>
<configuration>
<scrLocation>${src.osgi.location}</scrLocation>
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the related property changes actually has no effect because this setting controls where SCR (ie. declarative services) files are generated and not where the source is loaded from. In fact the bundle plugin analyses the bytecode to generate OSGi metadata and doesn't scan the Java source.

The reason the aopalliance import is appearing in the 'no-aop' jar is because bundleplugin 3.5.0 attempts to merge in headers from the main jar into forked builds, like the 'no_aop' munged build. This overwrites the custom 'no_aop' headers.

So the key difference is your change to the munge-maven-plugin phase on line 184 - changing that to generate-sources means the bundleplugin is no longer merging in those headers. I have an earlier pending PR that solved this a different way: #1173 - but both solutions are valid, just drop the SCR related changes here because they aren't necessary

Copy link
Author

@ctranxuan ctranxuan Jul 8, 2020

Choose a reason for hiding this comment

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

Damn, I was so focused on how configure the maven-bundle-plugin with another source directory that I've read srcLocation instead of scrLocation :/ And I didn't spot your PR was also fixing the issue. Thanks for the review!
I'll update the PR and let the Guice team decide what PR should be applied. Yours is fixing more stuff.

The MANIFEST.MF of the no_aop jar file shouldn't declare
an OSGi import package of org.aopalliance.intercept since no aop
is used.
The issue comes from the fact the maven-bundle-plugin generates
the MANIFEST.MF from the orginal source files and not the munged
source files for the 'guice.with.no_aop' profile.
This commit fixes the issue by changing the phase of munge-maven-plugin
to "generate-sources": this ensures the bundleplugin is no longer merging
in the headers from the main jar into the forked build and thus,
generates the appropriate MANIFEST.MF.

By removing this import, using the guice_no_aop.jar within an OSGi
container does no more require to deploy an OSGi bundle of the aopalliance.
@sameb
Copy link
Member

sameb commented Apr 18, 2023

Closing this b/c there's no more no_aop variant of Guice.

@sameb sameb closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants