Run forbidden api checks with runtimeJavaVersion #32947

alpar-t · 2018-08-17T14:08:44Z

Switching to the CLI version of the tool allows us to run it with the runtime java version.
The newly introduced task just a thin wrapper over the cli args and we configure it as before from precommit. Since we only used it in one place, the extensions is no longer used as tasks.withType achieves the same goal.

elasticmachine · 2018-08-17T14:08:45Z

Pinging @elastic/es-core-infra

alpar-t · 2018-08-17T14:09:33Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

+                        spec.classpath(sourceSet.runtimeClasspath)
+                        spec.executable = "${project.runtimeJavaHome}/bin/java"
+                    }
+                    targetCompatibility = JavaVersion.VERSION_1_8 // FIXME: change to min(compilerVersion, 10)


Will wait for the PR fixing the violations to be merged before changing this and merging this PR.

I think this should always be compiler version? That way if eg we use java 11, we will detect problems ahead of time, before we switch to using java 11 all the time?

alpar-t · 2018-08-17T14:13:29Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenApisCliTask.java

+    private Set<String> suppressAnnotations = new LinkedHashSet<>();
+    private JavaVersion targetCompatibility;
+    private FileCollection classesDirs;
+    private Action<JavaExecSpec> execAction;


This avoids inheriting from JavaExec. I prefer that because this way the task can control when to do the exec and we can do the required setup in the task action.
Gradle runs the task action of the inherited task first ( I couldn't find a documented order ).

rjernst

Thanks @atorok. I have a few comments.

rjernst · 2018-08-17T18:34:23Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

+                        spec.classpath(sourceSet.runtimeClasspath)
+                        spec.executable = "${project.runtimeJavaHome}/bin/java"
+                    }
+                    targetCompatibility = JavaVersion.VERSION_1_8 // FIXME: change to min(compilerVersion, 10)


I think this should always be compiler version? That way if eg we use java 11, we will detect problems ahead of time, before we switch to using java 11 all the time?

rjernst · 2018-08-17T18:38:04Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenApisCliTask.java

