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

LUCENE-9990: gradle7 support #175

Closed
wants to merge 11 commits into from
Closed

LUCENE-9990: gradle7 support #175

wants to merge 11 commits into from

Conversation

dweiss
Copy link
Contributor

@dweiss dweiss commented Jun 7, 2021

No description provided.

}
}

tasks.withType( JavaCompile ).configureEach {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add the issue reference you mentioned here. The entire workaround thing should be best moved under a separate file in gradle/hacks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding and reviewing the patch!

Sure. gradle/hacks makes sense. I'll also follow up on the ticket to see if there is a better way to solve this.

}
}
}
//JavaInstallationRegistry registry = extensions.getByType(JavaInstallationRegistry)
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 understand it's a PoC but a plan is needed how to enable what worked previously - pointing at an arbitrary JVM home and have it be the default for compilation, tests, etc. In the current form the patch removes things that are currently used to separate the gradle vs. lucene JVM toolchains - we do need this separation (for example to rule out early access JVM problems stemming from gradle itself vs. lucene).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.
Gradle 7 has something similar to what we have already setup today i.e., using an env variable to send in the location of the JVM. I can get it running and then add instructions in lucene/help/jvms.txt, cleanup the comments in this file as well!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear: I don't know how it works exactly but what we want is not one jvm to run gradle and lucene - we want the ability to separate these.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood :) The gradle website says this (under improvements in Gradle 7):

Additionally, you may want to build a project using a Java version that is not supported for running Gradle.

so I am optimistic that we should be able to run Gradle and Lucene on different Java versions.

Sorry for the delay in the response! I will try my best to find some time soon and get this running

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries at all. I think Palantir's plugin is the showstopper here too.

gradle/wrapper/gradle-wrapper.jar.version Outdated Show resolved Hide resolved
@gautamworah96
Copy link
Contributor

I gave this PR another shot (since the Palantir plugin has been patched in v2.0.0 for Gradle 7 support), but had some new issues come up. The good news, I think that using the -Porg.gradle.java.installations.paths command line param points Gradle to use that specific JDK for building and running the project. The bad news, since JavaInstallationRegistry is now deprecated, the build fails in multiple places (some that use the Java version to add specific JVM params, and others where we use the plugin to get the Java command to generate some javadoc). As of right now, I am just trial-and-erroring some code to see what works.

Some WIP code is pushed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants