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

Run forbidden APIs checks in a forked JVM so we can run it for any version #31715

Closed
alpar-t opened this issue Jul 2, 2018 · 3 comments
Closed
Assignees
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team

Comments

@alpar-t
Copy link
Contributor

alpar-t commented Jul 2, 2018

The forbidden APIs precommit check runs with the same Java version as Gradle, creating tight coupling between our build JDK and runtime JVM setting. We would like to run this check forked, so we can run it with the runtime JVM regardless of what's being used to build the project.
Since we already require env vars for the Java home of different versions, we could also run it for multiple java versions if so desired.

@alpar-t alpar-t added the :Delivery/Build Build or test infrastructure label Jul 2, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 4, 2018

These checks are implemented in https://github.com/policeman-tools/forbidden-apis, including the Gradle plugin, which does not currently support setting and external JDK ( like JavaCompile does ).

After looking at how Gradle implements JavaCompile for a bit, adding support for such a feature is not trivial. It also got me wondering why it's needed in the first place, @rjernst.
Running with a newer jre should allow us to inspect older versions of the byte-code, so why does it matter ?

It does matter when we have rules for JDK methods such as java.lang.Thread#stop(java.lang.Throwable) which was removed in Java 11, but only because the rule engine can't resolve the method. That could be addressed by failOnMissingClasses, and getFailOnUnresolvableSignatures but that doesn't buy us much, since the code would not compile on newer JDK version, so we would detect it anyhow.

I'm not sure what would happen if a third party jar would use such a removed method ? Would it fail at runtime ? But we wouldn't detect such usage anyhow since we don't inspect the bytecode of everything on the classppath, just the class files of the project.

@alpar-t alpar-t removed the stalled label Jul 13, 2018
@alpar-t
Copy link
Contributor Author

alpar-t commented Jul 13, 2018

We do have a separate check for third party dependencies, and we do need to check with specific java versions due to multi release jars.

alpar-t added a commit to alpar-t/elasticsearch that referenced this issue Jul 16, 2018
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
alpar-t added a commit that referenced this issue Aug 13, 2018
* 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 added a commit that referenced this issue Aug 22, 2018
* 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 added a commit that referenced this issue Aug 28, 2018
The new implementation is functional equivalent with the old, ant based one.
It parses task standard error to get the missing classes and violations in the same way.
I considered re-using ForbiddenApisCliTask but Gradle makes it hard to build inheritance with tasks that have task actions , since the order of the task actions can't be controlled.
This inheritance isn't dully desired either as the third party audit task is much more opinionated and we don't want to expose some of the configuration.
We could probably extract a common base class without any task actions, but probably more trouble than it's worth.

Closes #31715
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team
Projects
None yet
Development

No branches or pull requests

3 participants