+        getProject().javaexec((JavaExecSpec spec) -> {
+            execAction.execute(spec);
+            // This works because forbidden apis has no transitive dependencies.
+            spec.classpath(ClassPathUtils.getJar(CliMain.class));


While I know we still need forbidden on the gradle classpath because we invoke it through ant for third-party-audit, I think once we move that to use cli, we should change this to have forbidden api in a separate configuration and pull it there, instead of this hack on finding where a class we have already loaded is located.

rjernst · 2018-08-17T18:38:55Z

buildSrc/src/main/java/org/elasticsearch/gradle/ClassPathUtils.java

+
+public class ClassPathUtils {
+
+    public static Path getJar(Class<?> theClass) {


I don't like this hack, and especially not making it easier to use. Can it be isolated, or at least given large warning comments to not use this?

rjernst · 2018-08-17T18:39:14Z

buildSrc/src/main/java/org/elasticsearch/gradle/ClassPathUtils.java

+            throw new AssertionError(e);
+        }
+        if (Files.exists(path) == false) {
+            throw new AssertionError("Bath to class source does not exist: " + path);


typo: bath -> path

rjernst · 2018-08-17T18:41:03Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

-                        }
-                        existingOutputs.forEach { args "-d", it }
+                    classesDirs = sourceSet.output.classesDirs
+                    ext.replaceSignatureFiles = { String... names ->


Can we stick with the dsl of forbidden apis, instead of adding these helpers? I would like us to be able to switch back if/when it supports forking.

It this is the only concern, we can add these helpers with any implementation of the task.
I don't have a strong preference, but do like how these make the build scripts shorter, without involving build resources.

Alright, that is fine. Can we please name it set instead of replace then, to match common naming with other gradle dsl elements?

@rjernst set will not work, as the task already has that method, so the extension will never be fetched and called, but even with something like setSignatures it would be confusing as I don't think signatures = ... would work, as it won't call the closure on task extensions but look for a set method on the task instance.

I'm going to merge and submit a PR for the third party audit as well.
Let me know and I'll make changes to the name if needed,

rjernst · 2018-08-17T18:43:39Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/NamingConventionsTask.java

+                // This works because the class only depends on one class from junit that will be available from the
+                // tests compile classpath. It's the most straight forward way of telling Java where to find the main
+                // class.
+                ClassPathUtils.getJar(NamingConventionsCheck.class).toFile(),


I would prefer if we kept this url hack as it was and didn't make it easier to use

This reverts commit 668e8b1.

…ks-jvm-31715-part2

alpar-t · 2018-08-20T14:27:37Z

@rjernst I reverted the hack, and wired it to compiler version.

rjernst

LGTM. A couple last nits.

rjernst · 2018-08-20T20:39:26Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

+                        )
+                        spec.executable = "${project.runtimeJavaHome}/bin/java"
+                    }
+                    inputs.files(
                            sourceSet.compileClasspath,


Should the forbidden apis configuration also be an input?

rjernst · 2018-08-20T20:41:15Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenApisCliTask.java

+    }
+
+    @TaskAction
+    public void writeMarker() throws IOException {


nit: this method name does not seem accurate, since it does the actual execution, not just writing the marker file

rjernst · 2018-08-20T20:43:45Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy

-                        }
-                        existingOutputs.forEach { args "-d", it }
+                    classesDirs = sourceSet.output.classesDirs
+                    ext.replaceSignatureFiles = { String... names ->


Alright, that is fine. Can we please name it set instead of replace then, to match common naming with other gradle dsl elements?

Run forbidden APIs checks with runtime hava version

* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)

alpar-t added 4 commits August 16, 2018 20:19

Add a stand alone task for cli invocation

a960c1f

Cut over to cli based implementation

e39bed9

Remove additional configuration

668e8b1

remove implemnted todo

69f6248

alpar-t added >non-issue :Delivery/Build Build or test infrastructure v7.0.0 v6.5.0 labels Aug 17, 2018

alpar-t requested a review from rjernst August 17, 2018 14:08

alpar-t commented Aug 17, 2018

View reviewed changes

rjernst reviewed Aug 17, 2018

View reviewed changes

alpar-t added 3 commits August 20, 2018 16:40

Revert "Remove additional configuration"

479231f

This reverts commit 668e8b1.

Revert to using configuration

5e4506c

Merge remote-tracking branch 'origin/master' into forbidden-APIs-chec…

9b15b73

…ks-jvm-31715-part2

rjernst approved these changes Aug 20, 2018

View reviewed changes

build comments

95c1666

alpar-t merged commit 82d10b4 into elastic:master Aug 22, 2018

alpar-t deleted the forbidden-APIs-checks-jvm-31715-part2 branch August 22, 2018 06:05

alpar-t added a commit to alpar-t/elasticsearch that referenced this pull request Aug 27, 2018

Run forbidden api checks with runtimeJavaVersion (elastic#32947)

5a8427a

Run forbidden APIs checks with runtime hava version

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run forbidden api checks with runtimeJavaVersion #32947

Run forbidden api checks with runtimeJavaVersion #32947

alpar-t commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

alpar-t Aug 17, 2018

rjernst Aug 17, 2018

alpar-t Aug 17, 2018

rjernst left a comment

rjernst Aug 17, 2018

rjernst Aug 17, 2018

rjernst Aug 17, 2018

rjernst Aug 17, 2018

rjernst Aug 17, 2018

alpar-t Aug 20, 2018

rjernst Aug 20, 2018

alpar-t Aug 21, 2018 •

edited

Loading

alpar-t Aug 22, 2018

rjernst Aug 17, 2018

alpar-t commented Aug 20, 2018

rjernst left a comment

rjernst Aug 20, 2018

rjernst Aug 20, 2018

rjernst Aug 20, 2018


		public class ClassPathUtils {

		public static Path getJar(Class<?> theClass) {

Run forbidden api checks with runtimeJavaVersion #32947

Run forbidden api checks with runtimeJavaVersion #32947

Conversation

alpar-t commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Aug 20, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 21, 2018 •

edited

Loading