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

Jump to bottom

Absence of Pack200 in JDK 14 should not cause failure of JaCoCo build #984

Merged
merged 8 commits into from Dec 18, 2019
Merged

Absence of Pack200 in JDK 14 should not cause failure of JaCoCo build #984

merged 8 commits into from Dec 18, 2019

Conversation

Godin
Copy link
Member

@Godin Godin commented Dec 13, 2019 •
edited

Pack200 was deprecated in JDK 13 (https://openjdk.java.net/jeps/336)
with planned removal in JDK 14 (https://openjdk.java.net/jeps/367)
which happened recently (http://hg.openjdk.java.net/jdk/jdk/rev/f236fd5d0c2c)
and supposed to be part of upcoming JDK 14 Early Access Build 27.

This doesn't cause troubles with released JaCoCo 0.8.5 for cases without processing of Pack200, and in the last case it fails as following

$ java -jar jacoco-0.8.5/lib/jacococli.jar classinfo org.eclipse.eclemma.core_3.1.2.201903112331.jar.pack.gz
  INST   BRAN   LINE   METH   CXTY   ELEMENT
Exception in thread "main" java.lang.NoClassDefFoundError: java/util/jar/Pack200
        at org.jacoco.cli.internal.core.internal.Pack200Streams.unpack(Pack200Streams.java:43)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzePack200(Analyzer.java:294)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:200)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeGzip(Analyzer.java:287)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:198)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:226)
        at org.jacoco.cli.internal.commands.ClassInfo.execute(ClassInfo.java:59)
        at org.jacoco.cli.internal.Main.execute(Main.java:90)
        at org.jacoco.cli.internal.Main.main(Main.java:105)
Caused by: java.lang.ClassNotFoundException: java.util.jar.Pack200
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:602)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
        ... 9 more

However this causes compilation failure during JaCoCo build with JDK 14:

[INFO] --- maven-compiler-plugin:3.7.0:compile (default-compile) @ org.jacoco.core ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 121 source files to /Users/evgeny.mandrikov/projects/jacoco/org.jacoco.core/target/classes
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /Users/evgeny.mandrikov/projects/jacoco/org.jacoco.core/src/org/jacoco/core/internal/Pack200Streams.java:[23,21] cannot find symbol
  symbol:   class Pack200
  location: package java.util.jar
[ERROR] /Users/evgeny.mandrikov/projects/jacoco/org.jacoco.core/src/org/jacoco/core/internal/Pack200Streams.java:[43,17] cannot find symbol
  symbol:   variable Pack200
  location: class org.jacoco.core.internal.Pack200Streams
[ERROR] /Users/evgeny.mandrikov/projects/jacoco/org.jacoco.core/src/org/jacoco/core/internal/Pack200Streams.java:[62,17] cannot find symbol
  symbol:   variable Pack200
  location: class org.jacoco.core.internal.Pack200Streams

This PR prevents compilation failure and makes support of Pack200 optional by using reflection for JDKs below 14.

Also note that after this change error message will contain location:

$ java -jar jacoco-0.8.6.201912130302/lib/jacococli.jar classinfo org.eclipse.eclemma.core_3.1.2.201903112331.jar.pack.gz
  INST   BRAN   LINE   METH   CXTY   ELEMENT
Exception in thread "main" java.io.IOException: Error while analyzing org.eclipse.eclemma.core_3.1.2.201903112331.jar.pack.gz.
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzerError(Analyzer.java:163)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzePack200(Analyzer.java:296)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:200)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeGzip(Analyzer.java:287)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:198)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzeAll(Analyzer.java:226)
        at org.jacoco.cli.internal.commands.ClassInfo.execute(ClassInfo.java:59)
        at org.jacoco.cli.internal.Main.execute(Main.java:90)
        at org.jacoco.cli.internal.Main.main(Main.java:105)
Caused by: java.io.IOException
        at org.jacoco.cli.internal.core.internal.Pack200Streams.newIOException(Pack200Streams.java:95)
        at org.jacoco.cli.internal.core.internal.Pack200Streams.unpack(Pack200Streams.java:51)
        at org.jacoco.cli.internal.core.analysis.Analyzer.analyzePack200(Analyzer.java:294)
        ... 7 more
Caused by: java.lang.ClassNotFoundException: java.util.jar.Pack200
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:602)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
        at java.base/java.lang.Class.forName0(Native Method)
        at java.base/java.lang.Class.forName(Class.java:341)
        at org.jacoco.cli.internal.core.internal.Pack200Streams.unpack(Pack200Streams.java:44)
        ... 8 more

@Godin
(WIP) for tests use our Pack200Streams instead of java.util.jar.Pack200
5f625ef 
To avoid compilation errors with JDK 14, where Pack200 not available.
@Godin
(WIP) add unit tests to prevent regressions
6272d30 
Instrumenter and Analyzer provide location information
when Pack200 streams throw IOException on error.
@Godin
(WIP) use reflection in Pack200StreamsTest
a6dc17a 
To avoid compilation errors with JDK 14, where Pack200 not available.
@Godin
(WIP) add unit tests for the case when Pack200 not available in JDK
d8316f4
@Godin
(WIP) use reflection in Pack200Streams
18e4263 
To avoid compilation errors with JDK 14, where Pack200 not available.
@Godin
(WIP) tests that require Pack200 should be disabled on JDKs without it
0b373ff
@Godin Godin added this to the 0.8.6 milestone Dec 13, 2019
@Godin Godin requested a review from marchof December 13, 2019 15:08
@Godin Godin self-assigned this Dec 13, 2019
@Godin Godin added this to Implementation in Current work items via automation Dec 13, 2019
marchof
marchof reviewed Dec 13, 2019
View reviewed changes
Copy link
Member

@marchof marchof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @Godin! We now have some warnings that should be fixed or suppressed. Also I wonder whether there should be a change log entry making clear that Pack200 will no longer be supported by JaCoCo when executed on the latest JREs.

org.jacoco.core/src/org/jacoco/core/internal/Pack200Streams.java Show resolved Hide resolved
org.jacoco.core/src/org/jacoco/core/internal/Pack200Streams.java Show resolved Hide resolved
@Godin
Copy link
Member Author

Godin commented Dec 13, 2019

@marchof

Also I wonder whether there should be a change log entry

I was wondering the same - on one side we're making change, on the other side real change is in JDK and Pack200 doesn't work with and without our change - see behaviours in description of this PR.

Maybe something like

Support for Pack200 was removed in JDK 14, so instead of ClassNotFoundException JaCoCo now throws IOException with location, when JDK does not support Pack200.

as non-functional change?

Also now after preparation of this change, I see that there are actually more than 2 options that we discussed:

  1. this PR - analysis/instrumentation of Pack200 will work on JDKs <14, will throw IOException on JDKs >=14
  2. remove support by throwing IOException during analysis/instrumentation of Pack200
  3. remove support by ignoring Pack200 during instrumentation/analysis

In all these cases we have change of behavior, with conditional in 1st case.

2nd case unconditional, explicit and behaves the same way as 1st on JDKs >=14. And unfortunately we don't provide way to specify exclusions for contents of archives, so both 1st and 2nd cases might be problematic in case of nested archives.

3rd case is unconditional, but for consumers might be hard to spot reason of change in behavior.

So I'm wondering whether you had in mind 2nd or 3rd by "remove Pack200 support"? 😉

@marchof
Copy link
Member

marchof commented Dec 14, 2019

@Godin

As many users will not upgrade to Java 14 quickly I wouldn't kill that feature right now and stick with your proposed PR.

For the change log I would rephrase it a bit (as non-functional change):

Support for Pack200 was removed in JDK 14. JaCoCo will now throw a detailed exception when Pack200 archives are processed with the latest JDKs.

@Godin Godin moved this from Implementation to Review in Current work items Dec 17, 2019
@Godin
Copy link
Member Author

Godin commented Dec 17, 2019

@marchof once merged I'll continue updates of version of JDK 14 Early Access builds that is used for our builds in Travis CI.

@marchof marchof merged commit 60cf0dd into master Dec 18, 2019
Current work items automation moved this from Review to Done Dec 18, 2019
@marchof marchof deleted the pack200 branch December 18, 2019 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marchof marchof marchof approved these changes

Assignees

@Godin Godin

Labels
component: core type: enhancement
Projects
Current work items
  
Done
Milestone
0.8.6
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Godin @marchof