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

#1981: Optional OSGi bundle's dependency on sun.misc package #1993

Merged
merged 8 commits into from Oct 25, 2021

Conversation

@JaroslavTulach
Copy link
Contributor

@JaroslavTulach JaroslavTulach commented Oct 14, 2021

As #1981 wrotes: The Apache NetBeans project has just tried to migrate from version 2.8.5 to version 2.8.8 in the pull request apache/netbeans#3200 and the update fails with unsatisfied OSGi missing constraint: Import-Package: sun.misc. The best solution is to avoid such dependency.

The PR introduces a test that verifies Import-Package statement in GSON JAR's manifest no longer contains reference to sun.misc. Then this PR modifies bnd configuration file to explicitly avoid sun.misc package from list of imported packages.

@google-cla
Copy link

@google-cla google-cla bot commented Oct 14, 2021

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.

@dbalek
Copy link

@dbalek dbalek commented Oct 14, 2021

The Apache NetBeans IDE seems to work fine with the Gson library build based on this change.

Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

This is great, thanks! We do need the CLA, though.

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 14, 2021

Will this still work when an OSGi user wants to use Unsafe with Gson, for example to create instances of classes without default constructor? Or will OSGi users now be unable to use Unsafe at all with Gson?
I am not very familiar with OSGi, but would an optional import be a better solution (as mentioned in your issue)?

It appears OSGi integrates to some extend with the Java module system (see blog post), so maybe this is also related to #1695?

@JaroslavTulach
Copy link
Contributor Author

@JaroslavTulach JaroslavTulach commented Oct 15, 2021

This is great, thanks! We do need the CLA, though.

Working on it, but it is a process...

Will this still work when an OSGi user wants to use Unsafe with Gson,
for example to create instances of classes without default constructor?
Or will OSGi users now be unable to use Unsafe at all with Gson?

...alas, that may be the case...

I am not very familiar with OSGi, but would
an optional import
be a better solution (as mentioned in your issue)?

...feel free to implement the fix for #1981 in a better way on your own without waiting for my CLA. optional dependency would very likely work for us as well. Me and @dbalek can do manual testing (including check for classes without default constructor) once the better fix is proposed even in a different PR.

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 21, 2021

@JaroslavTulach could you please give the changes in #1999 a try? The OSGi metadata in the generated MANIFEST.MF file looks reasonable to me, but I don't have a testing setup.

Though I am not a member of this project so maybe that pull request has to be adjusted before it can be merged.

@JaroslavTulach
Copy link
Contributor Author

@JaroslavTulach JaroslavTulach commented Oct 22, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 22, 2021
@google-cla
Copy link

@google-cla google-cla bot commented Oct 22, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Oct 22, 2021
@JaroslavTulach JaroslavTulach changed the title #1981: Avoid OSGi bundle's dependency on sun.misc package #1981: Optional OSGi bundle's dependency on sun.misc package Oct 22, 2021
@google-cla
Copy link

@google-cla google-cla bot commented Oct 22, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 22, 2021

@googlebot I consent.

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 22, 2021
Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Could you please add the following to the description of this pull request so GitHub closes the corresponding issues (or wait until a maintainer manually links the issues):

Resolves #1695
Resolves #1981

Additionally, if possible, could you please test whether the following situations work (copied from #1999):

  • works for OGSi deployments without Unsafe
  • works for OSGi deployments with Unsafe and Gson is able to construct instances of class without default constructor (by using Unsafe)
    • With System property org.osgi.framework.bootdelegation=sun.misc.*
    • With System property org.osgi.framework.system.packages.extra=sun.misc
    • Using extension bundle from https://github.com/diffplug/osgiX (?)

@dbalek
Copy link

@dbalek dbalek commented Oct 22, 2021

The Apache NetBeans IDE seems to work fine with the Gson library build based on the last changes to this pull request.

@Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 23, 2021

Have done some basic testing based on the setup described in https://www.baeldung.com/osgi with Apache Karaf by editing its etc/config.properties to exclude sun.* / sun.misc from org.osgi.framework.bootdelegation and org.osgi.framework.system.packages.extra.

Gson 2.8.8 Pull request
bootdelegation + extra-package ✔️ ✔️
extra-package ✔️ ✔️
bootdelegation (fails due to missing requirement for sun.misc) ✔️
extension-bundle ✔️ ✔️
- (fails due to missing requirement for sun.misc) ⚠️ (bundle starts, but fails during runtime because no custom Gson InstanceCreator is registered and Unsafe is not available; as expected)

Key:

  • bootdelegation: sun.* set as org.osgi.framework.bootdelegation value
  • extra-package: sun.misc set as org.osgi.framework.system.packages.extra value
  • extension-bundle: extension bundle from https://github.com/diffplug/osgiX being installed

Note that I have not tested how Karaf interacts with the Java Platform Module System.

@eamonnmcmanus eamonnmcmanus linked an issue that may be closed by this pull request Oct 25, 2021
@eamonnmcmanus eamonnmcmanus linked an issue that may be closed by this pull request Oct 25, 2021
@eamonnmcmanus
Copy link
Member

@eamonnmcmanus eamonnmcmanus commented Oct 25, 2021

It looks as if this is ready to go, so I'm merging it. Thanks to everyone who helped out here!

@eamonnmcmanus eamonnmcmanus merged commit ca1df7f into google:master Oct 25, 2021
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants