Uploaded image for project: 'OpenJPA'
  1. OpenJPA
  2. OPENJPA-2487

upgrade openjpa to asm5 to support java 8

    Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4.0
    • Component/s: None
    • Labels:
      None
    1. openjpa-asm5.patch
      1 kB
      Romain Manni-Bucau
    2. openjpa-asm5-release.patch
      1 kB
      Romain Manni-Bucau

      Issue Links

        Activity

        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Patch uses xbean snapshot while not released

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Patch uses xbean snapshot while not released
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        ready to release

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - ready to release
        Hide
        curtisr7 Rick Curtis added a comment -

        Do the unit tests pass with this change?

        Show
        curtisr7 Rick Curtis added a comment - Do the unit tests pass with this change?
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Actually I was never able to get a clean run of openjpa (I mean since I looked the project years ago) so no. We can discuss some issue son IRC (freenode, I'm on openejb for instance - not sure openjpa has one active) if you think it is faster to understand the issue you have .

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Actually I was never able to get a clean run of openjpa (I mean since I looked the project years ago) so no. We can discuss some issue son IRC (freenode, I'm on openejb for instance - not sure openjpa has one active) if you think it is faster to understand the issue you have .
        Hide
        kwsutter Kevin Sutter added a comment -

        The patch looks good from a Java 7 perspective. I've tested with Java 7 and your asm 5 xbean shaded jar. Everything looks good. From that perspective, we should be okay with integrating this change into trunk. When I have time, I'll try junit testing with Java 8. (FYI, we are not going to change OpenJPA to build with Java 8.)

        Show
        kwsutter Kevin Sutter added a comment - The patch looks good from a Java 7 perspective. I've tested with Java 7 and your asm 5 xbean shaded jar. Everything looks good. From that perspective, we should be okay with integrating this change into trunk. When I have time, I'll try junit testing with Java 8. (FYI, we are not going to change OpenJPA to build with Java 8.)
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment - - edited

        Well the goal is just to let the code supports java 8 (same constraint for CXF, OWB, OpenEJB and TomEE)

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - - edited Well the goal is just to let the code supports java 8 (same constraint for CXF, OWB, OpenEJB and TomEE)
        Hide
        kwsutter Kevin Sutter added a comment -

        Right. Unfortunately, Java8 has introduced some incompatibilities with the OpenJPA test suite. As soon as I try to build the junit test suite with Java 8 (and asm 5), I immediately hit the issue documented by openjpa-2442. So, we could go ahead with your patch since it doesn't affect the Java 7 usage pattern. But, it does not resolve the overall issue of using Java 8 with OpenJPA applications.

        Show
        kwsutter Kevin Sutter added a comment - Right. Unfortunately, Java8 has introduced some incompatibilities with the OpenJPA test suite. As soon as I try to build the junit test suite with Java 8 (and asm 5), I immediately hit the issue documented by openjpa-2442. So, we could go ahead with your patch since it doesn't affect the Java 7 usage pattern. But, it does not resolve the overall issue of using Java 8 with OpenJPA applications.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        hmm,

        well there are multiple things here:
        1) in tomee we absolutely need an openjpa release with asm5 since we'll not ship asm3/4 anymore
        2) is 2442 linked to dynamic proxying? If so we stil lhave static enhancement. If not openjpa enhancement should be rewritten with full asm support since serp is not upgraded anymore. Can be a bit too long so a pre-release with at least asm5 would be very welcomed (otherwise tomee will fork again to release which is a pain for everyone )

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - hmm, well there are multiple things here: 1) in tomee we absolutely need an openjpa release with asm5 since we'll not ship asm3/4 anymore 2) is 2442 linked to dynamic proxying? If so we stil lhave static enhancement. If not openjpa enhancement should be rewritten with full asm support since serp is not upgraded anymore. Can be a bit too long so a pre-release with at least asm5 would be very welcomed (otherwise tomee will fork again to release which is a pain for everyone )
        Hide
        curtisr7 Rick Curtis added a comment -

        Does TomEE need to be built at a Java 1.8 source level, or just needs to run on java 1.8?

        > 2) is 2442 linked to dynamic proxying? If so we still have static enhancement.
        Static enhancement will not solve this problem. This bug is related to our dynamic proxies that get created for tracking non-standard types.

        > If not openjpa enhancement should be rewritten with full asm support since serp is not upgraded anymore.
        Great idea, but it is not a trivial task... and I don't know who is going to step up and do the work.

        Show
        curtisr7 Rick Curtis added a comment - Does TomEE need to be built at a Java 1.8 source level, or just needs to run on java 1.8? > 2) is 2442 linked to dynamic proxying? If so we still have static enhancement. Static enhancement will not solve this problem. This bug is related to our dynamic proxies that get created for tracking non-standard types. > If not openjpa enhancement should be rewritten with full asm support since serp is not upgraded anymore. Great idea, but it is not a trivial task... and I don't know who is going to step up and do the work.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        No tomee just need to work with a java 8 runtime.

        What I dont get is why needing dynamic proxies (I have to admit I almost only know enhancing of a single entity)? I thought (surely wrongly) dynamic proxies were related to collections and if so you expected 2 cases: 1) proxying the collection is just a standard proxy (java.lang.reflect), 2) proxying elements can be done reusing the static enhancement

        I can surely help to use asm but I need some pointers, maybe some tests to pass to work on it.

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - No tomee just need to work with a java 8 runtime. What I dont get is why needing dynamic proxies (I have to admit I almost only know enhancing of a single entity)? I thought (surely wrongly) dynamic proxies were related to collections and if so you expected 2 cases: 1) proxying the collection is just a standard proxy (java.lang.reflect), 2) proxying elements can be done reusing the static enhancement I can surely help to use asm but I need some pointers, maybe some tests to pass to work on it.
        Hide
        curtisr7 Rick Curtis added a comment -

        > No tomee just need to work with a java 8 runtime.
        I believe with java 8 that we can do that today.... mostly.

        > What I dont get is why needing dynamic proxies (I have to admit I almost only know enhancing of a single entity)?
        Sorry, I misspoke before. When I mentioned dynamic proxies, I should have said custom proxies[1].

        > I can surely help to use asm but I need some pointers, maybe some tests to pass to work on it.
        A bulk of the serp functionality that needs to be replaced is contained in PCEnhancer. I suspect you can start by running any unit test that interacts with an Entity and use the -javaagent enhancer.

        [1] http://ci.apache.org/projects/openjpa/trunk/docbook/manual.html#ref_guide_pc_scos_proxy_custom

        Show
        curtisr7 Rick Curtis added a comment - > No tomee just need to work with a java 8 runtime. I believe with java 8 that we can do that today.... mostly. > What I dont get is why needing dynamic proxies (I have to admit I almost only know enhancing of a single entity)? Sorry, I misspoke before. When I mentioned dynamic proxies, I should have said custom proxies [1] . > I can surely help to use asm but I need some pointers, maybe some tests to pass to work on it. A bulk of the serp functionality that needs to be replaced is contained in PCEnhancer. I suspect you can start by running any unit test that interacts with an Entity and use the -javaagent enhancer. [1] http://ci.apache.org/projects/openjpa/trunk/docbook/manual.html#ref_guide_pc_scos_proxy_custom
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Looked quickly over the code and BCClass is too important to let rewriting the enhancement quickly with ASM and without going deep in the code.

        Is it possible to move enhancement to a really smaller part of the code? Basically I'd only expect few methods doing bytecode manipulation. Input being a class or class name (to find .class) and output the byte[].

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Looked quickly over the code and BCClass is too important to let rewriting the enhancement quickly with ASM and without going deep in the code. Is it possible to move enhancement to a really smaller part of the code? Basically I'd only expect few methods doing bytecode manipulation. Input being a class or class name (to find .class) and output the byte[].
        Hide
        curtisr7 Rick Curtis added a comment -

        As I've noted before, removing references to serp is not for the faint of heart and I don't believe that we can easily refactor enhancement into a smaller part of the code. When/if someone takes on this task it is going to be pretty painful.

        Show
        curtisr7 Rick Curtis added a comment - As I've noted before, removing references to serp is not for the faint of heart and I don't believe that we can easily refactor enhancement into a smaller part of the code. When/if someone takes on this task it is going to be pretty painful.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Hmm

        did some basic tests (just a single entity, no relationships so no collection proxy) and asm5 upgrade make it working fine in java 8 (static enhancement or not)

        So the issue is mainly on relationship AFAIK, will try to test when I get a bit more time

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Hmm did some basic tests (just a single entity, no relationships so no collection proxy) and asm5 upgrade make it working fine in java 8 (static enhancement or not) So the issue is mainly on relationship AFAIK, will try to test when I get a bit more time
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1586647 from kwsutter@apache.org in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1586647 ]

        OPENJPA-2487. This patch provided by Romain Manni-Bucau looks good. As stated in the Comments, this patch by itself is not sufficient for Java 8 runtime support, but it is a crucial step. Follow the linked JIRAs for additional fixes required to fully support Java 8.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1586647 from kwsutter@apache.org in branch 'openjpa/trunk' [ https://svn.apache.org/r1586647 ] OPENJPA-2487 . This patch provided by Romain Manni-Bucau looks good. As stated in the Comments, this patch by itself is not sufficient for Java 8 runtime support, but it is a crucial step. Follow the linked JIRAs for additional fixes required to fully support Java 8.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1641438 from Jody Grassel in branch 'openjpa/branches/2.2.x'
        [ https://svn.apache.org/r1641438 ]

        OPENJPA-2487: upgrade openjpa to asm5 to support java 8 [JDK 8]

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1641438 from Jody Grassel in branch 'openjpa/branches/2.2.x' [ https://svn.apache.org/r1641438 ] OPENJPA-2487 : upgrade openjpa to asm5 to support java 8 [JDK 8]

          People

          • Assignee:
            kwsutter Kevin Sutter
            Reporter:
            romain.manni-bucau Romain Manni-Bucau
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development