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

Ignore module-info in jar hell checks #33011

Merged
merged 8 commits into from
Aug 30, 2018
Merged

Ignore module-info in jar hell checks #33011

merged 8 commits into from
Aug 30, 2018

Conversation

alpar-t
Copy link
Contributor

@alpar-t alpar-t commented Aug 21, 2018

JarHell should ignore module-info classes in any package so it doesn't clash with jigsaw.
The PR also adds a test around the jarHell Gradle task to assert that module-info is ignored, but jar hell is otherwise detected.

Closes #32985

@alpar-t alpar-t added :Core/Infra/Core Core issues without another label v7.0.0 v6.5.0 labels Aug 21, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@alpar-t alpar-t changed the title Jarhell module info 32985 Ignore module-info in jar hell cehsk Aug 21, 2018
@alpar-t alpar-t changed the title Ignore module-info in jar hell cehsk Ignore module-info in jar hell checks Aug 21, 2018
jasontedor
jasontedor reviewed Aug 21, 2018
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

The rough idea looks good to me but I prefer that @rjernst give the thorough review here.

rjernst
rjernst reviewed Aug 27, 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.

This looks fine. A couple quick questions.

buildSrc/src/test/java/org/elasticsearch/gradle/precommit/JarHellTaskIT.java Outdated
*/
public class JarHellTaskIT extends GradleIntegrationTestCase {

public void testDoesNotFailOnModuleInfo() {
Copy link
Member

Choose a reason for hiding this comment

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

can't this be in the existing JarHellTests unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it there and changed the integration test on the build side to test only that it's correctly integrated into the build and run as part of precommit. It's no longer strictly related with the issues this PR solves so let me know if you would want me to split it.

buildSrc/build.gradle Outdated
@@ -167,7 +167,13 @@ if (project != rootProject) {
it.tasks.matching { it.name == 'publishNebulaPublicationToLocalTestRepository'}
}
exclude "**/*Tests.class"
include "**/*IT.class"
// We need a custom solution so it doesn't interfere with the test task
if ( System.hasProperty("integTest.single") ) {
Copy link
Member

Choose a reason for hiding this comment

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

This change doesn't seem related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it into it's own PR. I realized it was needed while testing the new integration test - there's no way to run a single one.

rjernst
rjernst reviewed Aug 29, 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.

Thanks @atorok. LGTM, just one last question.

libs/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java
@@ -255,6 +255,10 @@ public static void checkJavaVersion(String resource, String targetVersion) {
}

private static void checkClass(Map<String, Path> clazzes, String clazz, Path jarpath) {
if (clazz.equals("module-info") || clazz.endsWith(".module-info")) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are there 2 variants? Shouldn't the class name always be module-info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, but it can live in a package and users might put it in the same package e.x. org.elasticsearch.module-info

@alpar-t alpar-t merged commit 5cf6e0d into elastic:master Aug 30, 2018
@alpar-t alpar-t deleted the jarhell-module-info-32985 branch August 30, 2018 08:41
alpar-t added a commit that referenced this pull request Aug 30, 2018
@alpar-t
Ignore module-info in jar hell checks (#33011)
6a699ad 
* Ignore module-info in JarHell checks
* Add unit test
* integration test to test that jarhell is ran with precommit
dnhatn added a commit that referenced this pull request Sep 1, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
a6312a5 
* 6.x:
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  TEST: Disable soft-deletes in ParentChildTestCase
  TEST: Disable randomized soft-deletes settings
  Integrates soft-deletes into Elasticsearch (#33222)
  drop `index.shard.check_on_startup: fix` (#32279)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  [DOCS] Moves ml folder from x-pack/docs to docs (#33248)
  TEST: mute more SmokeTestWatcherWithSecurityIT tests
  [DOCS] Move rollup APIs to docs (#31450)
  [DOCS] Rename X-Pack Commands section (#33005)
  Fixes SecurityIntegTestCase so it always adds at least one alias (#33296)
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307)
  MINOR: Remove Dead Code from PathTrie (#33280) (#33306)
  Fix pom for build-tools (#33300)
  Lazy evaluate java9home (#33301)
  SQL: test coverage for JdbcResultSet (#32813)
  Work around to be able to generate eclipse projects (#33295)
  Different handling for security specific errors in the CLI. Fix for #33230 (#33255)
  [ML] Refactor delimited file structure detection (#33233)
  SQL: Support multi-index format as table identifier (#33278)
  Enable forbiddenapis server java9 (#33245)
  [MUTE] SmokeTestWatcherWithSecurityIT flaky tests
  Add region ISO code to GeoIP Ingest plugin (#31669) (#33276)
  Don't be strict for 6.x
  Update serialization versions for custom IndexMetaData backport
  Replace IndexMetaData.Custom with Map-based custom metadata (#32749)
  Painless: Fix Bindings Bug (#33274)
  SQL: prevent duplicate generation for repeated aggs (#33252)
  TEST: Mute testMonitorClusterHealth
  Fix serialization of empty field capabilities response (#33263)
  Fix nested _source retrieval with includes/excludes (#33180)
  [DOCS] TLS file resources are reloadable (#33258)
  Watcher: Ensure TriggerEngine start replaces existing watches (#33157)
  Ignore module-info in jar hell checks (#33011)
  Fix docs build after #33241
  [DOC] Repository GCS ADC not supported (#33238)
  Upgrade to latest Gradle 4.10  (#32801)
  Fix/30904 cluster formation part2 (#32877)
  Move file-based discovery to core (#33241)
  HLRC: add client side RefreshPolicy (#33209)
  [Kerberos] Add unsupported languages for tests (#33253)
  Watcher: Reload properly on remote shard change (#33167)
  Fix classpath security checks for external tests. (#33066)
  [Rollup] Only allow aggregating on multiples of configured interval (#32052)
  Added deprecation warning for rescore in scroll queries (#33070)
  Apply settings filter to get cluster settings API (#33247)
  [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability   (#32743)
  HLRC: create base timed request class (#33216)
  HLRC: Use Optional in validation logic (#33104)
  Painless: Add Bindings (#33042)
@lonvia lonvia mentioned this pull request Feb 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst left review comments

@jasontedor jasontedor jasontedor left review comments

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue v6.5.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alpar-t @elasticmachine @rjernst @jasontedor @colings86