Skip to content

Upgrade forbiddenapis to 2.6#33809

Merged
alpar-t merged 5 commits intoelastic:masterfrom
alpar-t:upgrade-forbiddenapis-33759
Oct 23, 2018
Merged

Upgrade forbiddenapis to 2.6#33809
alpar-t merged 5 commits intoelastic:masterfrom
alpar-t:upgrade-forbiddenapis-33759

Conversation

@alpar-t
Copy link
Copy Markdown
Contributor

@alpar-t alpar-t commented Sep 18, 2018

  • version bump
  • 11 is now supported, so no need to restrict it to 10
  • adjust to output change in forbiddenapis

Closes #33759 ( w.r.t. the version upgrade, but the discussion there is welcome to continue )

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 18, 2018

Pinging @elastic/es-core-infra

uschindler
uschindler reviewed Sep 18, 2018
View reviewed changes
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ThirdPartyAuditTask.java

private static final Pattern MISSING_CLASS_PATTERN = Pattern.compile(
"WARNING: The referenced class '(.*)' cannot be loaded\\. Please fix the classpath!"
"WARNING: Class '(.*)' cannot be loaded \\(.*\\)\\. Please fix the classpath!"
Copy link
Copy Markdown
Contributor

@uschindler uschindler Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. Yes this message was changed due to the cleanup of missing class reporting.

Copy link
Copy Markdown
Contributor Author

@alpar-t alpar-t Sep 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... a much appreciated change. Thanks for that !

@alpar-t alpar-t mentioned this pull request Oct 1, 2018
Closed
@alpar-t
Copy link
Copy Markdown
Contributor Author

alpar-t commented Oct 2, 2018

The failed CI job is due to the fact that we still run Gradle with java 8 there, and run into class not found errors when scanning the java 9 classes which have references to classes introduced in 9.
This should not be a problem once #34179 is merged. Will have to merge this PR after that.

@uschindler
Copy link
Copy Markdown
Contributor

uschindler commented Oct 2, 2018

I was just wondering. Because a while back you changed the minimum version to build Elasticsearch to Java 9 (or even later), but used the "-release" to compile the Java 8 classes.

@uschindler
Copy link
Copy Markdown
Contributor

uschindler commented Oct 2, 2018

...BTW this was also a reason why I wondered that you fork anywhere in Elasticsearch's build. Since Java 9 it's perfectly possible to build 100% Java 8 compatible artifacts, as the "release" switch allows to actually compile against the older class signatures (real cross-compiling). IMHO, no forking is needed anymore, except for tests.

@alpar-t
Copy link
Copy Markdown
Contributor Author

alpar-t commented Oct 2, 2018

Yes, that is correct, we will only be working for tests.
Third party audit still forks and runs the CLI version but only because it needs to look at the output.
We could get rid of this if we had callbacks like onMissingClass and onViolation callbacks in the Gradle plugin. The java version we use to fork doesn't matter much, we make it the same as the target version only because it makes it easier to discover if something didn't go right, e.x. a java 9 specific class file was incorrectly extracted when targeting 8, we would get a class not found error, whereas it would silently continue if we were to run it with a never java.

@alpar-t
Copy link
Copy Markdown
Contributor Author

alpar-t commented Oct 2, 2018

I was just wondering. Because a while back you changed the minimum version to build Elasticsearch to Java 9 (or even later), but used the "-release" to compile the Java 8 classes.

AFAIK we don't fully enforce that and CI was still running Gradle with java 8 using -Dorg.gradle.java.home. I'm in the process of cleaning that up.

@alpar-t
Copy link
Copy Markdown
Contributor Author

alpar-t commented Oct 18, 2018

@rjernst this is ready for review now and passes CI

rjernst
rjernst approved these changes Oct 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alpar-t alpar-t merged commit 0536635 into elastic:master Oct 23, 2018
@alpar-t alpar-t deleted the upgrade-forbiddenapis-33759 branch October 23, 2018 09:13
alpar-t added a commit that referenced this pull request Oct 23, 2018
@alpar-t
Upgrade forbiddenapis to 2.6 (#33809)
3f3d2fb 
* Upgrade forbiddenapis to 2.6

Closes #33759

* Switch forbiddenApis back to official plugin

* Remove CLI based task

* Fix forbiddenApisJava9
kcm pushed a commit that referenced this pull request Oct 30, 2018
@alpar-t @kcm
Upgrade forbiddenapis to 2.6 (#33809)
c0d7a06 
* Upgrade forbiddenapis to 2.6

Closes #33759

* Switch forbiddenApis back to official plugin

* Remove CLI based task

* Fix forbiddenApisJava9
@not-napoleon not-napoleon mentioned this pull request Dec 27, 2018
Closed
@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

Reviewers

@rjernst rjernst rjernst approved these changes

+1 more reviewer

@uschindler uschindler uschindler left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team >upgrade v6.5.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update forbiddenapis to v2.6

6 participants

@alpar-t @elasticmachine @uschindler @rjernst @colings86 @mark-vieira