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

Java 8 and Linux/macos/Windows targets for Travis #176

Merged
merged 5 commits into from
Jul 7, 2020

Conversation

adamretter
Copy link
Contributor

@adamretter adamretter commented Jul 3, 2020

  • Minimum is now Java 8
  • Travis CI now tests for Java 8 on:
    • Linux (Ubuntu - Xenial)
    • macOS (10.13)
    • Windows

mathieucarbou
mathieucarbou previously approved these changes Jul 3, 2020
@adamretter adamretter changed the title Add Windows and macOS to Travis Java 8 and Linux/macos/Windows targets for Travis Jul 3, 2020
@adamretter
Copy link
Contributor Author

@mathieucarbou I think this one is now ready to merge.

There are 2 failing Windows tests as expected:

Tests in error: 
  ResourceFinderTest.test_load_absolute_file:47 � MojoFailure Resource C:\Users\...
  ResourceFinderTest.test_load_from_URL:74 � MojoFailure Resource file:///C:/Use...

@mathieucarbou
Copy link
Owner

@mathieucarbou I think this one is now ready to merge.

There are 2 failing Windows tests as expected:

Tests in error: 
  ResourceFinderTest.test_load_absolute_file:47 � MojoFailure Resource C:\Users\...
  ResourceFinderTest.test_load_from_URL:74 � MojoFailure Resource file:///C:/Use...

Hi @adamretter,
Thank you for the travis update!
This highlights the test issues.
We cannot merge right now : we first need to ensure all tests pass. Otherwise all subsequent PRs would also fail.
I would rather avoid to allow a PR to be merged if "at least" the linux build passed for this project.

=> did you already have a look at the failures or not ? Just let me know if you did and found a quick way to solve them :-)

@adamretter
Copy link
Contributor Author

did you already have a look at the failures or not ? Just let me know if you did and found a quick way to solve them :-)

No I haven't had time to look at them.

@adamretter
Copy link
Contributor Author

@mathieucarbou Okay I fixed the path issues. Some were in the main code, and some were in the tests.

@mathieucarbou
Copy link
Owner

@mathieucarbou Okay I fixed the path issues. Some were in the main code, and some were in the tests.

Thanks a lot!
Also, BIG thank you for all your involvement in this plugin 👍
Without you, probably there wouldn't have been a 4.x release any time soon!

@adamretter
Copy link
Contributor Author

@mathieucarbou You are absolutely welcome. I have used your plugin in many projects for many years - so I am happy of course to contribute back. Thanks for making it easy :-)

Copy link
Contributor

@McFoggy McFoggy left a comment

Choose a reason for hiding this comment

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

I am ok with the changeset but have some concerns regarding the groovy compiler stuff.

It is not explicitly the problem of this PR (issue coming from parent pom probably) but stills it looks weird.

Comment on lines +191 to +200
<dependency>
<groupId>org.codehaus.groovy</groupId>
<artifactId>groovy-eclipse-compiler</artifactId>
<version>3.6.0-03</version>
</dependency>
<dependency>
<groupId>org.codehaus.groovy</groupId>
<artifactId>groovy-eclipse-batch</artifactId>
<version>3.0.4-02</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

why is groovy compiler added ?

Copy link
Contributor

Choose a reason for hiding this comment

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

might be a requirement of the parent pom, but looks quite weird to me.

Copy link
Contributor Author

@adamretter adamretter Jul 6, 2020

Choose a reason for hiding this comment

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

@McFoggy So the issue was this...

We need to specify the source and target version of Java to the Maven Compiler Plugin to ensure that we get 1.8. We are also using the maven-integration-plugin to do integration tests. The maven-integration-plugin expects you to write test scripts in Groovy, which we have done.

When the Maven Compiler Plugin is executed, it sees that we have both Java and Groovy code and so chooses to use the groovy-eclipse-compiler so that it can compile both types of code. Unfortunately the Maven Compiler Plugin uses an older version of the groovy-eclipse-compiler which does not like the <source>1.8</source> and <target>1.8</target> arguments and fails the build. Updating to the latest groovy-eclipse-compiler fixes the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mathieucarbou @adamretter I also use maven-invoker in other projects (see jgitver-maven-plugin for example) and I do not need to explicitly add groovy support.

Groovy is a dependency of maven-invoker and invoker groovy scripts should be evaluated during IT execution.
In this particular case, this PR I mean, the groovy dependencies are introduced by the parent pom (with old versions of groovy compiler) ; I don't know why they are introduced at that level. Then if removed from this pom the build itself fails.
The question would be why do we need groovy support for the project ?

FYI: when executiong the build you will see that groovy compiler is trying to find to find groovy scripts in src/main/groovy and src/test/groovy which is not what is used right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@McFoggy I am not really sure how to explain it in a different way than I did above. Would it be easier perhaps to have a call where we share a screen and try to resolve this?

Copy link
Contributor

@McFoggy McFoggy Jul 7, 2020

Choose a reason for hiding this comment

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

@adamretter As I said, I am ok with the PR. I was just saying that the groovy part is useless (because not used) but mandatory due to the missed but forced usage from the parent pom.

See maven-invoker-plugin documentation to realize that invoker tests using groovy do not need groovy compilation (which is normal because groovy in most cases is used as a scripting/runtime feature)

If we want to be clean then we "just" have to open another issue to evaluate the need of keeping the groovy compiler part (which would require a cleanup or to abandon the parent pom).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@McFoggy Okay... But how would you suggest that I improve this without breaking the build?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it in the PR first and then tackle it in another issue.

The PR is not about cleaning unused groovy compilers ; let's keep it focused.

Copy link
Owner

@mathieucarbou mathieucarbou Jul 7, 2020

Choose a reason for hiding this comment

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

The parent pom is very old... I had to override some plugins from the parent pom to be able to release the 4.0.rc1 some days ago. But. Ido not see anything related to groovy in the parent pom ? The parent pom is declaring some plugins in the plugin dependency management section but does not enforce its usage. The only plugin called in the release profile is the groovydoc-maven-plugin, which could easily be disabled by overriding its phase to none.
Where is the problem from the parent pom ?
Sorry - understand after reading @adamretter's message. All good :-)

@mathieucarbou
Copy link
Owner

try to clean the stale github status about travis

@mathieucarbou mathieucarbou merged commit 2c0b240 into mathieucarbou:master Jul 7, 2020
@mathieucarbou mathieucarbou added this to the 4.0.rc2 milestone Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants