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.

Sign up for GitHub

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

Jump to bottom

Upgrade forbiddenapis to 2.6 #33809

Merged
merged 5 commits into from
Oct 23, 2018
Merged

Upgrade forbiddenapis to 2.6 #33809

merged 5 commits into from
Oct 23, 2018

Conversation

alpar-t
Copy link
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
Collaborator

Pinging @elastic/es-core-infra

uschindler
uschindler reviewed Sep 18, 2018
View reviewed changes
buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ThirdPartyAuditTask.java
@@ -51,7 +51,7 @@
public class ThirdPartyAuditTask extends DefaultTask {

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
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
Contributor Author

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
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
Contributor

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
Contributor

...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
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
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
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
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

@uschindler uschindler uschindler left review comments

@rjernst rjernst rjernst approved these changes

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