-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Java 11 and AspectJ LTW #909
Comments
Seems to be resolved by using 0.8.3 |
Similarly to #896 (comment) :
the only change in JaCoCo version Looking only at the error message in stacktrace
one can only say that In any case please report this to developers of AspectJ weaver agent. |
Thank you for your quick response. Bug raised with AspectJ: https://bugs.eclipse.org/bugs/show_bug.cgi?id=549438 |
FYI, if anyone else has the same problem in the future: This is neither a JaCoCo nor an AspectJ problem, but simply caused by the fact that aspects were applied too broadly during load-time weaving, AspectJ also weaving aspects into Surefire, JUnit and JaCoCo itself. Proper AspectJ configuration avoids this, as explained in eclipse-aspectj/aspectj#68 (comment) and proven in https://github.com/kriegaex/AJ_LTWJacocoWeavingProblem_549438. That it worked before in JDK 8 was pure luck. |
@Godin @kriegaex tried your options but still doesn't work with Java 17. My aspects applied only within my app packages and i can confirm that from the .dumpstream files. Works fine though if I use https://github.com/bazelbuild/bazel/tree/master/third_party/java/jacoco version instead. |
@ibragfir, please do not hijack this closed issue. Firstly, it was created in July 2019 when the most recent Java release was 12. The LTS was 11, and this question also is about Java 11. Like I have proven in my answer, the issue was simply wrong AspectJ load-time weaving configuration and neither a JaCoCo problem nor an AspectJ bug. I am not actually a JaCoCo user and only was involved here because AspectJ was in the game. But if you read the JaCoCo 0.8.7 release notes, you will see that Java 17 support is marked as "experimental" there. That using another JaCoCo build with a special patch works for you, indicates that there is an actual JaCoCo problem with Java 17. This should be dealt with in a separate issue, not in this one, because the two topics are unrelated. I suggest you create one (please first check if one exists already), add an MCVE and feel free to link back to your comment here where the story started accidentally. Thank you. Besides, if you want to use the AspectJ compiler with Java 17, 1.9.8.RC3 is the correct version. 1.9.7 knows nothing about Java 17. For LTW that might still work, if there are not bytecode constructs unknown to the aspect weaver. But to be on the safe side, stick to 1.9.8.RC3. |
@kriegaex thanks. I don't see a problem with aspects being applied too broadly in this case. Does it explain the underlying issue? |
It explains the issue as described here. You are, according to your own words, using my solution for the issue, experiencing a completely different problem solved by upgrading JaCoCo. Therefore, you are off topic here and should open a new issue for your problem, like I said. If you disagree, publish an MCVE proving your point. |
I'm experiencing exactly the same issue with Java 11 and Java 17 which can be solved by using that patched version of JaCoCo. The author here raised this problem which is exactly the one I'm trying to find a solution for.
And I don't think it was pure luck that it was working with Java 8 but the fact that the author upgraded from 0.8.3 to 0.8.4 along with Java 11. He wouldn't be able to use 0.8.4 with Java 8 since JaCoCo 0.8.4 is using Java 11+ feature (https://openjdk.java.net/jeps/309). |
Actually he could and worked because there's a condition not to use dynamic constants if the version is not >=11 |
Why did you not say that right from the beginning? You only talked about Java 17 and different JaCoCo and AspectJ versions than the ones in this issue.
That is a lame excuse, unworthy of a developer. I said MCVE, not "original code". Simply fork my repository and adjust it until it reproduces the problem. Basically, your claim is "it does not work the way @kriegaex suggested" without even a scintilla of proof. I cannot debug what I cannot see. Can you?
That might be true, my solution is still to be applied - maybe in addition to whatever fix there might be necessary in JaCoCo - in order to avoid instrumenting the bytecode of tools like JaCoCo, JUnit and Surefire when running tests including aspects which apply too broadly. |
OK, I just did that for you, see my latest commits. I managed to reproduce the problem now, despite excluding foreign packages, by adding something using virtual methods (a It is actually #1151, which also the patch you mentioned relates to. See also JEP 309. I built the current JaCoCo snapshot locally without the patch, and it still reproduces the problem. With the patch, like you said, the problem goes away. Now we have something reproducible to look into for AspectJ. |
OK, the solution is actually very simple, see eclipse-aspectj/aspectj#68 (comment). Quote:
Bottom line: The order of Java agents simply was wrong. JaCoCo has to be applied after AspectJ. I had not thought of that before, but should have suggested it in the first place, in addition to limiting the aspect scope, which is still necessary. Incorrect Surefire configuration: <argLine>
--add-opens java.base/java.lang=ALL-UNNAMED
@{argLine}
-javaagent:${user.home}/.m2/repository/org/aspectj/aspectjweaver/${aspectj.version}/aspectjweaver-${aspectj.version}.jar
</argLine> Correct Surefire configuration: <argLine>
--add-opens java.base/java.lang=ALL-UNNAMED
-javaagent:${user.home}/.m2/repository/org/aspectj/aspectjweaver/${aspectj.version}/aspectjweaver-${aspectj.version}.jar
@{argLine}
</argLine> |
OK, reversing the order of instrumentation agents is suboptimal, because it might lead to incorrect coverage data, applying JaCoCo instrumentation to byte code no longer corresponding to the source code it was compiled from, now also containing aspect code. Today I found a better solution when re-investigating a related problem, see eclipse-aspectj/aspectj#170 (comment): use the If a JaCoCo maintainer like @Godin is listening here: The fact that JaCoCo uses the exact same dynamic constant name and initialisation method name in each instrumented class is what causes problem with byte code engineering tools inlining code from class A into class B due to name clashes. Unique names like |
After updating to JDK 11 the Maven Surefire plugin fails to run when both Jacoco agent and AspectJ weaver agent present.
Steps to reproduce
JaCoCo version: 0.8.4
Operating system: Windows 10
JDK: open-jdk-11.0.1
Tool integration: Maven
aspectjweaver: 1.9.4
Expected behaviour
Tests to run as they did in JDK 8.
Actual behaviour
The text was updated successfully, but these errors were encountered: