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

Upgrade to ASM 6 #600

Merged
merged 1 commit into from Oct 19, 2017

Conversation

Projects
3 participants
@andrioli
Contributor

andrioli commented Sep 27, 2017

Upgrade to new ASM release 6.0. This version supports Java 9 class files, so I also removed Java9Support class.

This ASM version comes with a module-info.class file (major 53) but the shade plugin don't like it. So I also updated the maven-shade-plugin.

The shade plugin will output some warnings:

[WARNING] Discovered module-info.class. Shading will break its strong encapsulation.

I tried to apply a filter but the filter have no effect removing the warn message. Looking in the bundled JAR the module-info.class looks to not be included. So I think is ok to ignored the warn.

I built with success in my machine with Maven 3.5.0 and JDK 1.7.0_80, 1.8.0_144 and 9+181. Also tried with Maven 3.2.5 and JDK 1.6.0_65

@Godin Godin self-requested a review Sep 27, 2017

@Godin Godin self-assigned this Sep 27, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 27, 2017

Member

@andrioli Thanks for this nice and clean contribution!

@Godin Beside change log entry this PR looks good to me. Local IDE and builds are ok.

Member

marchof commented Sep 27, 2017

@andrioli Thanks for this nice and clean contribution!

@Godin Beside change log entry this PR looks good to me. Local IDE and builds are ok.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 27, 2017

Member

@andrioli Also nice that you managed to open PR #600 for ASM 6. 😄

Member

marchof commented Sep 27, 2017

@andrioli Also nice that you managed to open PR #600 for ASM 6. 😄

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 27, 2017

Member

@andrioli thank you for your contribution 👍 and congratulations with round number 600 😉

Upgrade to new ASM release 6.0.

This is definitely good thing since should bring fixes for bugs reported by us.

@marchof I believe that we would need to add tests for them to prevent regressions since they are not tested in ASM explicitly. Also maybe will be better to do this update after finishing work on "filtering" - to ease its integration into Eclipse EclEmma, recall that it consumes ASM from Eclipse Orbit.

The shade plugin will output some warnings:

[WARNING] Discovered module-info.class. Shading will break its strong encapsulation.

I tried to apply a filter but the filter have no effect removing the warn message. Looking in the bundled JAR the module-info.class looks to not be included. So I think is ok to ignored the warn.

This is unavoidable warning, meaning that initially non-exported packages became visible at least for classes in final JAR - see mention of this in announcement of release of maven-shade-plugin version 3.1.0 (http://markmail.org/message/s6ogzz6h2j72xno6).

Member

Godin commented Sep 27, 2017

@andrioli thank you for your contribution 👍 and congratulations with round number 600 😉

Upgrade to new ASM release 6.0.

This is definitely good thing since should bring fixes for bugs reported by us.

@marchof I believe that we would need to add tests for them to prevent regressions since they are not tested in ASM explicitly. Also maybe will be better to do this update after finishing work on "filtering" - to ease its integration into Eclipse EclEmma, recall that it consumes ASM from Eclipse Orbit.

The shade plugin will output some warnings:

[WARNING] Discovered module-info.class. Shading will break its strong encapsulation.

I tried to apply a filter but the filter have no effect removing the warn message. Looking in the bundled JAR the module-info.class looks to not be included. So I think is ok to ignored the warn.

This is unavoidable warning, meaning that initially non-exported packages became visible at least for classes in final JAR - see mention of this in announcement of release of maven-shade-plugin version 3.1.0 (http://markmail.org/message/s6ogzz6h2j72xno6).

@Godin Godin added this to the ASM milestone Sep 27, 2017

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Sep 27, 2017

Member

@Godin As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

But this is not something we can request from @andrioli .

Member

marchof commented Sep 27, 2017

@Godin As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

But this is not something we can request from @andrioli .

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 27, 2017

Member

As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

@marchof Don't know specifically about PDE, but AFAIK for Oxygen.1a, whose focus is Java 9, there were no interest in ASM 6 BETA (see https://dev.eclipse.org/mhonarc/lists/orbit-dev/msg04882.html) and AFAIK final version was released too late for inclusion into Oxygen.1a.

But that's doesn't matter: by saying "ease integration", I meant exactly "easier", otherwise will just need to work on its addition as did for the two previous versions. There are quite some mess in sources of ASM published to Maven Central as usual, but maybe I'll find time to deal with this and raise CQ for addition.

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

😆

But this is not something we can request from @andrioli .

This wasn't a request, just a remark/reminder for us.

Member

Godin commented Sep 27, 2017

As Eclipse platform is working towards Java 9 I hope ASM 6 will show up in orbit soon (AFAIK ASM is required for PDE).

@marchof Don't know specifically about PDE, but AFAIK for Oxygen.1a, whose focus is Java 9, there were no interest in ASM 6 BETA (see https://dev.eclipse.org/mhonarc/lists/orbit-dev/msg04882.html) and AFAIK final version was released too late for inclusion into Oxygen.1a.

But that's doesn't matter: by saying "ease integration", I meant exactly "easier", otherwise will just need to work on its addition as did for the two previous versions. There are quite some mess in sources of ASM published to Maven Central as usual, but maybe I'll find time to deal with this and raise CQ for addition.

You're right about regression tests. Looks like the primary purpose of JaCoCo is to serve as a regression test suite for ASM and the JDK. 😉

😆

But this is not something we can request from @andrioli .

This wasn't a request, just a remark/reminder for us.

@andrioli

This comment has been minimized.

Show comment
Hide comment
@andrioli

andrioli Sep 28, 2017

Contributor

Done. @Godin

Now is even easier to understand the changes. Unfortunately Travis is failing. Looks that the JDK download URL is not valid. Maybe you can fix the environment variables and re-run the jobs to keep everything green 😄

Contributor

andrioli commented Sep 28, 2017

Done. @Godin

Now is even easier to understand the changes. Unfortunately Travis is failing. Looks that the JDK download URL is not valid. Maybe you can fix the environment variables and re-run the jobs to keep everything green 😄

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Sep 28, 2017

Member

@andrioli Thanks! We'll merge this later.

@marchof I raised CQ for ASM 6 and it is already approved, so I'll raise PR for addition to Eclipse Orbit soon. However migration from Hudson to Jenkins for Orbit has been requested today, so might be delays in merges of PRs and builds.

Member

Godin commented Sep 28, 2017

@andrioli Thanks! We'll merge this later.

@marchof I raised CQ for ASM 6 and it is already approved, so I'll raise PR for addition to Eclipse Orbit soon. However migration from Hudson to Jenkins for Orbit has been requested today, so might be delays in merges of PRs and builds.

@Godin Godin added this to TODO in Update ASM to 6.0 Oct 6, 2017

@@ -59,8 +58,8 @@ public void teardown() {
@Test
public void should_not_loose_InnerClasses_attribute() throws Exception {

This comment has been minimized.

@Godin

Godin Oct 13, 2017

Member

After upgrade this test fails on JDK 5 due to another ASM bug - https://gitlab.ow2.org/asm/asm/issues/317800 😆

@Godin

Godin Oct 13, 2017

Member

After upgrade this test fails on JDK 5 due to another ASM bug - https://gitlab.ow2.org/asm/asm/issues/317800 😆

This comment has been minimized.

@Godin

Godin Oct 13, 2017

Member

However ASM 5.2 already had an issues in a similar scenario - see #487

@Godin

Godin Oct 13, 2017

Member

However ASM 5.2 already had an issues in a similar scenario - see #487

This comment has been minimized.

@Godin

Godin Oct 18, 2017

Member

Given that scenarios are limited to class files without frames and with big methods that require wide jumps, and that there was problem #487 in the exact same scenarios, I do not consider this as regression.

In these scenarios after update of ASM, we'll be producing class files with frames that are not supposed to be present. While this can cause problems of reading of produced classes by the ASM as described in https://gitlab.ow2.org/asm/asm/issues/317800 , seems that JVM can read such classes and I believe that doesn't perform validation of frames, so this should not cause execution problems.

And taking into account that this update will resolve #487, #544, #584 and #606, I propose to update test to make it pass, and merge update into master.

Also seems that we can implement removal of these unwanted frames after instrumentation if there were no frames in original class - see my comment in ASM issue (https://gitlab.ow2.org/asm/asm/issues/317800#note_9609). However I propose to defer this and merge update without this - can implement later and maybe soon there will be ASM 6.0.1 with fix (https://gitlab.ow2.org/asm/asm/merge_requests/46).

@marchof WDYT?

@Godin

Godin Oct 18, 2017

Member

Given that scenarios are limited to class files without frames and with big methods that require wide jumps, and that there was problem #487 in the exact same scenarios, I do not consider this as regression.

In these scenarios after update of ASM, we'll be producing class files with frames that are not supposed to be present. While this can cause problems of reading of produced classes by the ASM as described in https://gitlab.ow2.org/asm/asm/issues/317800 , seems that JVM can read such classes and I believe that doesn't perform validation of frames, so this should not cause execution problems.

And taking into account that this update will resolve #487, #544, #584 and #606, I propose to update test to make it pass, and merge update into master.

Also seems that we can implement removal of these unwanted frames after instrumentation if there were no frames in original class - see my comment in ASM issue (https://gitlab.ow2.org/asm/asm/issues/317800#note_9609). However I propose to defer this and merge update without this - can implement later and maybe soon there will be ASM 6.0.1 with fix (https://gitlab.ow2.org/asm/asm/merge_requests/46).

@marchof WDYT?

This comment has been minimized.

@marchof

marchof Oct 19, 2017

Member

@Godin Let's move forward to 6.0 without the workaround -- a you propose. I assume that 6.0.1 will be there before we get the first bug report for old class files.

@marchof

marchof Oct 19, 2017

Member

@Godin Let's move forward to 6.0 without the workaround -- a you propose. I assume that 6.0.1 will be there before we get the first bug report for old class files.

@Godin Godin modified the milestones: ASM, 0.8.0 Oct 19, 2017

@Godin

Godin approved these changes Oct 19, 2017

@Godin Godin merged commit caa820e into jacoco:master Oct 19, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Godin Godin moved this from TODO to DONE in Update ASM to 6.0 Oct 19, 2017

@Godin Godin referenced this pull request Dec 9, 2017

Merged

Support JDK 10 #629

@jacoco jacoco locked as resolved and limited conversation to collaborators Jan 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.