-
Notifications
You must be signed in to change notification settings - Fork 440
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
Clarify targeted Java version #145
Comments
Would be nice, especially once #146 is resolved. Perhaps Java 1.7? Not sure there's any point in staying with 1.5. I think TestBuilder needs 1.7 for finicky Swing API reasons. |
I think the point here isn't the merits of one version over another, it's about the pom stating the version explicitly: https://github.com/locationtech/spatial4j/blob/master/pom.xml#L172 IMO every project ought to do this explicitly (and vast majority do); it's a shame Maven doesn't make you because it's presumptuous to depend on the JVM the user is using to run Maven itself. |
@dsmiley Fair enough. But in order to state the version we first need to decide what the version should be. |
Are there actually two issues here? Which Java to use for compiling, and which versions JTS will run on? And is there a relationship between them? I.e. if we compile with 1.7 can it still run on say 1.5 or even earlier? |
I don't have experience targeting an earlier Java version than what I'm compiling on, but it seems like it could complicate project development and testing (developers would need to verify that standard library methods used are in fact available in the earlier versions, and that they behave equivalently, right?) Is JTS used on platforms that do not support Java 8? (whether the project wants to use Java 8 features being another matter) |
FWIW you can just add the following properties to the parent pom: <properties>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
.... for java 8 (or 1.7 for java 7 or...), by default maven uses 1.5 because that is what the compiler plugin is defined to do. IMHO, since the new release will also be a change in groupId I'd say it would be a safe bet to pin to 1.7 (unless you want to use closures etc..) |
Can we pick a source and target version for the 1.15.0 release? |
Generally, I'd suggest one of two options: A. Be conservative, and pick something like 1.5. |
just for the record, we use JTS for Android apps. Java 1.8 on android is pretty sketchy - some Java 1.8 classes / methods are unsupported depending on android version etc. Conservative is good for us 👍 https://developer.android.com/studio/write/java8-support.html |
@snodnipper Do you know anything we can use in a Maven build to verify/check that JTS would be compatible with Android? (It sounds like Android compatibility will not be captured entirely by the Maven/compiler language version targeting options.) Overall, I'd like to see JTS be as useful as possible while at the same time letting the project leverage as many Java features as possible. |
@jnh5y - elegantly no. Typically, one would build with the Android gradle build tools, which have all kinds of lint checks. There is the android-maven-plugin but I haven't used that for years. Practically, 1.7 should be fine and CI building/testing with the Android build tools would give feedback without everyone having to install the Android SDK. 1.8 syntax can be ok too but clearly there are API chunks missing for many devices - where the app would terminate. okhttp is an example of a maven project with Android artifacts. We use CircleCI for feedback ourselves, our search repo is an (old) example. |
So is the TLDR: target JTS to build on 1.7 ? If so, that seems reasonable to me. |
Resolves locationtech#145 Signed-off-by: Daniel Baston <dbaston@gmail.com>
Given that JTS now uses interface default methods (for example, here) it seems like the target is now 1.8. |
Staying at Java 1.7 would help us with Android support - see graphhopper/graphhopper#1469 |
Traditionally JTS has tried to stay at older Java versions to help with this very issue (and also to minimize issues in porting new Java language features). The use of interface default methods definitely made the additions to the Coordinate code easier, though. Is there another option for this that would preserve Java 7 compatibility? Thoughts @jodygarnett @jnh5y ? |
If the default interface methods are the only hang-up, I could imagine going back to pick up Java 7 support. That said, @jodygarnett and I were just at a code sprint focusing on Java 11 support for GeoServer.:) Are there any other Java 8 features we are using? |
No other Java8 dependencies t that I know of. Would keeping JTS at Java7 be a problem for Java11 support? |
I don't think there would be any particular issues supporting all the versions. If there are things that come up, we may be able to maintain a few versions/branches for some period of time... |
(I was more commenting out loud. It seems like Android and Oracle may push/pull libraries in different directions.) |
Would it be reasonable to specify the compile version at 1.8 in the root |
Looks like this was done at d13f230. Probably this issue can be closed then? |
For posterity: Java 1.8 is the version JTS uses as of v 1.17. I see it in the release notes too (good). It seems to have come about accidentally but it's just as well to upgrade out of that old version. |
I'm not able to find anything in the POM files or project documentation that states which version of Java is targeted by JTS. (Does Maven default to Java 5?).
The text was updated successfully, but these errors were encountered: