Netflix / denominator

Portably control DNS clouds using java or bash
Apache License 2.0
580 stars 110 forks source link

Ensure java 6 source compatibility #344

Closed codefromthecrypt closed 9 years ago

codefromthecrypt commented 9 years ago

The other day, I had a pull request bug as my code depended accidentally on a java 8 default method. Seems it should not have built on any JDK. I wonder if our gradle can be tightened. I'd have guessed that the following should have done it. Either that's not being read or we need something else like animal sniffer.

sourceCompatibility = 1.6
rspieldenner commented 9 years ago

So this is a problem with the jdk in general. Source compatibility will keep you from using language features, so 1.6 will flag if you use lambdas or multicatches, or diamond operators but not if you use a method on a class that didn't exist in 1.6. Only real way to catch would be to run compilation and tests in the target jdk. Our Nebula plugins won't run in JDK6 because we started to use JDK7 features and since 6 was EOL a long time ago and 7 is EOL at the end of April we won't move the plugins themselves to be JDK6 compatible. Cloudbees and TravisCI are both set to run in JDK7 so the pull request jobs should fail on JDK8 only methods.

On Tue, Mar 17, 2015 at 8:43 AM, Adrian Cole notifications@github.com wrote:

Assigned #344 https://github.com/Netflix/denominator/issues/344 to @rspieldenner https://github.com/rspieldenner.

— Reply to this email directly or view it on GitHub https://github.com/Netflix/denominator/issues/344#event-257032833.

codefromthecrypt commented 9 years ago

I see what you mean, though it seems that default methods on an interface would be caught in source compatibility, right? Ex. that wasn't possible in the language before 8.

Animal sniffer should be able to take care of at least some of this. I've used this before in maven, but not yet gradle http://mojo.codehaus.org/animal-sniffer-maven-plugin/

I probably need to experiment more to see if indeed JDK8 with source compatibility set to 6 indeed should compile fail on default method usage, or if I was just crazy earlier.

ps this is more about not having to deal with the weird language support matrix on android than being able to run on JRE 6

codefromthecrypt commented 9 years ago

Ok.

Looks like we would need to have javac's boot classpath set on the host we are building in order to enforce a language level.

$ cat MyIterator.java 
class MyIterator implements java.util.Iterator<String> {
  public boolean hasNext() {
    return false;
  }

  public String next() {
    throw new IllegalStateException("I said I don't have next!");
  }
  // note this lacks remove()!
}
# Note this didn't fail as I didn't set bootclasspath to correspond with the source version
$ javac -source 1.6 MyIterator.java 
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=utf8
warning: [options] bootstrap class path not set in conjunction with -source 1.6
1 warning
codefromthecrypt commented 9 years ago

I think adding animal-sniffer for 1.6 is probably the highest leverage action to take.

Travis running 1.7 will catch most other things that would affect android (ex. accidental use of lambda or default methods).