feat: Allow passing environment variables in maven launcher invocations #4830
Conversation
Some maven builds might need environment variables to complete successfully.
|
Okay, I am stumped. I am not sure why the maven invocation fails, but it only seems to fail on windows. I don't have a usable windows install right now, so I'd be grateful if somebody else could help here :) |
| @AfterEach | ||
| void tearDown() throws IOException { | ||
| // Clean up old classpath files to ensure tests actually re-run properly | ||
| try (Stream<Path> paths = Files.walk(Path.of("src/test/resources/maven-launcher"))) { | ||
| List<Path> filesToDelete = paths | ||
| .filter(it -> it.getFileName().toString().equals("spoon.classpath.tmp")) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| for (Path path : filesToDelete) { | ||
| Files.deleteIfExists(path); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it would be better if the tests were executed in a temporary directory instead (e.g. using @TempDir from JUnit5).
There was a problem hiding this comment.
This is something of a systematic error we're doing (i.e. just running in the local directory), but perhaps this PR would be a good driving force to actually start running tests appropriately isolated.
There was a problem hiding this comment.
Yea, that was a bit lazy… In exchange I have converted all launcher tests (that aren't disabled) to use temporary folders. The diff is a bit more annoying now but I hope still readable enough.
|
|
||
| boolean containsSpoonDependency = Arrays | ||
| .stream(launcher.getEnvironment().getSourceClasspath()) | ||
| .anyMatch(it -> it.matches(".*fr.inria.gforge.spoon.spoon-core.10.1.0.spoon-core-10.1.0.jar")); |
There was a problem hiding this comment.
Are you entirely certain the classpath will end with this dependency? Perhaps the test fails on Windows because it's somewhere else?
| .anyMatch(it -> it.matches(".*fr.inria.gforge.spoon.spoon-core.10.1.0.spoon-core-10.1.0.jar")); | |
| .anyMatch(it -> it.matches(".*fr.inria.gforge.spoon.spoon-core.10.1.0.spoon-core-10.1.0.jar.*")); |
There was a problem hiding this comment.
That's a good spot, but:
It is an array with one entry per dependency, so it should be fine. The classpath printed in the CI is an empty array, so nothing is found. I could observe that happening locally if the maven invocation failed and did not produce any output, but the invocation request does not seem to throw an exception in such cases.
There was a problem hiding this comment.
It probably failed then. We've had trouble with Maven on Windows before. I'll have a peek once I get home then!
| * Additional options for maven invocations. | ||
| */ | ||
| @Internal | ||
| public static class MavenOptions { |
There was a problem hiding this comment.
What's the reasoning for making this a nested class in SpoonPom? Barring the fact that it's used by SpoonPom, it doesn't seem intrinsically related.
There was a problem hiding this comment.
I wanted to keep this an internal implementation detail and the directory SpoonPom resides in didn't feel adequate (it doesn't have anything to do with compilers). Additionally it is quite closely related so I hoped I could get away with it.
If you feel that's too much of a stretch I can extract it with a more verbose name :)
There was a problem hiding this comment.
the directory SpoonPom resides in didn't feel adequate
That we can agree on. It would have to go into an internal subpackage, and that'd put it far away from it's currently singular use. I mostly wanted to see if you'd thought it through, which you have, so let's leave it where it is :)
I-Al-Istannen
force-pushed
the
feat/maven-launcher-options
branch
from
dbafdce
to
bc56684
Compare
Why does it work without `env.` on linux???
This is not documented as far as I can tell, but it works-on-my-machine:tm: and just failed on windows. Let's see.
This reverts commit 24db9e4.
|
@slarse Okay, I think I solved it. The docs say that only The problem is that maven normalizes all env variables to upper case – but apparently not the occurences in the pom. So |
Some maven builds might need environment variables to complete successfully and we should offer an API for this.
As the methods I overloaded are part of the public API, I enclosed the environment variables in an opaque settings object. This should allow us to add more options later without another overload layer per feature. I am not sure if this is a good idea, but it might ease the burden a bit.
Closes: #4817