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

Add support Java 11 #15

Merged
merged 8 commits into from Jan 26, 2019
Merged

Conversation

matsumana
Copy link
Contributor

@matsumana matsumana commented Jan 16, 2019

Hello.
I want to use this library with Java 8 and 11.

Changes

@codecov
Copy link

@codecov codecov bot commented Jan 16, 2019

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #15   +/-   ##
=========================================
  Coverage     78.55%   78.55%           
  Complexity      141      141           
=========================================
  Files            23       23           
  Lines           569      569           
  Branches         78       78           
=========================================
  Hits            447      447           
  Misses           91       91           
  Partials         31       31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb1ff3f...4e1a50d. Read the comment docs.

compile 'com.google.guava:guava:27.0.1-jre'
compile 'com.squareup:javapoet:1.11.1'
compile 'javax.annotation:javax.annotation-api:1.3.2'
compile 'javax.inject:javax.inject:1'
Copy link
Contributor Author

@matsumana matsumana Jan 16, 2019

Choose a reason for hiding this comment

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

I have a question.
When I tested this PR on my local Docker, I needed to change here.
Otherwise the following exception occurred in ./test/run_tests.sh.
It seems can't resolve library version.

Do you know the reason?

org.gradle.internal.resolve.ModuleVersionNotFoundException: Could not find com.github.spullara.mustache.java:compiler:.

Copy link
Owner

@imasahiro imasahiro Jan 18, 2019

Choose a reason for hiding this comment

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

Could you attach run_tests.sh here? I could not reproduce it without that script.

At least, ModuleVersionNotFoundException will occur if dependency-management-plugin does not work correctly.

Copy link
Contributor Author

@matsumana matsumana Jan 20, 2019

Choose a reason for hiding this comment

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

I've moved the contents to .travis.yml, then deleted the file in ee5abd5.

here is the file.
ee5abd5#diff-d83a620dfa3ec1bb27fd6c6dd172e526

I installed this library to mavenLocal to test it with test module, but the maven-publish plugin may not be work in this source repo.

@imasahiro
Copy link
Owner

@imasahiro imasahiro commented Jan 17, 2019

Thanks for filing a PR. I think it is time to migrate from manual jdk installation to travis-ci managed one. Could you try to replace current jdk scripts with jdk: ...?

jdk:
  - oraclejdk8
  - openjdk11

https://docs.travis-ci.com/user/languages/java/#testing-against-multiple-jdks

@matsumana
Copy link
Contributor Author

@matsumana matsumana commented Jan 17, 2019

Updated.

@imasahiro
Copy link
Owner

@imasahiro imasahiro commented Jan 18, 2019

Add new test project to test with multiple version JDKs

If we set javaSourceCompatibility=1.8 and javaTargetCompatibility=1.8, JDK makes sure the produced binaries can run on JDK8, 9, 10, 11, right? If so, because we set javaSourceCompatibility=1.8 and javaTargetCompatibility=1.8 by default, we don't need to add this test module. Or am I missing something?

@matsumana
Copy link
Contributor Author

@matsumana matsumana commented Jan 20, 2019

If we set javaSourceCompatibility=1.8 and javaTargetCompatibility=1.8, JDK makes sure the produced binaries can run on JDK8, 9, 10, 11, right? If so, because we set javaSourceCompatibility=1.8 and javaTargetCompatibility=1.8 by default, we don't need to add this test module. Or am I missing something?

Right, This test module is not required.
However, I wanted to test whether compilation and execution are work well on both Java 8 and 11, just in case so I added the test module.
If you don't need it, I'd like to delete it.

@imasahiro
Copy link
Owner

@imasahiro imasahiro commented Jan 25, 2019

I see. Then, could you split the PR to 1. introduce javax.annotation-api and switch to travis-ci provided JDK, 2. introduce test module if you don't mind? I would like to take a time to think about how to test compatibility.

@matsumana
Copy link
Contributor Author

@matsumana matsumana commented Jan 26, 2019

Updated.
I only change about 1.

@imasahiro imasahiro merged commit 86a9ada into imasahiro:master Jan 26, 2019
3 checks passed
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.

None yet

2 participants