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

Patch candidate for OSGi support #6

Merged
merged 2 commits into from
Apr 23, 2015
Merged

Conversation

pderop
Copy link
Contributor

@pderop pderop commented Apr 22, 2015

I changed the pom.xml in order to generate an OSGi bundle, using maven-bundle-plugin.

Also, while doing a real test under Felix, I realized that it was not working. I figured out that the TypeResolver class was doing a class.forName in order to load actual type classes, but we can't do this within an OSGi environment, and the bundle classloader has to be used.
So, I added a patch in TypeDescriptor.java, in order to use the bundle classloader of the lambda expression. It seems to work.

Please review, thanks.

PS: first time I'm using github ... hope my pull request is correct ...

jhalterman and others added 2 commits April 21, 2015 13:43
- Modified pom.xml in order to generate OSGi manifest headers (using maven-bundle-plugin).
- Patched TypeDescriptor.getType() method in order to use bundle classloader.
@jhalterman
Copy link
Owner

This looks pretty good to me. I'm not an OSGI user so I can't verify that the bundle works - but I assume it does? :)

@kuujo
Copy link

kuujo commented Apr 22, 2015

Awesome! Welcome to GitHub!

On Apr 22, 2015, at 11:02 AM, pderop [email protected] wrote:

I changed the pom.xml in order to generate an OSGi bundle, using maven-bundle-plugin.

Also, while doing a real test under Felix, I realized that it was not working. I figured out that the TypeResolver class was doing a class.forName in order to load actual type classes, but we can't do this within an OSGi environment, and the bundle classloader has to be used.
So, I added a patch in TypeDescriptor.java, in order to use the bundle classloader of the lambda expression. It seems to work.

Please review, thanks.

PS: first time I'm using github ... hope my pull request is correct ...

You can view, comment on, or merge this pull request online at:

#6

Commit Summary

Doc fix
Patch candidate for the support of OSGi:
File Changes

M README.md (4)
M pom.xml (18)
M src/main/java/net/jodah/typetools/TypeDescriptor.java (16)
M src/main/java/net/jodah/typetools/TypeResolver.java (6)
Patch Links:

https://github.com/jhalterman/typetools/pull/6.patch
https://github.com/jhalterman/typetools/pull/6.diff

Reply to this email directly or view it on GitHub.

@pderop
Copy link
Contributor Author

pderop commented Apr 22, 2015

Yes Jonathan, it works, I tested with Felix 4.6.1.

Now, ultimately, it would be nice to see if we can add an integration test
in your pom, using pax-exam, in order to execute some tests under Osgi ...
I will see if I can do this (but I'm affraid I won't have time to do this
quickly).

Once remark: the TypeTools OSGi manifest is importing the package
"sun.reflect" ("Import-Package: sun.reflect" in the OSGi manifest headers).

So, I think that in your documentation, you should mention that and say
that the OSGi framework should "reexport" the "sun.reflect" package to the
"system bundles", like the following using Felix:

org.osgi.framework.system.packages.extra=sun.reflect

So, I suggest to document:


OSGi support: When using TypeTools in an OSGi environment, the
"sun.reflect" system package should be exported by the framework to the
application bundles, because TypeTools is importing it.

For example, in Felix, configure the following in the config.properties
file:

org.osgi.framework.system.packages.extra=sun.reflect

/Pierre

On Wed, Apr 22, 2015 at 8:38 PM, Jonathan Halterman <
[email protected]> wrote:

This looks pretty good to me. I'm not an OSGI user so I can't verify that
the bundle works - but I assume it does? :)


Reply to this email directly or view it on GitHub
#6 (comment).

jhalterman added a commit that referenced this pull request Apr 23, 2015
Patch candidate for OSGi support
@jhalterman jhalterman merged commit db79c07 into jhalterman:master Apr 23, 2015
@jhalterman
Copy link
Owner

Looks good. I don't think an integration test is necessary. Thanks for this!

@jhalterman
Copy link
Owner

0.4.1 has been released. It should be in central shortly..

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.

3 participants