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.

Sign up for GitHub

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

Support the Maven Toolchains Plugin #63

Open
exabrial opened this issue Jun 12, 2018 · 23 comments
Open

Support the Maven Toolchains Plugin #63

exabrial opened this issue Jun 12, 2018 · 23 comments

Comments

@exabrial
Copy link

Spotbugs has a dependency on JDK 1.8. It's also not good to use JDK 1.8 as your primary JDK as it's showing it's age.

Fortunately, Maven has a plugin called the toolchains plugin which allows a different JDK to be used to run a plugin than the JDK that maven is running in. The user could be running maven in JDK 10, but then SpotBugs could be ran using a JDK 1.8 installation.

https://cwiki.apache.org//confluence/display/MAVENOLD/Toolchains
@hazendaz
Copy link
Member

Spotbugs works on 9, 10, and 11. I only use 10 now for regular builds. Toolchains is overratted in my opinion and rarely if ever actually needed. I'm ok with the addition though and see the PR got the commits cleaned up after the first push. If this is ready and not a WIP as initially noted, can you rebase leaving the version alone and I'll include it in 3.1.4 release?

@exabrial
Copy link
Author

@hazendaz Thanks a bunch! I cleaned up the PR and rebased, should be ready for your review

@jansohn
Copy link

jansohn commented Apr 10, 2019

@hazendaz I don't think toolchains is overrated but maybe I'm missing something. We have the following setup:

  • Maven 3.5.0
  • JAVA_HOME points to 11.0.2
  • Project needs to be JDK8 compliant (meaning it will run with JDK/JRE 8)

The project uses classes like javax.xml.bind.annotation.XmlRootElement which were removed from JDK11 onward. Now if we don't use the toolchain-plugin we get compilation errors like

error: package javax.xml.bind.annotation does not exist
error: cannot find symbol
  symbol: class XmlRootElement

If we use the toolchains-plugin to make sure the project gets compiled and built with JDK8 everything works fine until the spotbugs-plugin runs with the default JDK (because it does not support the toolchain):

[INFO] configuring report plugin com.github.spotbugs:spotbugs-maven-plugin:3.1.11
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.vmplugin.v7.Java7$1 (file:/C:/Users/z200876/.m2/repository/org/codehaus/groovy/groovy/2.5.5/groovy-2.5.5-indy.jar) to constructor java.lang.invoke.MethodHandles$Lookup(java.lang.Class,int)
WARNING: Please consider reporting this to the maintainers of org.codehaus.groovy.vmplugin.v7.Java7$1
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[INFO] Fork Value is true
     [java] The following classes needed for analysis were missing:
     [java]   javax.xml.bind.JAXBContext
     [java]   javax.xml.bind.Unmarshaller
     [java]   javax.xml.bind.Marshaller
     [java]   javax.xml.bind.JAXBException
     [java] Missing classes: 4
[INFO] Done SpotBugs Analysis....

As we can see it is also missing some JAXB classes which were removed from JDK11.

Personally, I think toolchain support would avoid these problems but maybe you can tell me a different way to fix the analysis?

@hazendaz
Copy link
Member

hazendaz commented Apr 10, 2019 via email

@jansohn
Copy link

jansohn commented Apr 10, 2019

No, it's not as simple as that. The project isn't ready for JDK 11 (e.g. eclipselink eclipse-ee4j/eclipselink#269) so our unit tests will fail if they are run with JDK 11. Also I want to run my unit tests with the same JDK version which I use in production.

The only ways to achieve to build and test our application are:

  • toolchain definition
  • .mavenrc file setting JAVA_HOME to appropriate JDK

The toolchain option is way more flexible as it can be configured per project and does not affect the complete build environment. With .mavenrc option I can only build projects up to the specified JAVA_HOME JDK version.

I agree that toolchains is not necessary in every project but that doesn't mean it shouldn't be supported!

@hazendaz
Copy link
Member

hazendaz commented Apr 10, 2019 via email

@jansohn
Copy link

jansohn commented Apr 10, 2019

There has already been an effort to achieve this:
#64

@mathieucarbou
Copy link

We also need this support.

At Terracotta we are using the toolchain plugin to ensure projects are built with a supported JDK, and tested against different JVMs. This is a good way to control the environment, ensure the right JDK / vendor are used, since a build platform or dev computer could have different JVMs installed and used by default. That's also not true that a project compiling with 8 for example will compile without any issue in 9 and 11 and so on.

There is a difference between the jvm used to run maven, the jdk used to compile and jvm used to run the tests. In the case of spotbugs, it would need to use the same jdk as the one used by the compiler plugin to give meaningful results and not crash with some "unsupported class" errors.

For reference, a colleague (@akomakom) did a PR in the surefire/failsafe plugin a while ago to add toolchain support to these plugins: https://github.com/apache/maven-surefire/pull/285/files

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022 via email

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022 via email

@mathieucarbou
Copy link

mathieucarbou commented Apr 2, 2022 •
edited
Loading

Having 100s of projects at my day job using jdk 17 to build and jdk 8 to run, toolchains isn't at all used

That's the point.

The issue is happening when using toolchain because the spotbugs plugin does not obey the toolchain constrains.

Example:

> git clone [email protected]:Terracotta-OSS/angela.git
> cd angela
> ./mvnw clean install -DskipTests

You will see plenty of:

     [java] Exception in thread "main" java.lang.IllegalArgumentException: Unsupported class file major version 61
     [java]     at org.objectweb.asm.ClassReader.<init>(ClassReader.java:196)
     [java]     at org.objectweb.asm.ClassReader.<init>(ClassReader.java:177)
     [java]     at org.objectweb.asm.ClassReader.<init>(ClassReader.java:163)
     [java]     at edu.umd.cs.findbugs.asm.FBClassReader.<init>(FBClassReader.java:35)
     [java]     at edu.umd.cs.findbugs.classfile.engine.asm.ClassReaderAnalysisEngine.analyze(ClassReaderAnalysisEngine.java:48)
     [java]     at edu.umd.cs.findbugs.classfile.engine.asm.ClassReaderAnalysisEngine.analyze(ClassReaderAnalysisEngine.java:34)
     [java]     at edu.umd.cs.findbugs.classfile.impl.AnalysisCache.getClassAnalysis(AnalysisCache.java:261)
     [java]     at edu.umd.cs.findbugs.classfile.engine.ClassInfoAnalysisEngine.analyze(ClassInfoAnalysisEngine.java:75)
     [java]     at edu.umd.cs.findbugs.classfile.engine.ClassInfoAnalysisEngine.analyze(ClassInfoAnalysisEngine.java:38)
     [java]     at edu.umd.cs.findbugs.classfile.impl.AnalysisCache.getClassAnalysis(AnalysisCache.java:261)
     [java]     at edu.umd.cs.findbugs.ba.XFactory.getXClass(XFactory.java:685)
     [java]     at edu.umd.cs.findbugs.ba.AnalysisContext.setAppClassList(AnalysisContext.java:975)
     [java]     at edu.umd.cs.findbugs.FindBugs2.setAppClassList(FindBugs2.java:909)
     [java]     at edu.umd.cs.findbugs.FindBugs2.execute(FindBugs2.java:252)
     [java]     at edu.umd.cs.findbugs.FindBugs.runMain(FindBugs.java:395)
     [java]     at edu.umd.cs.findbugs.FindBugs2.main(FindBugs2.java:1231)

This happens when:

❯  ./mvnw -v
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /Users/matc/.m2/wrapper/dists/apache-maven-3.6.3-bin/lm9vem38rfmjij3jj0mk5bvnt/apache-maven-3.6.3
Java version: 17.0.2, vendor: Azul Systems, Inc., runtime: /Users/matc/.sdkman/candidates/java/17.0.2-zulu/zulu-17.jdk/Contents/Home
Default locale: en_FR, platform encoding: UTF-8
OS name: "mac os x", version: "12.3", arch: "x86_64", family: "mac"

Using the toolchain plugin to enforce compilation and test with JDK 1.8, but spotbugs seems to analyze some classes coming from the wrong jvm (17) instead of looking into toolchain for the right jvm that was used to compile the classes.

Attached a normal maven run with a run with -X and -e.

normal.txt
debug.txt

There is a STRONG difference to make between the JVM used to run an application / tool like maven and the JDK used to compile the classes (and even eventually the jvm used to run the tests), which may not be the same.

Basically:

  • have a jvm for your system (i.e. 17) - used to run tooling
  • toolchain is used because we require projects to be compiled in 1.8
  • toolchain is also used because once compiled, we also have toolchain enforcement to run tests with a different jvm (i.e. 1.8, 11, etc)

That's the correct way when one need a project to be compiled with an older jdk for compatibility reasons but tested on a different jvm to ensure the project will work and is supported on the tested versions, when a 1.8 compiled jar is provided.

It is not possible to use the same jdk to run maven, compile and test, except if you deploy jars per jdk version specifying that each jar is then only tested with the jvm used to compile. So foo.jar compiled in 17 would be tested with jvm 17 and then only has a guarantee to work with jvm 17 only.

Only is a string word but when working on low level projects there are a couple of changes from jvm to jvm that it might impact how the project works or behaves and its perfs.

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022 •
edited
Loading

@mathieucarbou Feel free to raise a PR. Some work was done in 2018 but never panned out.

However those are IMHO old considerations.

With jdk 9+, what animal sniffer used to do is built in. Its controlled by the release flags. Further using extra enforcer rules to prevent third party binary incompatibility which removed any need to use a separate jdk via toolchains. It won't cause any differences to your runtimes. You might get some tests differences with timestamps and other released items but nothing of any significance. In fact, the api added won't even be used with jdk 8 or before. Java requires endorced or extension to do so otherwise the classpath elements of the api are skipped before 9. Further setting them provided gives that extra comfort of it doesn't matter. As to surefire usage, you can split the entire build to use separate revision levels for tests to based on level you want to remove the minor differences if any. However, if your need is really insistent on toolchains, then feel free to add the support. I'm not against it just noting it is not required and has been effectively dispelled over many years. Some may still want to use it as you do and that is fine provided the support is provisioned to do so. If you want to see how this is achieved though on modern setup without toolchains, look at https://github.com/hazendaz/base-parent/ parent this project uses as it contains that support now and is in process of dropping jdk 8 entirely at this point (build with jdk 8 that is, will continue to target 8).

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022

suggestion on that project you pointed out.

        <source>${java.build.version}</source>
        <target>${java.build.version}</target>

On the compiler plugin is ignoring all other plugins that use the same values. Also, if using jdk 17, its missing setup for release tags. Again looking at the project I noted, see how maven is done in proeprties section instead. I don't recall if that matters to spotbugs plugin but it might. There are so many that leverage the properties from maven compiler that it needs to be setup in the properties section to ensure other plugins get the same configuration. It may be that it doesn't realize jdk 8 was wanted. I didn't go up to the parent level on that project but looked at how compiler was being used to spot that consideration.

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022

These are what control it more globally that other plugins further rely on.

        <maven.compiler.source>${java.version}</maven.compiler.source>
        <maven.compiler.target>${java.version}</maven.compiler.target>
        <maven.compiler.testSource>${java.test.version}</maven.compiler.testSource>
        <maven.compiler.testTarget>${java.test.version}</maven.compiler.testTarget>

note this is split source/test and doesn't need to be. The test ones will use the main ones.

If using jdk 11+ (really 9+), the following can come into play.

<maven.compiler.release>${java.release.version}</maven.compiler.release>
<maven.compiler.testRelease>${java.test.release.version}</maven.compiler.testRelease>

Same thing here, the test one is optional. You may want to play around with that and see if the issue behaves any different.

@mathieucarbou
Copy link

Also, if using jdk 17,

We don't want to use 17. There is a difference between:

A) the JVM used to run Maven (17)
B) the JVM used to compile the project (1.8, from toolchain speced by version and vendor)
C) the JVM used to test the project (1.8 or 11, from toolchain, speced by version and vendor)

