Add a task to run forbiddenapis using cli #32076

alpar-t · 2018-07-16T09:07:34Z

Add a task that offers an equivalent check to the forbidden APIs plugin,
but runs it using the forbiddenAPIs CLI instead.

This isn't wired into precommit first, and doesn't work for projects
that require specific signatures, etc. It's meant to show how this can
be used. The next step is to make a custom task type and configure it
based on the project extension from the pugin and make some minor
adjustments to some build scripts as we can't bee 100% compatible with
that at least due to how additional signatures are passed.

Notes:

there's no --target for the CLI version so we have to pass in
specific bundled signature names
the cli task already wires to runtimeJavaHome
no equivalent for failOnUnsupportedJava = false but doesn't seem to
be a problem. Tested with Java 8 to 11
there's no way to pass additional signatures as URL, these will have
to be on the file system, and can't be resources on the cp unless we
rely on how forbiddenapis is implemented and mimic them as boundled
signatures.
the average of 3 runs is 4% slower using the CLI for :server.
( ./gradlew clean :server:forbiddenApis vs ./gradlew clean :server:forbiddenApisCli)
up-to-date checks don't work on the cli task yet, that will happen
with the custom task.

See also: #31715

Add a task that offers an equivalent check to the forbidden APIs plugin, but runs it using the forbiddenAPIs CLI instead. This isn't wired into precommit first, and doesn't work for projects that require specific signatures, etc. It's meant to show how this can be used. The next step is to make a custom task type and configure it based on the project extension from the pugin and make some minor adjustments to some build scripts as we can't bee 100% compatible with that at least due to how additional signatures are passed. Notes: - there's no `--target` for the CLI version so we have to pass in specific bundled signature names - the cli task already wires to `runtimeJavaHome` - no equivalent for `failOnUnsupportedJava = false` but doesn't seem to be a problem. Tested with Java 8 to 11 - there's no way to pass additional signatures as URL, these will have to be on the file system, and can't be resources on the cp unless we rely on how forbiddenapis is implemented and mimic them as boundled signatures. - the average of 3 runs is 4% slower using the CLI for :server. ( `./gradlew clean :server:forbiddenApis` vs `./gradlew clean :server:forbiddenApisCli`) - up-to-date checks don't work on the cli task yet, that will happen with the custom task. See also: elastic#31715

elasticmachine · 2018-07-16T09:07:42Z

Pinging @elastic/es-core-infra

nik9000

This seems like a good start for forking this. I left some minor things.

nik9000 · 2018-07-16T13:19:35Z

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

+            forbiddenApisCliJar 'de.thetaphi:forbiddenapis:2.5'
+        }
+        Task forbiddenApisCli = project.tasks.create('forbiddenApisCli')
+        project.ext.getForbiddenSignaturesFile = { name -> project.getForbiddenSignaturesFile(name) }


Do we need this one? It looks circular.

It's so we can call getForbiddenSignaturesFile(name) from a build script instead of import ... ; PrecommitTasks.getForbiddenSignaturesFile(project, name)

I'll remove it for now, and add it back once it's actually used.

nik9000 · 2018-07-16T13:20:15Z

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

