[WIP] Issues/63 support maven toolchains plugin #64
Conversation
exabrial
changed the title
|
@KengoTODA I'm ok with the fact there are no unit tests. There are none currently at all so that doesn't break with the current state and adding them would sort of mean far more on unit tests than the change would imply. The code otherwise looks good and only enables under certain circumstances which I feel are rather limited in real world use. As the large IT set of tests do not break on this, we should be good. However, @exabrial what might be useful is to write a IT test that covers an example usage of this. Those all run using maven invoker so you should be able to setup one using your expected configuration. I'll hang on this a while before merging in case you can get that together soon. Thanks. |
|
How would one assert which jdk is used, especially since antbuilder.java is a static method? (normally i'd mock out that dependency and check to see if the jvm flag is passed) |
|
@exabrial Look at the IT tests and see if you can create one there that runs with toolchains. I did go ahead and release 3.1.5 so others can use that but need to hold on this until concerns raised are addressed :) |
exabrial
changed the title
|
Attempting an IT... I'm curious what's going to happen on CI |
| @@ -998,31 +1014,43 @@ class SpotBugsMojo extends AbstractMavenReport implements SpotBugsPluginsTrait { | |||
|
|
|||
| def ant = new AntBuilder() | |||
|
|
|||
| log.info("Fork Value is ${fork}") | |||
There was a problem hiding this comment.
please do not remove log that is not related with your fix.
There was a problem hiding this comment.
I found that this log is moved to L1041, what is our motivation?
There was a problem hiding this comment.
No reason, just made a bit more sense in the log flow I thought
| def tc = toolchainManager.getToolchainFromBuildContext("jdk", session); | ||
| if (tc != null) { | ||
| String foundExecutable = tc.findTool("java"); | ||
| if (foundExecutable != null) { |
There was a problem hiding this comment.
We can shorten implementation by safe navigation operator.
There was a problem hiding this comment.
So actually, I need to leave the code this way to be consistent with how the other plugins generate their log messages about using Toolchains. I totally understand the safe operator though, and I'm glad you pointed that out.
| ant.java(classname: "edu.umd.cs.findbugs.FindBugs2", inputstring: getSpotbugsAuxClasspath(), fork: "${fork}", failonerror: "true", clonevm: "false", timeout: "${timeout}", maxmemory: "${maxHeap}m") { | ||
| ant.java([classname: "edu.umd.cs.findbugs.FindBugs2", inputstring: getSpotbugsAuxClasspath(), fork: "${fork}", | ||
| jvm: jvmExecutable, failonerror: "true", clonevm: "false", timeout: "${timeout}", maxmemory: "${maxHeap}m", | ||
| ].findAll{it.value != null}) { |
There was a problem hiding this comment.
Because the jvmExecutable could possibly be null at this point. This ensures null values are dropped from the map. If you pass null to ant.java you get an NPE.
|
btw I'm in another city at the moment, sorry for the slow progress. I don't have much time to work on this but I'll eventually get it working |
|
@KengoTODA Hey could this build possibly be failing because the lack of a toolchains.xml on the build machine? How can we add this file? |
|
How about using |
|
@KengoTODA Just so you're absolutely informed what's going on, it's missing a |
|
The toolchains.xml is supposed to be a machine configuration of where the JVMs are physically located on disk. Including those in a commit means that everyone has to set up their machines in the exact same way. |
|
How about --global-toolchains ? |
|
I'm going to give up :/ There's not a good way to do Integration Tests without control over the environment. I think the original problem I was facing is less of an issue anyway... (cross compiling a jdk 1.8 on a jdk 1.9 maven vm) |
|
Tied to #63 |
No description provided.