B et C are controlled through properties (in our case java.build.version and java.yesy.version). We are building and testing our projects with several combinations of test JVM and OS. But we always compile them with 1.8. So the build machines all have a toolchain file pointing to different JVMs that will be used by maven to compile and test, regardless of the JVM used to run maven (which MUST NOT be the one used to compile or test).

In my example, 17 is just the JVM used to run maven, and eventually the default jvm used by the system to run other tools. It doesn't have to interfere with project building or testing.

But the spotbogs plugin seems to miss a "jvm" arg here that would allow the plugin specify the right jvm to use when ant is launching the JVM for spotbugs.

@hazendaz
Copy link
Member

hazendaz commented Apr 2, 2022

@mathieucarbou I don't see that option available in the groovy docs.

However, the issue is far easer than that based on testing of this. The version of spotbugs used in 'angela' doesn't support jdk 17. The spotbugs plugin is built groovy which is built on ASM and requires that it support jdk 17. The errors are with the plugin rather than anything with compilation / testing as noted. I didn't notice that at first glance here but your stack trace shows that. This support was not available back when 4.0.0 was released in 2020 that terracota-parent provides. I do not believe this is relavent at all to toolchains as a result. Groovy runs on ASM and as such needed 9.1 which came in groovy 3.0.8 with spotbugs maven plugin at 4.3.0. Currently it supports up to jdk 18.

