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

Upgrade forbiddenapis Gradle plugin to 3.5.1 #94773

Closed
uschindler opened this issue Mar 27, 2023 · 8 comments · Fixed by #96032
Closed

Upgrade forbiddenapis Gradle plugin to 3.5.1 #94773

uschindler opened this issue Mar 27, 2023 · 8 comments · Fixed by #96032
Assignees
Labels
:Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team

Comments

@uschindler
Copy link
Contributor

uschindler commented Mar 27, 2023

Description

The new version was released a minute ago: https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-35-released-2023-03-27

Summary of relevant changes for Elasticsearch:

  • Faster startup time as Gradle plugin initialization was compiled statically and is part of JAR file
  • Support for Java 20 signatures and bytecode of Java 21

I have no time to submit PR (and don't want to due to legal reasons => no open source license). So this is for informative purposes only.

@uschindler uschindler added >enhancement needs:triage Requires assignment of a team area label labels Mar 27, 2023
@arteam arteam added :Delivery/Build Build or test infrastructure and removed needs:triage Requires assignment of a team area label labels Mar 27, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Delivery Meta label for Delivery team label Mar 27, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@uschindler
Copy link
Contributor Author

uschindler commented Mar 27, 2023

Hi,
I noticed on a fork of Elasticsearch that the thirdPartyAudit Gradle plugin fails now because it parses the output of the checker with regular expressions.
It looks like somebody has to rewrite it and at least remove the "class exclusions" feature: Due to policeman-tools/forbidden-apis#210 the full list of class names that were not found while parsing the 3rd party JAR is no longer printed to stdout in the CLI (and no longer logged as warning). This makes the consistency checks fail. I don't fully understand why it is done like that, but IMHO it should maybe just pass the "ignore-missing-classes" parameter and not parse its output.

@mark-vieira
Copy link
Contributor

mark-vieira commented Mar 27, 2023

@uschindler we do this so that ignoring missing classes is explicit. Essentially, if someone adds a new dependency which references missing classes we want that to be known and either a) add them as exclusions (for example, if those classes are only needed by a feature we don't use) or b) fix our dependencies to bring in the dependency that provides those missing classes.

I think parsing the summary at the end is fine. As long as each missing class is logged at least once that satisfies our use case. I'll see about updating our parsing logic to support the new format.

@uschindler
Copy link
Contributor Author

Hi Mark,
ok. So to me it looks like you are misusing forbiddenapis to not only find JAR files that call forbidden APIs like Runtime.exec() but you also use it to find out, if JAR files refer to classes that are not available in the rest of classpath.
Unfortunately the latest version of forbiddenapis now only collects all missing classes, sorts them and prints the first 3 ones and then an ellipsis. So you won't get the whole list anymore (example from Apache Lucene):

$ gradlew forbiddenApis
Starting a Gradle Daemon (subsequent builds will be faster)

> Task :errorProneSkipped
WARNING: errorprone disabled (skipped on builds not running inside CI environments, pass -Pvalidation.errorprone=true to enable)

> Task :lucene:core:forbiddenApisMain19
While scanning classes to check, the following referenced classes were not found on classpath (this may miss some violations):
  java.lang.foreign.MemorySegment, java.lang.foreign.MemorySession, java.lang.foreign.ValueLayout,... (and 5 more).

> Task :lucene:core:forbiddenApisMain20
While scanning classes to check, the following referenced classes were not found on classpath (this may miss some violations):
  java.lang.foreign.Arena, java.lang.foreign.MemorySegment, java.lang.foreign.SegmentScope,... (and 6 more).

BUILD SUCCESSFUL in 35s
233 actionable tasks: 76 executed, 157 up-to-date

Actually this is also in line with how earlier versions of forbiddenapis behaves (since about version 3.0, not exactly sure) when it parses signatures. If you allow the checker to "ignore signatures where classes are missing" it reports this in the same way. This was added to mitigate complaints by people. Of course because you rely on this feature this is unfortunate (although it is only a side-effect of forbiddenapis that Robert was using back at that time). I wasn't aware of that!

One thing that I could offer: I could maybe add DEBUG (verbose) logging also into the CLI tool (with a command line parameter). In that case it would not show the ellipsis. DEBUG logging was added already for Gradle/Maven/Ant plugins, but not yet for CLI. In addition with a if debug => no ellipsis logic (or switching to a one class per line output), you would be able to still use forbiddenapis to scan for missing classes.

An alternative would be to create your own ASM parser scanning class files (actually thats not too much work, you just need to add a listener that triggers at right places for class names). Or even better scan through the constant pool of classes with another library (there are some) to collect all references from it.

As a workaround you could update forbiddenapis to 3.5 now only for the "normal forbiddenapis" task, but use the 3.4 version for the forbiddenApisCLI dependency.

@uschindler
Copy link
Contributor Author

If you want me to add DEBUG logging to the CLI tool, just open an issue on forbiddenapis referring to my above description. I can quickly release a bugfix version 3.5.1, so you won't have to wait till Java 21.

@mark-vieira
Copy link
Contributor

So to me it looks like you are misusing forbiddenapis

That is accurate 😉

An alternative would be to create your own ASM parser scanning class files (actually thats not too much work, you just need to add a listener that triggers at right places for class names). Or even better scan through the constant pool of classes with another library (there are some) to collect all references from it.

That's certainly possible, but this class parsing is expensive, especially with large classpaths, so since forbidden APIs already has to do this for it's purposes are simply taking advantage of this.

If you want me to add DEBUG logging to the CLI tool, just open an issue on forbiddenapis referring to my above description. I can quickly release a bugfix version 3.5.1, so you won't have to wait till Java 21.

Thanks, @uschindler! I've opened policeman-tools/forbidden-apis#226.

@uschindler
Copy link
Contributor Author

Hi,
version 3.5.1 was released to work around your problem. Basically adding the --debug option to the CLI script and adapting the regular expression a bit should be enough to restore previous behaviour.

https://github.com/policeman-tools/forbidden-apis/wiki/Changes#version-351-released-2023-03-30

@uschindler uschindler changed the title Upgrade forbiddenapis Gradle plugin to 3.5 Upgrade forbiddenapis Gradle plugin to 3.5.1 Mar 30, 2023
@breskeby
Copy link
Contributor

fixed by #96032

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 >enhancement Team:Delivery Meta label for Delivery team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants