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

Use the ToolProvider SPI to run google-java-format #1795

Closed
wants to merge 1 commit into from

Conversation

cushon
Copy link
Contributor

@cushon cushon commented Nov 6, 2024

This makes the compile-time dependency on google-java-format optional.

This makes the compile-time dependency on google-java-format optional.
@CLAassistant
Copy link

CLAassistant commented Nov 6, 2024

CLA assistant check
All committers have signed the CLA.

@ben-manes
Copy link
Owner

thanks! Is there a strong reason to prefer the SPI or is it just a nicer practice?

I can polish to pass the CI and switch to a runtime dependency to merge this. I'm not sure why it recompiled in each job instead of using the build cache, so sorry for that warning spam.

ben-manes pushed a commit that referenced this pull request Nov 7, 2024
This makes the compile-time dependency on google-java-format optional.
@ben-manes ben-manes closed this Nov 7, 2024
@cushon
Copy link
Contributor Author

cushon commented Nov 7, 2024

thanks! Is there a strong reason to prefer the SPI or is it just a nicer practice?

I can polish to pass the CI and switch to a runtime dependency to merge this. I'm not sure why it recompiled in each job instead of using the build cache, so sorry for that warning spam.

Thanks for the lightning fast review!

The motivation for this was an internal bazel build for google-java-format. The formatter's tests depend on caffeine, which depends on google-java-format, so I was having to rebuild caffeine when testing some formatter changes. So not a common use-case, but I also didn't see downsides to using the SPI here. Thanks again!

@ben-manes
Copy link
Owner

So not a common use-case...

haha, well I know that fun. Caffeine's build depends on errorprone which depends on caffeine, and Gradle rewrites that to a project dependency so it relies on the pre-built classes. I have to trick it to include the jar.

@ben-manes
Copy link
Owner

The formatter's tests depend on caffeine, which depends on google-java-format

oh can you explain this? The published metadata, pom and module, don’t include that as a dependency. It should only be a build phase in this repository and not in a user’s, so it sounds like the thirdparty java_import rule might be excessive?

@cushon
Copy link
Contributor Author

cushon commented Nov 8, 2024

The formatter's tests depend on caffeine, which depends on google-java-format

oh can you explain this? The published metadata, pom and module, don’t include that as a dependency. It should only be a build phase in this repository and not in a user’s, so it sounds like the thirdparty java_import rule might be excessive?

The key part here was that this is an internal bazel build, which is building everything from source and not relying on the published artifacts. So it's probably really not a common use-case, but it's helpful being able to drop the build-time dep for that.

@ben-manes
Copy link
Owner

great, I figured something like that and likely for reproducible builds to protect against code injection attacks. Thanks for following up.

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.

3 participants