That said, as you have documented here rather well, you want jdk 17 to run the maven build but not compilation / tests. Spotbugs isn't running compilation or tests. Spotbugs is static analysis so it only cares about the source. Its running on the jdk it was launched on which makes sense. I tested this now and with jdk 8 or 11, your build works fine (toolchained to 8). Only with the jump to 17 does it start to fail like this (would have been same on 15/16). So bump to 4.6.0.0 for the plugin and 4.6.0 for spotbugs and it again. It does get 'EI_EXPOSE_REP' issues in angela common module. See below. Can you try to apply same and see if those are something to be fixed in current code of angela? I'm not 100% sure when those popped up but seems any version past 4.3.0 and varies in count.

image

@mathieucarbou
Copy link

mathieucarbou commented Apr 3, 2022 •
edited
Loading

The version of spotbugs used in 'angela' doesn't support jdk 17. The spotbugs plugin is built groovy which is built on ASM and requires that it support jdk 17.

Yest, I know, and this is exactly why, in the plugin, when starting the spotbug jvm through the ant task programmatically, the ant java task has to use the right jvm that was used to compile the project.

https://ant.apache.org/manual/Tasks/java.html

The jvm attribute is used to specify which jvm has to be used to spawn the java class, in this case , spotbug, and it has to be taken from the toolchain if toolchain is used to control the jvm used to compile.

This is not a solution to upgrade the spotbug library... The issue I have shows specifically that spotbug is looking at version 17 classes a spart of it's analysis since ASM tries to load java 17 classes.

It should never do that, that's a mistake that could lead to wrong results and a non reproductible build that depends on the jvm version used to run maven

The project was compiled with jvm 1.8 : it has to be analysed with the same 1.8 classes that it was compiled with.

Why would you want spotbug to load as part of it's analysis the classes from the jvm that is used to run maven ? This JVM has nothing to do with the project!

Spotbugs is static analysis so it only cares about the source.

I agree

Its running on the jdk it was launched on which makes sense.

I disagree: if spotbug cares about the sources, to analyze them it has to use the jvm used to compile them to ensure a better compatibility and also isolate the build for it to be reproductible.

This is totally fine to run with a spotbug version that is not supporting 17. The jvm used on one computer is only used to run maven and does not have to interfere with the build. On their system, devs can have 8,11, 17, 18, etc as set by default.

What is important is to make the build isolated and reproductible by controlling the jvm used within maven plugins.

And in the case of spotbug, it has to analyze the compiled classes with the same jvm used to compile, otherwise there is a dependency of the build on the jvm version used to run maven, which Is totally unacceptable for a reproductible build.

The jvm version used on dev computer to run maven must not interfere with the build.

@hazendaz
Copy link
Member

hazendaz commented Apr 3, 2022 via email

@hazendaz
Copy link
Member

hazendaz commented Oct 11, 2022 via email

@mathieucarbou
Copy link

Can you provide an example of an actual issue? This runs on numerous jdks already. It doesn't have issues itself as noted Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg
…
________________________________ From: Jeremy Landis @.> Sent: Saturday, April 2, 2022 12:05:58 PM To: spotbugs/spotbugs-maven-plugin @.>; spotbugs/spotbugs-maven-plugin @.> Cc: Mention @.> Subject: Re: [spotbugs/spotbugs-maven-plugin] Support the Maven Toolchains Plugin (#63) Can you provide an example of an actual issue? doesn't run 8nto unsupported class issues. Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg
________________________________ From: Mathieu Carbou @.> Sent: Saturday, April 2, 2022 6:32:18 AM To: spotbugs/spotbugs-maven-plugin @.> Cc: Jeremy Landis @.>; Mention @.> Subject: Re: [spotbugs/spotbugs-maven-plugin] Support the Maven Toolchains Plugin (#63) We also need this support. At Terracotta we are using the toolchain plugin to ensure projects are built with a supported JDK, and tested against different JVMs. This is a good way to control the environment, ensure the right JDK / vendor are used, since a build platform or dev computer could have different JVMs installed and used by default. That's also not true that a project compiling with 8 for example will compile without any issue in 9 and 11 and so on. There is a difference between the jvm used to run maven, the jdk used to compile and jvm used to run the tests. In the case of spotbugs, it would need to use the same jdk as the one used by the compiler plugin to give meaningful results and not crash with some "unsupported class" errors. For reference, a colleague @.https://github.com/akomakom) did a PR in the surefire/failsafe plugin a while ago to add toolchain support to these plugins: https://github.com/apache/maven-surefire/pull/285/files — Reply to this email directly, view it on GitHub<#63 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAHODI2IADNNUI65MZQJKJLVDAO3FANCNFSM4FETI5BA. You are receiving this because you were mentioned.Message ID: @.>

Please read the whole thread ;-) It's there.

@hazendaz
Copy link
Member

Sorry @mathieucarbou not sure what is going on with my email. I sent that message not 4 days ago but back on 4/2 per my email history. Having messages show up on various tickets all 4 days ago so feels like some lost set of emails now showing up and seems to span all year. All seem to be from verizon so likely some bug on their end got fixed.

Anyway back on this highly unlikely this will be provided. I had saw on maven mailing list recently Maven team members stating how tool chains isn't necessary any more which was my basic points anyways. I'm not against support if someone with more knowledge on how that plugin works could provide it and understanding probably the major constraint here is using groovy ant which does not appear to provide api you noted ant does provide.

@mathieucarbou
Copy link

mathieucarbou commented Oct 15, 2022 •
edited
Loading

Sorry @mathieucarbou not sure what is going on with my email. I sent that message not 4 days ago but back on 4/2 per my email history. Having messages show up on various tickets all 4 days ago so feels like some lost set of emails now showing up and seems to span all year. All seem to be from verizon so likely some bug on their end got fixed.

Anyway back on this highly unlikely this will be provided. I had saw on maven mailing list recently Maven team members stating how tool chains isn't necessary any more which was my basic points anyways. I'm not against support if someone with more knowledge on how that plugin works could provide it and understanding probably the major constraint here is using groovy ant which does not appear to provide api you noted ant does provide.

Where I work, we are still supporting the branches of our products (and previous ones) that are more than 10 years old. Most of our projects need to be compiled with 1.8 (and not with a more recent one using compatibility flags), and can run on >= 1.8. We also need to run our project tests on 1.8, 11 and 17 to ensure everything works fine because we are doing low level stuff and dealing with off heap memory / misc.Unsafe.

That's the reason we are using toolchain, to be able to select the jdk to use per plugin. There is no other way to ensure a project can be compiled with version X and tests run with version Y. There are other plugins too that are obeying toolchain. As a rule, any plugin that are dealing with the compiled classes need to support changing it's jvm (like surefire and compiler are doing).

Maven itself is running with the java executable found in the path or java_home. There is strictly no requirement and no link at all between the jvm running maven and the jdk that should be selected for testing and compiling.

Example: we have some requirements to compile with Zulu and run our test with a selection of JVM and versions we can support on different OSs at customer sites.

But to run maven, the same jvm can be used.

Anyway.... If Maven and its plugins are closing more and more the door to support such use cases, I am then glad we switched some projects to Gradle, which by the way allows that flexibility.

@hazendaz
Copy link
Member

hazendaz commented Oct 15, 2022 via email

@hazendaz hazendaz mentioned this issue Feb 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mathieucarbou @hazendaz @exabrial @jansohn