-
Notifications
You must be signed in to change notification settings - Fork 1
Introduce FailableProvider for better error handling in GradleExec
#435
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
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. 🔄 Changelog entries were re-generated at Fri, 29 Aug 2025 09:03:09 UTC! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces FailableProvider<T> to improve error handling in GradleExec by automatically throwing descriptive exceptions on non-zero exit codes while providing flexible error handling options.
Key changes:
- Introduces
FailableProviderinterface andDefaultFailableProviderimplementation for failure-aware providers - Changes
GradleExec.exec()return type fromProvider<ExecResultWithOutput>toFailableProvider<ExecResultWithOutput> - Updates existing usage in
GradleOperatingSystemto use the new error handling capabilities
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
providers/src/main/java/com/palantir/gradle/utils/providers/FailableProvider.java |
Defines the interface for failure-aware providers with methods like mapFailure() and fold() |
providers/src/main/java/com/palantir/gradle/utils/providers/DefaultFailableProvider.java |
Implements the FailableProvider interface with failure detection and custom error handling |
exec/src/main/java/com/palantir/gradle/utils/exec/GradleExec.java |
Updates exec() method to return FailableProvider with automatic failure detection |
exec/src/main/java/com/palantir/gradle/utils/exec/ExecResultWithOutput.java |
Extracts ExecResultWithOutput interface into separate file |
exec/src/main/java/com/palantir/gradle/utils/exec/ExecFailedException.java |
Adds specialized exception for process execution failures |
platform/src/main/java/com/palantir/platform/GradleOperatingSystem.java |
Updates to use fold() method for handling both success and failure cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
providers/src/main/java/com/palantir/gradle/utils/providers/DefaultFailableProvider.java
Show resolved
Hide resolved
providers/src/main/java/com/palantir/gradle/utils/providers/DefaultFailableProvider.java
Outdated
Show resolved
Hide resolved
providers/src/main/java/com/palantir/gradle/utils/providers/DefaultFailableProvider.java
Outdated
Show resolved
Hide resolved
kelvinou01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool idea and clean implementation! Here are some initial thoughts before lunchtime, but I'm still simmering on this.
| /** | ||
| * Exception thrown when a process execution fails with a non-zero exit code. | ||
| */ | ||
| public class ExecFailedException extends RuntimeException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make ExecFailedException a checked exception, so that we force people to wrap it in SafeRuntimeException or UnsafeRuntimeException with their own context?
Not sure if the context added is worth the extra code it requires. But since this is a library, it might be a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn on this, I think FallibleProvider must use RuntimeException because it extends Gradle's Provider<T> interface, which doesn't declare checked exceptions on get(), so we would need to wrap ourselves in GradleExec. but it would be nice to force / encourage users to provider there own context
| * Executes a process and returns a FailableProvider for the result. | ||
| * <p> | ||
| * This method always sets {@code ignoreExitValue} to {@code true} on the {@code ExecSpec}, | ||
| * ensuring that the build does not fail regardless of the process exit code. Callers do need | ||
| * to handle exit codes manually. | ||
| * <p> | ||
| * The result includes the process's standard output, standard error, and the {@link ExecResult}. | ||
| * Usage: | ||
| * <pre> | ||
| * def result = gradleExec.exec { | ||
| * commandLine 'git', 'status' | ||
| * } | ||
| * | ||
| * // Handle success/failure | ||
| * result.handle( | ||
| * { output -> println "Success: ${output.stdOut}" }, | ||
| * { output -> println "Failed: ${output.stdErr}" } | ||
| * ) | ||
| * | ||
| * @param action an action to configure the {@link ExecSpec} for the process to be executed | ||
| * @return a Provider of {@link ExecResultWithOutput} containing the standard output, standard error, | ||
| * and execution result | ||
| * // Or throw on failure | ||
| * def output = result.get().stdOut | ||
| * </pre> | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explicate the tribal knowledge that GradleExec is needed, and people should use GradleExec, because ProviderFactory::exec doesn't give good stack traces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe even errorprone away usages of ProviderFactory::exec to GradleExec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah think an errorprone is great idea - I've updated the javadoc to be more explicit as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added an errorprone see - palantir/gradle-guide#210
exec/src/main/java/com/palantir/gradle/utils/exec/ExecResultWithOutput.java
Outdated
Show resolved
Hide resolved
providers/src/main/java/com/palantir/gradle/utils/providers/FailableProvider.java
Outdated
Show resolved
Hide resolved
exec/src/main/java/com/palantir/gradle/utils/exec/GradleExec.java
Outdated
Show resolved
Hide resolved
FailableProvider for better error handling in GradleExec
Before this PR
GradleExec.exec()returned a rawProvider<ExecResultWithOutput>that required manual exit code checking and error handling, leading to boilerplate code and inconsistent error messages.After this PR
GradleExec.exec()returns aFailableProvider<ExecResultWithOutput>that:ExecFailedExceptionon non-zero exit codes by default.mapFailure()and.handle().getOrElse()and.getOrNull()==COMMIT_MSG==
Introduce
FailableProviderfor better error handling in GradleExecGradleExec.exec()now returns aFailableProviderthat automatically throwsdescriptive exceptions on non-zero exit codes while supporting custom error
handling through
mapFailure()andfold()methods.==COMMIT_MSG==
Possible downsides?
Breaking change: Return type changed from
Provider<ExecResultWithOutput>toFailableProvider<ExecResultWithOutput>. WhileFailableProviderextendsProvider,.get()now throws on non-zero exit codes instead of returning silently. Use.getRaw()for old behavior.