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

Set correct bundle version of fragment host. #1050

Closed
wants to merge 1 commit into from

Conversation

kenwenzel
Copy link
Contributor

* Fixes google#1049
* Ensures that extensions are only applied to a specific version of com.google.inject
@googlebot
Copy link

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).

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

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@kenwenzel
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@sameb
Copy link
Member

sameb commented Apr 20, 2023

Hello. It has been many many years since this PR was opened. I have finally attempted to finally merge it, but the integration tests fail with Error: Manifest com.google.inject.extensions:guice-throwingproviders:jar:5.1.1-SNAPSHOT : Fragment-Host com.google.inject specifies invalid version range 5.1.1-SNAPSHOT.

I am unfortunately not familiar enough with the OSGi to resolve this. Do you have suggestions for how to fix this?

(@mcculls , you may have an idea too.)

For reference, this is the PR that is trying to submit and failing: #1701, and you can see an example failure from the CI tests @ https://github.com/google/guice/actions/runs/4758367659/jobs/8456337740.

@kenwenzel
Copy link
Contributor Author

Hello. It has been many many years since this PR was opened. I have finally attempted to finally merge it, but the integration tests fail with Error: Manifest com.google.inject.extensions:guice-throwingproviders:jar:5.1.1-SNAPSHOT : Fragment-Host com.google.inject specifies invalid version range 5.1.1-SNAPSHOT.

I am unfortunately not familiar enough with the OSGi to resolve this. Do you have suggestions for how to fix this?

(@mcculls , you may have an idea too.)

For reference, this is the PR that is trying to submit and failing: #1701, and you can see an example failure from the CI tests @ https://github.com/google/guice/actions/runs/4758367659/jobs/8456337740.

The bundle version needs to be normalized to 5.1.1.SNAPSHOT. This version must also match the exact Bundle-Version of com.google.inject

@sameb
Copy link
Member

sameb commented Apr 21, 2023

Thanks @kenwenzel. Are there any variables I can use in the POM (or anything, really) that will get the normalized version or refer to the bundle-version used in the parent? AFAICT, the bundle-plugin will automatically normalize the ${project.version} to auto-add to the <Bundle-Version>... but if I'm explicitly setting it within the <Fragment-Host>, then it just copies it verbatim (not normalized). I can workaround by spelling it out explicitly and trying to make sure it's kept up-to-date... but I'd rather not do that.

@kenwenzel
Copy link
Contributor Author

kenwenzel commented Apr 22, 2023

Dear @sameb ,
the cleanest solution would be a switch to:
https://github.com/bndtools/bnd/tree/master/maven-plugins/bnd-maven-plugin

Then we can simply use a built-in macro for version normalization:
https://bnd.bndtools.org/macros/version_cleanup.html

All macros can be found here:
https://bnd.bndtools.org/chapters/855-macros-ref.html

Can I help somehow?

@sameb
Copy link
Member

sameb commented Apr 22, 2023

Help would be greatly appreciated. If you're willing to update this PR (or make another) with a version that works at head and normalizes snapshots, etc .. that would be awesome.

Thank you for your help, and my sincere apologies again for the many years of earlier silence on this issue.

@mcculls
Copy link
Contributor

mcculls commented Apr 22, 2023

Hi, since the maven-bundle-plugin also uses bnd to generate the manifest then you can use the same solution without needing to change the plugin. The root cause is that Maven interpolates the property before bnd sees it - at which point it becomes hard for bnd to automatically correct the version syntax without potentially masking real issues.

The solution is to use a property syntax that Maven won't interpolate, but bnd will - such as $(project.version)

ie. use parentheses () instead of the usual braces {}

@sameb
Copy link
Member

sameb commented Apr 22, 2023

Perfect, thanks @mcculls !

@mcculls
Copy link
Contributor

mcculls commented Apr 22, 2023

Looks like bnd is still not cleaning the version - let me check the right macro to use for that

@mcculls
Copy link
Contributor

mcculls commented Apr 22, 2023

Using the macro @kenwenzel suggested works for me locally:

diff --git a/extensions/pom.xml b/extensions/pom.xml
index 8ce3e145e..70626e2f7 100644
--- a/extensions/pom.xml
+++ b/extensions/pom.xml
@@ -92,7 +92,7 @@
         <artifactId>maven-bundle-plugin</artifactId>
         <configuration>
           <instructions>
-            <Fragment-Host>com.google.inject</Fragment-Host>
+            <Fragment-Host>com.google.inject;bundle-version=$(version_cleanup;${project.version})</Fragment-Host>
           </instructions>
         </configuration>
       </plugin>

(Maven will interpolate the project version before passing the header to bnd - at which point bnd will process the macro)

copybara-service bot pushed a commit that referenced this pull request Apr 22, 2023
… Much thanks to @kenwenzel and @mcculls for their help figuring out the right incantation for this.

PiperOrigin-RevId: 525810926
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.

[OSGi] Multibindings and AssistedInject JARs need version property for fragment host
4 participants