+        return forbiddenApisCli
+    }
+
+    private static File getForbiddenSignaturesFile(project, String name) {


Could you add a type to the project parameter?

I haven't found a good way to fight IDEA on this ...
The refactoring don't add a type. Will change it.

nik9000

LGTM

rjernst

This looks good. The only critical thing I see is how the resource files are loaded.

rjernst · 2018-07-17T16:13:35Z

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

+                    executable = "${project.runtimeJavaHome}/bin/java"
+                    [
+                            'jdk-unsafe-1.8', 'jdk-deprecated-1.8', 'jdk-non-portable', 'jdk-system-out'
+                    ].forEach { args "-b", it }


I think this would be easier to read not using a foreach. Just call args directly with each argument.

rjernst · 2018-07-17T16:13:48Z

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

+                    [
+                            getForbiddenSignaturesFile(project, 'jdk-signatures'),
+                            getForbiddenSignaturesFile(project, 'es-all-signatures')
+                    ].forEach { args "-f", it }


Same here, the foreach just makes this harder to read.

rjernst · 2018-07-17T16:22:30Z

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

+    }
+
+    private static File getForbiddenSignaturesFile(Project project, String name) {
+        return project.rootProject.file('buildSrc/src/main/resources/forbidden/' + name + '.txt')


We can't assume this is accessible, since build-tools is used by external plugin developers. We need to load this from resources and copy to a local file.

I wasn't aware we publish group: 'org.elasticsearch.gradle', name: 'build-tools', version: '6.3.1' and encourage others to use it. Is this fully documented or more like plugin authors picked up on it ? I don't think we have documentation on any of the build plugins. I think I broke this assumption recently with the naming conventions check, I'll take a look and see if we can add some tests for this.

Naming conventions check is fine, it looks up the source jar of the relevant class so works as long as the jar is on the file system, the class doesn't have to be. I'll still add a test for this.

It's not "documented", but we have encouraged plugin authors to use it since we switched to gradle in 5.0. We have long needed a permanent home for plugin author documentation, but we probably need a broader reorg of the docs to enable that, as there are a few parts for plugin authors to consume eg how to build, how to test, core apis and services available, how to publish, etc.

Fails with: ``` ERROR: IO problem while reading files with API signatures: java.io.FileNotFoundException: /home/alpar/work/elastic/elasticsearch/buildSrc/src/testKit/forbidden-apis/buildSrc/src/main/resources/forbidden/jdk-signatures.txt (No such file or directory) ``` because signature definitiions are accessed on the FS directly.

alpar-t · 2018-07-20T03:59:55Z

Depends on #32201 for completion.

…resources

This reverts commit 60570d7.

…hecks-jvm-31715

alpar-t · 2018-08-10T15:19:02Z

@rjernst This now uses the buildResources task to pass in the files.

rjernst

LGTM aside from the naming I noted in #32201

alpar-t · 2018-08-13T05:50:07Z

I changed the name tocopy

…listeners * elastic/master: (58 commits) [ML] Partition-wise maximum scores (elastic#32748) [DOCS] XContentBuilder#bytes method removed, using BytesReference.bytes(docBuilder) (elastic#32771) HLRC: migration get assistance API (elastic#32744) Add a task to run forbiddenapis using cli (elastic#32076) [Kerberos] Add debug log statement for exceptions (elastic#32663) Make x-pack core pull transport-nio (elastic#32757) Painless: Clean Up Whitelist Names (elastic#32791) Cat apis: Fix index creation time to use strict date format (elastic#32510) Clear Job#finished_time when it is opened (elastic#32605) (elastic#32755) Test: Only sniff host metadata for node_selectors (elastic#32750) Update scripted metric docs to use `state` variable (elastic#32695) Painless: Clean up PainlessCast (elastic#32754) [TEST] Certificate NONE not allowed in FIPS JVM (elastic#32753) [ML] Refactor ProcessCtrl into Autodetect and Normalizer builders (elastic#32720) Access build tools resources (elastic#32201) Tests: Disable rolling upgrade tests with system key on fips JVM (elastic#32775) HLRC: Ban LoggingDeprecationHandler (elastic#32756) Fix test reproducability in AbstractBuilderTestCase setup (elastic#32403) Only require java<version>_home env var if needed Tests: Muted ScriptDocValuesDatesTests.testJodaTimeBwc ...

* Add a task to run forbiddenapis using cli Add a task that offers an equivalent check to the forbidden APIs plugin, but runs it using the forbiddenAPIs CLI instead. This isn't wired into precommit first, and doesn't work for projects that require specific signatures, etc. It's meant to show how this can be used. The next step is to make a custom task type and configure it based on the project extension from the pugin and make some minor adjustments to some build scripts as we can't bee 100% compatible with that at least due to how additional signatures are passed. Notes: - there's no `--target` for the CLI version so we have to pass in specific bundled signature names - the cli task already wires to `runtimeJavaHome` - no equivalent for `failOnUnsupportedJava = false` but doesn't seem to be a problem. Tested with Java 8 to 11 - there's no way to pass additional signatures as URL, these will have to be on the file system, and can't be resources on the cp unless we rely on how forbiddenapis is implemented and mimic them as boundled signatures. - the average of 3 runs is 4% slower using the CLI for :server. ( `./gradlew clean :server:forbiddenApis` vs `./gradlew clean :server:forbiddenApisCli`) - up-to-date checks don't work on the cli task yet, that will happen with the custom task. See also: #31715

alpar-t requested review from nik9000, rjernst and jasontedor July 16, 2018 09:07

alpar-t added the :Delivery/Build Build or test infrastructure label Jul 16, 2018

nik9000 requested changes Jul 16, 2018

View reviewed changes

PR review

4f30963

nik9000 approved these changes Jul 17, 2018

View reviewed changes

rjernst requested changes Jul 17, 2018

View reviewed changes

alpar-t added 3 commits July 18, 2018 17:25

Task skeleton with test failing due to missing implementation

0fbfdfb

Task implementation and complete task

02d7bf0

alpar-t added the stalled label Jul 20, 2018

alpar-t added 7 commits July 25, 2018 01:13

No more whitelist, reduce boilerplate

5bb35ac

Merge remote-tracking branch 'origin/master' into access-build-tools-…

ac290de

…resources

Merge remote-tracking branch 'origin/master' into access-build-tools-…

c46d30b

…resources

remove unused enum

8cfbd67

Revert "Add failing test to highlight paths issue"

f771138

This reverts commit 60570d7.

Merge branch 'access-build-tools-resources' into fix/forbidden-APIs-c…

352e215

…hecks-jvm-31715

Use the build resources task for running forbidden apis with teh CLI

3249464

Merge branch 'master' into fix/forbidden-APIs-checks-jvm-31715

6061ce5

rjernst approved these changes Aug 10, 2018

View reviewed changes

rename method to copy

5531638

alpar-t merged commit 02f2fad into elastic:master Aug 13, 2018

alpar-t deleted the fix/forbidden-APIs-checks-jvm-31715 branch August 13, 2018 05:52

alpar-t added v7.0.0 v6.5.0 and removed stalled labels Aug 22, 2018

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

Add a task to run forbiddenapis using cli #32076

Add a task to run forbiddenapis using cli #32076

alpar-t commented Jul 16, 2018

elasticmachine commented Jul 16, 2018

nik9000 left a comment

nik9000 Jul 16, 2018

alpar-t Jul 17, 2018

alpar-t Jul 17, 2018

nik9000 Jul 16, 2018

alpar-t Jul 17, 2018

nik9000 left a comment

rjernst left a comment

rjernst Jul 17, 2018

rjernst Jul 17, 2018

rjernst Jul 17, 2018

alpar-t Jul 18, 2018

alpar-t Jul 18, 2018

rjernst Jul 18, 2018

alpar-t commented Jul 20, 2018

alpar-t commented Aug 10, 2018

rjernst left a comment

alpar-t commented Aug 13, 2018

Add a task to run forbiddenapis using cli #32076

Add a task to run forbiddenapis using cli #32076

Conversation

alpar-t commented Jul 16, 2018

elasticmachine commented Jul 16, 2018

nik9000 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

nik9000 left a 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

alpar-t commented Jul 20, 2018

alpar-t commented Aug 10, 2018

rjernst left a comment

Choose a reason for hiding this comment

alpar-t commented Aug 13, 2018