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

Configurable java executable #750

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

wfhartford
Copy link
Contributor

This feature was discussed in #742. The change allows a user to specify the path to a Java executable in the ProtobufExtension or the GenerateProtoTask, or both, with the task overriding the extension. If the user does not specify any task, the existing behaviour (using the java running gradle) is unchanged.

Since gradle 6.7 there is a much cleaner solution using the configured Java toolchain (see https://docs.gradle.org/current/userguide/toolchains.html#sec:plugins), I did not investigate this option since the plugin currently supports gradle versions back to 5.6.

The path of the java executable can now be configured in the ProtobufExtension and/or specific GenerateProtoTask instances.

* GenerateProtoTask gains the javaExecutablePath Property,
* ProtobufExtension gains the javaExecutablePath Property and the defaultJavaExecutablePath provider, which provides the default path using the same logic as previous versions
* computeJavaExePath moved from GenerateProtoTask to ProtobufExtension since it is now only used in ProtobufExtension
* isWindows moved from GenerateProtoTask to Util since it is now used in GenerateProtoTask and ProtobufExtension
@ejona86 ejona86 requested review from YifeiZhuang and rougsig March 14, 2024 17:30
@wfhartford
Copy link
Contributor Author

I think there is a problem with my test when run on Windows, I'm waiting for some feedback from a colleague, but will probably have to add another commit to this PR.

@wfhartford
Copy link
Contributor Author

Is it true that most of the tests will not pass on windows? My colleague reported that a lot more tests failed than I expected.

@ejona86
Copy link
Collaborator

ejona86 commented Mar 14, 2024

I can't say the tests have ever been run on Windows. So, yeah, they could be pretty broken.

@ejona86 ejona86 merged commit 12ad318 into google:master Mar 14, 2024
11 checks passed
@ejona86
Copy link
Collaborator

ejona86 commented Mar 14, 2024

Thank you!

@wfhartford
Copy link
Contributor Author

Thanks folks

Copy link

@Boombitchz Boombitchz left a comment

Choose a reason for hiding this comment

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

Test

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.

4 participants