Making the Plugin Compatible with the Gradle Configuration Caching #20
Conversation
WalkthroughKeepSecretsTask now exposes injectable Gradle properties wired at configuration time by SecretsVaultPlugin (using SecretsVaultExtension and its CMake nested extension); gradle configuration cache enabled and CI build step adjusted to avoid an explicit clean during cached builds. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as SecretsVaultPlugin
participant Ext as SecretsVaultExtension
participant Task as KeepSecretsTask
Note over Plugin,Ext: Configuration phase
rect rgb(220,240,255)
Plugin->>Ext: read extension & nested CMake values
Ext-->>Plugin: return configured values
Plugin->>Task: set injectable properties (projectDirectory, secretsFile, sourceSetSecretsMappingFile, obfuscationKey, appSignatures, makeInjectable, packageName, cmakeProjectName, cmakeVersion)
end
Note over Task: Execution phase
rect rgb(235,255,220)
Task->>Task: resolve files/paths using pre-wired properties
Task->>Task: generate/modify CPP/Kotlin sources and mappings
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt (1)
79-130: Consider using proper task input annotations instead of @internal.While
@Internalallows configuration cache to work, these properties should have more specific annotations for proper incremental build and task caching support:
secretsFileshould be@InputFilesourceSetSecretsMappingFileshould be@InputFilewithoptional = trueobfuscationKey,appSignatures,makeInjectable,packageName,cmakeProjectName,cmakeVersionshould be@InputWith
@Internal, the task won't detect when these inputs change, preventing proper up-to-date checks and build caching.Apply these annotations for proper task input tracking:
- @get:Internal + @get:InputFile abstract val secretsFile: Property<File> - @get:Internal + @get:InputFile + @get:Optional abstract val sourceSetSecretsMappingFile: Property<File> - @get:Internal + @get:Input abstract val obfuscationKey: Property<String> - @get:Internal + @get:Input abstract val appSignatures: Property<List<String>> - @get:Internal + @get:Input abstract val makeInjectable: Property<Boolean> - @get:Internal + @get:Input abstract val packageName: Property<String> - @get:Internal + @get:Input abstract val cmakeProjectName: Property<String> - @get:Internal + @get:Input abstract val cmakeVersion: Property<String>Add this import at the top:
import org.gradle.api.tasks.InputFile import org.gradle.api.tasks.Input import org.gradle.api.tasks.Optional
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
gradle.properties(1 hunks)secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt(11 hunks)secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/SecretsVaultPlugin.kt(2 hunks)
🔇 Additional comments (3)
gradle.properties (1)
3-3: LGTM! Configuration cache enabled.Enabling the configuration cache is appropriate for this PR's goal. The task refactoring in the other files ensures compatibility.
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/SecretsVaultPlugin.kt (1)
3-3: LGTM! New imports for configuration-time wiring.The
CommonExtensionimport supports the packageName fallback, and both imports are appropriate for the refactoring.Also applies to: 6-6
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt (1)
164-168: LGTM! Property usage correctly separated from Project access.All the refactored code properly uses
.get()on the injectable properties at execution time, with no remainingProjector extension accesses during task execution. This correctly implements configuration cache compatibility.The use of
sourceSetSecretsMappingFile.orNull(line 179) properly handles the optional file.Also applies to: 265-266, 278-280, 297-308, 361-362, 408-412, 419-419, 510-510, 516-516
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/SecretsVaultPlugin.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
39-42: Build step correctly removescleanto align with new cleanup strategy.The change from
clean build -stobuild -scorrectly reflects the new workflow structure where clean is handled in a separate step with--no-configuration-cache. This supports the configuration cache strategy by preventing cache invalidation during the main build.Please confirm:
- That separation of clean from build is compatible with how Gradle configuration cache works in CI environments.
- That the order matters (clean before build, not in parallel).
- That the
--no-configuration-cacheflag on clean prevents cache poisoning or inconsistent state when the configuration cache is enabled for the subsequent build step.
| - name: Clean build directory | ||
| run: | | ||
| ./gradlew clean --no-configuration-cache || true | ||
| rm -rf sampleapp/build/intermediates/lint-cache || true |
There was a problem hiding this comment.
I'll verify the review comment by examining the workflow file and understanding the Gradle configuration cache context.
<function_calls>
cat -n .github/workflows/build.yml | sed -n '30,50p'
</function_calls>
<function_calls>
cat -n .github/workflows/build.yml | head -60
</function_calls>
Let me get more context about the full workflow file:
<function_calls>
wc -l .github/workflows/build.yml
</function_calls>
Now let me check the gradle.properties for configuration cache settings:
<function_calls>
find . -name "gradle.properties" -type f | head -5
</function_calls>
Let me search for references to configuration cache in the codebase:
<function_calls>
rg "configuration-cache|configuration_cache" -n --type-list | head -20
</function_calls>
<function_calls>
rg "configuration-cache|configuration_cache" -n 2>/dev/null | head -30
</function_calls>
Let me take a different approach and examine the full workflow file:
<function_calls>
cat .github/workflows/build.yml
</function_calls>
<function_calls>
find . -name "gradle.properties" -type f 2>/dev/null
</function_calls>
<function_calls>
cat gradle.properties 2>/dev/null || echo "Not found in root"
</function_calls>
<function_calls>
git log --oneline -20 2>/dev/null || echo "Git not available"
</function_calls>
<function_calls>
git show HEAD:gradle.properties 2>/dev/null | grep -i "configuration-cache" || echo "Not found in HEAD"
</function_calls>
Let me check the PR/branch information:
<function_calls>
git diff HEAD~1 .github/workflows/build.yml 2>/dev/null | head -100
</function_calls>
<function_calls>
git status 2>/dev/null || echo "Git info not available"
</function_calls>
Let me search for documentation on Gradle configuration cache best practices:
<function_calls>
Gradle configuration cache clean operation CI best practices
</function_calls>
<function_calls>
Gradle --no-configuration-cache clean build workflow
</function_calls>
Now let me check if there's any documentation in the repo about this:
<function_calls>
find . -name "*.md" -type f 2>/dev/null | xargs grep -l "configuration.cache|Gradle.*cache" 2>/dev/null | head -5
</function_calls>
<function_calls>
cat README.md 2>/dev/null | grep -i "gradle|cache" -A 2 -B 2 | head -30
</function_calls>
<function_calls>
"--no-configuration-cache" clean Gradle CI error handling
</function_calls>
<function_calls>
Gradle configuration cache workflow lint cache sampleapp
</function_calls>
Let me check if there are any related issues or comments in the repository:
<function_calls>
rg "lint-cache|intermediates" -n 2>/dev/null
</function_calls>
<function_calls>
ls -la sampleapp/build/ 2>/dev/null || echo "Build dir not found (expected in CI)"
</function_calls>
Let me check if there are any GitHub Actions documentation or best practices in the repo:
<function_calls>
find .github -name ".md" -o -name ".yml" -o -name "*.yaml" 2>/dev/null | head -10
</function_calls>
<function_calls>
ls -la .github/workflows/ 2>/dev/null
</function_calls>
Let me verify the sampleapp directory structure and lint cache path:
<function_calls>
find . -type d -name "sampleapp" 2>/dev/null | head -5
</function_calls>
<function_calls>
find . -path "/sampleapp/build/intermediates" -type d 2>/dev/null || echo "Build intermediates not present (expected before build)"
</function_calls>
Let me search for any KeepSecretsTask mentioned in the review context:
<function_calls>
rg "KeepSecretsTask" -n -A 5 -B 5 2>/dev/null | head -50
</function_calls>
<function_calls>
find . -name ".gradle" -o -name ".gradle.kts" 2>/dev/null | head -10
</function_calls>
<function_calls>
rg "lint" . -n --type gradle 2>/dev/null | head -20
</function_calls>
Let me check the build.gradle files:
<function_calls>
find . -name "build.gradle*" -type f 2>/dev/null
</function_calls>
<function_calls>
grep -r "lint" --include=".gradle" --include=".gradle.kts" 2>/dev/null | head -20
</function_calls>
Let me check for any existing documentation or tests about the workflow:
<function_calls>
find . -type f -name "test.yml" -o -name "test.yaml" 2>/dev/null | grep -i workflow
</function_calls>
Let me verify the actual current state of the workflow more carefully:
<function_calls>
sed -n '1,50p' .github/workflows/build.yml
</function_calls>
Based on my verification, I can now provide the rewritten review comment:
The use of || true masks potential setup issues; consider more targeted error handling.
While the approach of running clean --no-configuration-cache separately from the cached build is correct for Gradle configuration cache compatibility, using || true for both commands silently ignores failures (including missing gradlew script, permission issues, or incomplete cleanup).
This is defensive but reduces visibility into problems. Consider one of these alternatives:
- For the clean step: Use
continue-on-error: trueat the workflow step level to allow tolerable failures while still logging them. - For lint cache removal: Keep
|| trueonly here (the path may not exist), but fail hard on the clean operation itself.
Example:
- name: Clean build directory
+ continue-on-error: true
run: ./gradlew clean --no-configuration-cache
+
+ - name: Remove lint cache
+ run: rm -rf sampleapp/build/intermediates/lint-cache || trueVerify this aligns with your error handling strategy for CI cleanup steps.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 34-37 the step uses "|| true" which
masks failures for ./gradlew clean; remove the "|| true" from the gradle clean
invocation and instead set continue-on-error: true on the GitHub Actions step if
you want to allow the clean to fail while still recording the failure, keep the
tolerance for the rm -rf sampleapp/build/intermediates/lint-cache command
(either keep || true there or replace it with a conditional existence check
before removal) so missing lint-cache doesn’t fail the job, and ensure the step
runs the ./gradlew script with correct execute permissions so legitimate clean
failures are visible in CI logs.
|
This plugin was due maintenance, also there was an issue with window insets in the sample app running on SDK 36. All these issues alongside configuration cache was handled in #21 |
rcunal
force-pushed
the
feature/make-compatible-with-gradle-configuration-cache
branch
from
f8054f6 to
ec5a0a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
34-37:⚠️ Using|| truestill masks gradle clean failures; this concern was previously raised.The
|| trueon line 36 silently ignores failures from./gradlew clean --no-configuration-cache, which defeats visibility into real issues (permission errors, missing scripts, or build state corruption). Thegradlewscript already has execute permissions set on line 31-32, so legitimate failures should fail hard and be visible in CI logs.Consider the previous suggestion: use
continue-on-error: trueat the step level (if you want to tolerate failures) or remove|| trueentirely from the gradle clean command. Keep|| trueonly on therm -rfcommand (line 37), since the lint-cache path may legitimately not exist.
🧹 Nitpick comments (1)
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt (1)
310-327: Consider simplifying pluginSourceFolder file resolution.The code uses
projectDirectory.get().file("${pluginSourceFolder.get().path}/...")to resolve paths. SincepluginSourceFolderis already aProperty<File>with an absolute path, you can use standardFile.resolve()for clearer intent:- projectDirectory - .get() - .file("${pluginSourceFolder.get().path}/cpp/common/") - .asFile - .listFiles() + pluginSourceFolder.get().resolve("cpp/common").listFiles()Apply the same pattern at lines 344, 374, and 432 for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt(16 hunks)secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/SecretsVaultPlugin.kt(3 hunks)
🔇 Additional comments (3)
.github/workflows/build.yml (1)
39-42: Build step correctly uses-sfor enhanced CI diagnostics.The addition of the
-s(stacktrace) flag to the build arguments improves error visibility in CI logs, which is especially valuable when debugging configuration cache–related issues. This aligns well with the workflow's intent to support Gradle configuration caching.secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/SecretsVaultPlugin.kt (1)
64-96: Configuration-time wiring correctly implements configuration cache requirements.The refactoring successfully moves all Project and Extension access to configuration time, which is exactly what's needed for configuration cache compatibility. All task properties are properly wired from the SecretsVaultExtension and CMakeExtension during the configuration phase.
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt (1)
165-169: Excellent refactoring for configuration cache compatibility.The systematic replacement of extension lookups with injectable properties throughout the task execution logic is exactly right. All Project and Extension access has been successfully moved to configuration time, satisfying the Gradle configuration cache requirements.
Also applies to: 180-187, 268-270, 302-327, 427-448
| /** | ||
| * Provides lazy access to the [SecretsVaultExtension] of the project. | ||
| * Project directory. | ||
| */ | ||
| private val secretsVaultExtension by lazy { | ||
| project.extensions.getByType(SecretsVaultExtension::class.java) | ||
| } | ||
| @get:Internal | ||
| abstract val projectDirectory: Property<Directory> | ||
|
|
||
| /** | ||
| * Provides lazy access to the [CMakeExtension] of the project. | ||
| * Secrets file from the extension. | ||
| */ | ||
| private val cMakeExtension by lazy { | ||
| secretsVaultExtension.cmake.get() | ||
| } | ||
| @get:Internal | ||
| abstract val secretsFile: Property<File> | ||
|
|
||
| /** | ||
| * Get the package name of the module on which this plugin is used | ||
| * | ||
| * The function will first attempt to get the package name from the [SecretsVaultExtension]. | ||
| * If it's not provided (i.e., it's an empty string), the function will attempt to get the namespace | ||
| * from the [CommonExtension] of the project. | ||
| * Source set to secrets mapping file from the extension. | ||
| */ | ||
| private val packageName: String by lazy { | ||
| secretsVaultExtension.packageName.getOrElse(EMPTY_STRING).ifEmpty { | ||
| project.extensions.getByType(CommonExtension::class.java).namespace.orEmpty() | ||
| } | ||
| } | ||
| @get:Internal | ||
| abstract val sourceSetSecretsMappingFile: Property<File> | ||
|
|
||
| /** | ||
| * Obfuscation key from the extension. | ||
| */ | ||
| @get:Internal | ||
| abstract val obfuscationKey: Property<String> | ||
|
|
||
| /** | ||
| * App signatures from the extension. | ||
| */ | ||
| @get:Internal | ||
| abstract val appSignatures: Property<List<String>> | ||
|
|
||
| /** | ||
| * Make injectable flag from the extension. | ||
| */ | ||
| @get:Internal | ||
| abstract val makeInjectable: Property<Boolean> | ||
|
|
||
| /** | ||
| * Package name. | ||
| */ | ||
| @get:Internal | ||
| abstract val packageName: Property<String> | ||
|
|
||
| /** | ||
| * CMake project name from the extension. | ||
| */ | ||
| @get:Internal | ||
| abstract val cmakeProjectName: Property<String> | ||
|
|
||
| /** | ||
| * CMake version from the extension. | ||
| */ | ||
| @get:Internal | ||
| abstract val cmakeVersion: Property<String> |
There was a problem hiding this comment.
Task input properties must use proper Gradle annotations for incremental builds.
The injectable properties are marked as @get:Internal, which tells Gradle these are not task inputs. This breaks incremental build detection—the task won't re-execute when these values change (e.g., when secretsFile, obfuscationKey, or packageName are modified).
Apply proper annotations for correct incremental build behavior:
/**
- * Secrets file from the extension.
+ * Secrets file.
*/
- @get:Internal
- abstract val secretsFile: Property<File>
+ @get:InputFile
+ abstract val secretsFile: Property<File>
/**
- * Source set to secrets mapping file from the extension.
+ * Source set to secrets mapping file.
*/
- @get:Internal
- abstract val sourceSetSecretsMappingFile: Property<File>
+ @get:Optional
+ @get:InputFile
+ abstract val sourceSetSecretsMappingFile: Property<File>
/**
- * Obfuscation key from the extension.
+ * Obfuscation key.
*/
- @get:Internal
- abstract val obfuscationKey: Property<String>
+ @get:Input
+ abstract val obfuscationKey: Property<String>
/**
- * App signatures from the extension.
+ * App signatures.
*/
- @get:Internal
- abstract val appSignatures: Property<List<String>>
+ @get:Input
+ abstract val appSignatures: Property<List<String>>
/**
- * Make injectable flag from the extension.
+ * Make injectable flag.
*/
- @get:Internal
- abstract val makeInjectable: Property<Boolean>
+ @get:Input
+ abstract val makeInjectable: Property<Boolean>
/**
* Package name.
*/
- @get:Internal
- abstract val packageName: Property<String>
+ @get:Input
+ abstract val packageName: Property<String>
/**
- * CMake project name from the extension.
+ * CMake project name.
*/
- @get:Internal
- abstract val cmakeProjectName: Property<String>
+ @get:Input
+ abstract val cmakeProjectName: Property<String>
/**
- * CMake version from the extension.
+ * CMake version.
*/
- @get:Internal
- abstract val cmakeVersion: Property<String>
+ @get:Input
+ abstract val cmakeVersion: Property<String>Note: projectDirectory can remain @Internal since it's used for output file resolution, not as a logical input.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Provides lazy access to the [SecretsVaultExtension] of the project. | |
| * Project directory. | |
| */ | |
| private val secretsVaultExtension by lazy { | |
| project.extensions.getByType(SecretsVaultExtension::class.java) | |
| } | |
| @get:Internal | |
| abstract val projectDirectory: Property<Directory> | |
| /** | |
| * Provides lazy access to the [CMakeExtension] of the project. | |
| * Secrets file from the extension. | |
| */ | |
| private val cMakeExtension by lazy { | |
| secretsVaultExtension.cmake.get() | |
| } | |
| @get:Internal | |
| abstract val secretsFile: Property<File> | |
| /** | |
| * Get the package name of the module on which this plugin is used | |
| * | |
| * The function will first attempt to get the package name from the [SecretsVaultExtension]. | |
| * If it's not provided (i.e., it's an empty string), the function will attempt to get the namespace | |
| * from the [CommonExtension] of the project. | |
| * Source set to secrets mapping file from the extension. | |
| */ | |
| private val packageName: String by lazy { | |
| secretsVaultExtension.packageName.getOrElse(EMPTY_STRING).ifEmpty { | |
| project.extensions.getByType(CommonExtension::class.java).namespace.orEmpty() | |
| } | |
| } | |
| @get:Internal | |
| abstract val sourceSetSecretsMappingFile: Property<File> | |
| /** | |
| * Obfuscation key from the extension. | |
| */ | |
| @get:Internal | |
| abstract val obfuscationKey: Property<String> | |
| /** | |
| * App signatures from the extension. | |
| */ | |
| @get:Internal | |
| abstract val appSignatures: Property<List<String>> | |
| /** | |
| * Make injectable flag from the extension. | |
| */ | |
| @get:Internal | |
| abstract val makeInjectable: Property<Boolean> | |
| /** | |
| * Package name. | |
| */ | |
| @get:Internal | |
| abstract val packageName: Property<String> | |
| /** | |
| * CMake project name from the extension. | |
| */ | |
| @get:Internal | |
| abstract val cmakeProjectName: Property<String> | |
| /** | |
| * CMake version from the extension. | |
| */ | |
| @get:Internal | |
| abstract val cmakeVersion: Property<String> | |
| /** | |
| * Project directory. | |
| */ | |
| @get:Internal | |
| abstract val projectDirectory: Property<Directory> | |
| /** | |
| * Secrets file. | |
| */ | |
| @get:InputFile | |
| abstract val secretsFile: Property<File> | |
| /** | |
| * Source set to secrets mapping file. | |
| */ | |
| @get:Optional | |
| @get:InputFile | |
| abstract val sourceSetSecretsMappingFile: Property<File> | |
| /** | |
| * Obfuscation key. | |
| */ | |
| @get:Input | |
| abstract val obfuscationKey: Property<String> | |
| /** | |
| * App signatures. | |
| */ | |
| @get:Input | |
| abstract val appSignatures: Property<List<String>> | |
| /** | |
| * Make injectable flag. | |
| */ | |
| @get:Input | |
| abstract val makeInjectable: Property<Boolean> | |
| /** | |
| * Package name. | |
| */ | |
| @get:Input | |
| abstract val packageName: Property<String> | |
| /** | |
| * CMake project name. | |
| */ | |
| @get:Input | |
| abstract val cmakeProjectName: Property<String> | |
| /** | |
| * CMake version. | |
| */ | |
| @get:Input | |
| abstract val cmakeVersion: Property<String> |
🤖 Prompt for AI Agents
In
secretsvaultplugin/src/main/kotlin/com/commencis/secretsvaultplugin/KeepSecretsTask.kt
around lines 79 to 131, several properties are incorrectly annotated as
@get:Internal which prevents Gradle from treating them as task inputs; update
the annotations so Gradle can perform incremental build checks: keep
projectDirectory as @get:Internal, change secretsFile and
sourceSetSecretsMappingFile to @get:InputFile, and mark obfuscationKey,
appSignatures, makeInjectable, packageName, cmakeProjectName, and cmakeVersion
with @get:Input (use @get:Input on the List<String> and Boolean as well) so the
task will re-run when any of these values change.
|
Configuration cache support has been added with #21 in version 0.1.4 |
3 participants
KeepSecretsTaskis updated to use' FileSystemOperations' and' ProjectLayout' instead of accessing the project at execution time to improve cache-friendliness.https://docs.gradle.org/8.9/userguide/configuration_cache.html#config_cache:requirements:use_project_during_execution
Issue: #19
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.