fix/ADFA-3960 run bundlellamaassets for release builds only otherwise skip#1299
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflows set INCLUDE_LLAMA_ASSETS for release and debug builds. The Gradle build script computes skipLlamaAssets from that env var with inverted/default semantics. V7/V8 debug assemble tasks add assetsDownloadDebug only when not CI and when llama assets are not skipped. ChangesLlama Asset Bundling Workflow
Sequence DiagramsequenceDiagram
participant ReleaseWorkflow
participant DebugWorkflow
participant GradleBuild
participant BuildScript
ReleaseWorkflow->>GradleBuild: step env INCLUDE_LLAMA_ASSETS="true"
DebugWorkflow->>GradleBuild: job env INCLUDE_LLAMA_ASSETS="false"
GradleBuild->>BuildScript: expose env INCLUDE_LLAMA_ASSETS
BuildScript->>BuildScript: compute skipLlamaAssets = !INCLUDE_LLAMA_ASSETS.equals("true")
BuildScript->>GradleBuild: add assetsDownloadDebug if !isCiCd && !skipLlamaAssets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/build.gradle.kts (2)
776-780: 💤 Low valueSimplify the
skipLlamaAssetsexpression.The chained
?.equals(...)?.not() ?: trueis correct but harder to read than a direct comparison. A more idiomatic form:♻️ Suggested refactor
-val skipLlamaAssets: Boolean = - System.getenv("SKIP_LLAMA_ASSETS") - ?.equals("false", ignoreCase = true) - ?.not() - ?: true +val skipLlamaAssets: Boolean = + !System.getenv("SKIP_LLAMA_ASSETS").equals("false", ignoreCase = true)
String?.equals(other, ignoreCase = true)on a null receiver returnsfalse, so when the env var is unset this yields!false = true(skip), matching the intended default. This also keeps the semantics in one line and avoids the Elvis fallback that depends on chained nullable returns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/build.gradle.kts` around lines 776 - 780, The skipLlamaAssets expression is overly verbose; replace the chained nullable calls with a single logical negation of the env var equality check: compute skipLlamaAssets by checking if System.getenv("SKIP_LLAMA_ASSETS") equals "false" (ignore case) and negating that result so unset/null yields true; update the binding of skipLlamaAssets accordingly (refer to the skipLlamaAssets variable and the SKIP_LLAMA_ASSETS env var).
782-786: 💤 Low valueLifecycle log can be misleading when env var is unset.
With the new default semantics,
skipLlamaAssetsistrueeven whenSKIP_LLAMA_ASSETSis not set, but the log at line 783 says"SKIP_LLAMA_ASSETS enabled - ...", implying the env var is set. Consider phrasing the message around the resulting behavior rather than the env var, e.g.:♻️ Suggested wording
-if (skipLlamaAssets) { - project.logger.lifecycle("SKIP_LLAMA_ASSETS enabled - debug assemble tasks will skip llama asset bundling.") -} else { - project.logger.lifecycle("SKIP_LLAMA_ASSETS is disabled - assemble tasks will do llama asset bundling.") -} +if (skipLlamaAssets) { + project.logger.lifecycle("Llama asset bundling will be SKIPPED (set SKIP_LLAMA_ASSETS=false to enable).") +} else { + project.logger.lifecycle("Llama asset bundling is ENABLED.") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/build.gradle.kts` around lines 782 - 786, The lifecycle log messages around the skipLlamaAssets conditional are misleading because they reference the env var instead of the actual behavior; update the messages in the if/else that checks skipLlamaAssets so they state the resulting behavior (e.g., "llama asset bundling will be skipped" when skipLlamaAssets is true and "llama asset bundling will run" when false) rather than asserting the env var is enabled/disabled; modify the project.logger.lifecycle calls in that block (the conditional that uses skipLlamaAssets) to use the new behavior-focused phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/build.gradle.kts`:
- Around line 870-872: The task dependency currently gates the general debug
asset downloader (assetsDownloadDebug) behind skipLlamaAssets; change it so
assetsDownloadDebug remains conditioned only on !isCiCd while the llama-only
dependencies (bundleLlamaV7Assets / bundleLlamaV8Assets) remain gated by
!skipLlamaAssets; update the two occurrences that use
dependsOn("assetsDownloadDebug") so they no longer check skipLlamaAssets and
ensure only the llama bundling lines keep the skipLlamaAssets guard.
---
Nitpick comments:
In `@app/build.gradle.kts`:
- Around line 776-780: The skipLlamaAssets expression is overly verbose; replace
the chained nullable calls with a single logical negation of the env var
equality check: compute skipLlamaAssets by checking if
System.getenv("SKIP_LLAMA_ASSETS") equals "false" (ignore case) and negating
that result so unset/null yields true; update the binding of skipLlamaAssets
accordingly (refer to the skipLlamaAssets variable and the SKIP_LLAMA_ASSETS env
var).
- Around line 782-786: The lifecycle log messages around the skipLlamaAssets
conditional are misleading because they reference the env var instead of the
actual behavior; update the messages in the if/else that checks skipLlamaAssets
so they state the resulting behavior (e.g., "llama asset bundling will be
skipped" when skipLlamaAssets is true and "llama asset bundling will run" when
false) rather than asserting the env var is enabled/disabled; modify the
project.logger.lifecycle calls in that block (the conditional that uses
skipLlamaAssets) to use the new behavior-focused phrasing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e49c9e5-4eb8-46ff-8208-4616db6e6388
📒 Files selected for processing (2)
.github/workflows/release.ymlapp/build.gradle.kts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/build.gradle.kts`:
- Around line 776-780: The skipLlamaAssets boolean is inverted due to a double
negation around the INCLUDE_LLAMA_ASSETS check; update the expression that
computes skipLlamaAssets (the val skipLlamaAssets using
System.getenv("INCLUDE_LLAMA_ASSETS")) to remove the unnecessary .not() so the
logic becomes a single negation of the equals result, yielding "true"→false
(don’t skip), "false"→true (skip), and unset→true (default skip).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ea2109b-0a52-4134-bc29-6ff86b312494
📒 Files selected for processing (3)
.github/workflows/debug.yml.github/workflows/release.ymlapp/build.gradle.kts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/debug.yml
jatezzz
approved these changes
May 13, 2026
jomen-adfa
force-pushed
the
fix/ADFA-3960-build-speedup
branch
2 times, most recently
from
55f944d to
15a5794
Compare
jomen-adfa
force-pushed
the
fix/ADFA-3960-build-speedup
branch
from
15a5794 to
731f352
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run bundlellamaassets for release builds only otherwise skip. Debug builds and generate assets zips will skip.