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

JDK version determined incorrectly by Gradle on JDK 10 #143

Closed
sdavids opened this issue Aug 28, 2018 · 16 comments
Closed

JDK version determined incorrectly by Gradle on JDK 10 #143

sdavids opened this issue Aug 28, 2018 · 16 comments
Assignees

Comments

@sdavids
Copy link

sdavids commented Aug 28, 2018

https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures
"Gradle automatically add the compile Java version"

forbiddenApis {
  bundledSignatures = ['jdk-internal']
}

Error:
Parsing signatures failed: Invalid bundled signature reference (JDK version is invalid): jdk-internal-1.10

Workaround:

forbiddenApis {
  bundledSignatures = ['jdk-internal-10']
}
@sdavids
Copy link
Author

sdavids commented Aug 28, 2018

org.gradle.api.JavaVersion

    // Since Java 9, version should be X instead of 1.X
    // However, to keep backward compatibility, we change from 11

Possible fix:

def javaVersion = JavaVersion.current()

def fixed = javaVersion.java9Compatible ? javaVersion.majorVersion : javaVersion

@sdavids sdavids closed this as completed Aug 28, 2018
@sdavids sdavids reopened this Aug 28, 2018
@uschindler
Copy link
Member

This is not a bug. The version should anyways be defined in the project setup, checking against the current version is wrong anyways.

@sdavids
Copy link
Author

sdavids commented Aug 29, 2018

https://docs.gradle.org/current/userguide/java_plugin.html#other_convention_properties

Neither setting sourceCompatibility nor targetCompatibility is not recommended but allowed.

Both sourceCompatibility and targetCompatibility are of type org.gradle.api.JavaVersion.

JavaVersion.VERSION_1_10 => jdk-internal-1.10 => does not work
JavaVersion.valueOf("1.10") => jdk-internal-1.10 => does not work
JavaVersion.valueOf("10") => No enum constant JavaVersion.10 => does not work
JavaVersion.toVersion("1.10") => jdk-internal-1.10 => does not work
JavaVersion.toVersion("10") => No enum constant JavaVersion.10 => does not work

@uschindler
Copy link
Member

Actually this is a serious bug in Gradle. The forbiddenapis backend solely uses official version numbers, as accepted by javac. Javac does not accept "1.[9|10|11]" as version for the new -release parameter, so i have no idea, how the compile task handles that internally. Forbiddenapis enforces full version numbers for Java versions >=9, for previous versions it accepts both variants ("1.8" and "8"), but normalizes them (to "1.8").
Nevertheless, the targetCompatibility property of the forbiddenapis task is of type String (for good reason), so you can simply specify it as a string in the task config, no need to rely on shitty Gradle enum constants.
I am not sure If I should add a hack for this because the forbiddenapis main task is about correctness, so I'd like to enforce correctness - also in Gradle. It works correctly in Maven and Ant, only Gradle is broken (as always).

@uschindler
Copy link
Member

Neither setting sourceCompatibility nor targetCompatibility is not recommended but allowed.

For build reproducibility it is required to be set. Otherwise the project build depends on the actually used Java version and produces artifacts solely working with this Java version. This may also cause compile errors, as the source code parsing uses an undefined Java version.

@uschindler uschindler self-assigned this Aug 29, 2018
@dweiss
Copy link
Contributor

dweiss commented Aug 29, 2018

Sadly even setting source/target doesn't always help (for example, covariants on ByteBuffer.flip() can cause incompatibilities if compiled under java 9 with 1.8 as the target).

@sdavids
Copy link
Author

sdavids commented Aug 29, 2018

Actually this is a serious bug in Gradle

https://github.com/gradle/gradle/blob/master/subprojects/base-services/src/main/java/org/gradle/api/JavaVersion.java#L46

For build reproducibility it is required to be set.

I know but some people like pain 😳

@uschindler
Copy link
Member

uschindler commented Aug 29, 2018 via email

@uschindler
Copy link
Member

@sdavids I can add a fix there, but I would do it only for the 2 enum entries (Java 9 and Java 10). Starting with Java 11, Gradle behaves correct. So it would apply the setting as-is, but for Java 9 and Java 10 it would set it to "9" and "10" manually.

@uschindler
Copy link
Member

My fix would be (just workaround the 2 bogus versions):

// Define our tasks (one for each SourceSet):
project.sourceSets.all{ sourceSet ->
  def getSourceSetClassesDirs = { sourceSet.output.hasProperty('classesDirs') ? sourceSet.output.classesDirs : project.files(sourceSet.output.classesDir) }
  project.tasks.create(sourceSet.getTaskName(FORBIDDEN_APIS_TASK_NAME, null), CheckForbiddenApis.class) { task ->
    description = "Runs forbidden-apis checks on '${sourceSet.name}' classes.";
    conventionMapping.with{
      extensionProps.each{ key ->
        map(key, { extension[key] });
      }
      classesDirs = { getSourceSetClassesDirs() }
      classpath = { sourceSet.compileClasspath }
      targetCompatibility = { EnumSet.of(JavaVersion.VERSION_1_9,JavaVersion.VERSION_1_10).contains(project.targetCompatibility) ? project.targetCompatibility.majorVersion : project.targetCompatibility?.toString() }
    }
    // add dependency to compile task after evaluation, if the classesDirs collection has overlaps with our SourceSet:
    project.afterEvaluate{
      def sourceSetDirs = getSourceSetClassesDirs().files;
      if (classesDirs.any{ sourceSetDirs.contains(it) }) {
        task.dependsOn(sourceSet.output);
      }
    }
    forbiddenTask.dependsOn(task);
  }
}

@uschindler
Copy link
Member

Actually the fix is much easier:

      targetCompatibility = { project.targetCompatibility?.majorVersion }

This works, because Forbiddenapis always accepts the pure Major version (6,7,8,9,10,...) (like Java does, too). I thinks that fix is easiest.

@uschindler
Copy link
Member

The previous suggestion failed with older Gradle versions, as the enum constants were missing!

Any other suggestions?

@uschindler uschindler reopened this Aug 29, 2018
@uschindler
Copy link
Member

I checked the old Gradle docs. Minimum Gradle version of forbiddenapis is 2.3. This one has (https://docs.gradle.org/2.3/javadoc/org/gradle/api/JavaVersion.html) the Major Version property, so we can use it. This is also tested on Jenkins. I changed the check now to use the major version for everything below Java 11. Starting from Java 11 (where gradle is correct), it uses toString(). The latter is done to be future-proof (if JDK decides to add minor versions again,...).

@uschindler
Copy link
Member

uschindler commented Aug 29, 2018

I will commit this workaround and let Jenkins decide if it's fine with Gradle:

targetCompatibility = { (project.targetCompatibility?.hasProperty('java11Compatible') && project.targetCompatibility?.java11Compatible) ?
  project.targetCompatibility.toString() : project.targetCompatibility?.majorVersion }

@uschindler
Copy link
Member

I will release a new version anyways for compatibility with Java 11, so this fix should be out soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants