build: keep local verification runs from hijacking the desktop#6370
Conversation
Full local suites (spotless+detekt+assembleDebug+test+allTests) made dev laptops unusable: every forked test JVM initialized AWT on macOS, spawning a Dock icon that stole window focus, and the daemon ran at normal OS priority, starving foreground apps. The remote HTTP build cache was also silently disabled locally because the settings script never looked in secrets.properties, where developers keep the GRADLE_CACHE_* credentials. - Force java.awt.headless=true (+apple.awt.UIElement) on all Test forks and on the Gradle daemon itself; no-op on headless CI - Run the daemon and its workers at low OS priority (org.gradle.priority) - Include secrets.properties in the build-cache credential lookup walk Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesBuild Configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)gradle.propertiesTraceback (most recent call last): 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)
gradle.properties (1)
26-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid lowering CI build priority unintentionally.
gradle.propertiesis consumed by project and CI builds, soorg.gradle.priority=lowapplies everywhere—not only on developer laptops. Gradle documents this property as the equivalent of--priority low. (docs.gradle.org)Keep this in developer-local Gradle properties, or explicitly restore normal priority in CI.
🤖 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 `@gradle.properties` around lines 26 - 28, Remove org.gradle.priority=low from the shared gradle.properties so CI builds retain normal priority, and place the low-priority setting in developer-local Gradle properties instead. Preserve the existing build configuration while limiting this setting to local developer environments.
🤖 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 `@gradle/build-cache.settings.gradle`:
- Around line 24-30: Update the property lookup loop in the build-cache settings
so blank or whitespace-only values do not terminate the search. In the lookup
method containing the local.properties, config.properties, and
secrets.properties iteration, retrieve and trim the property value, return it
only when non-empty, and continue to later files otherwise, preserving the
existing precedence and getMeshProperty behavior.
---
Nitpick comments:
In `@gradle.properties`:
- Around line 26-28: Remove org.gradle.priority=low from the shared
gradle.properties so CI builds retain normal priority, and place the
low-priority setting in developer-local Gradle properties instead. Preserve the
existing build configuration while limiting this setting to local developer
environments.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0806d427-fa89-4ed9-bcee-d9f0b76fb912
📒 Files selected for processing (3)
build-logic/convention/src/main/kotlin/org/meshtastic/buildlogic/ProjectExtensions.ktgradle.propertiesgradle/build-cache.settings.gradle
| for (name in ["local.properties", "config.properties", "secrets.properties"]) { | ||
| def file = new File(currentDir, name) | ||
| if (file.exists()) { | ||
| def props = new Properties() | ||
| file.withInputStream { props.load(it) } | ||
| if (props.containsKey(key)) return props.getProperty(key) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '1,120p' gradle/build-cache.settings.gradle || true
echo
echo "== searches for build-cache/settings/cache password usage =="
rg -n "build-cache|GRADLE_CACHE_PASSWORD|getOrCreateDirectory|cache|secrets.properties|local.properties|config.properties" gradle settings.gradle* settings.gradle 2>/dev/null || true
echo
echo "== git status =="
git status --porcelainRepository: meshtastic/Meshtastic-Android
Length of output: 5306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from properties import loadString # only if installed; avoid
PY
python3 - <<'PY'
import io
from grdp.properties import loadString
PYRepository: meshtastic/Meshtastic-Android
Length of output: 290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files gradle settings.gradle settings.gradle.kts 2>/dev/null || true
echo
echo "== Kotlin/Java Properties.load behavior documentation if Gradle source is present =="
rg -n "class Properties|fun Properties|Properties\\.load|containsKey\\(key\\)" -S . 2>/dev/null | head -80 || true
echo
echo "== repository files with credentials/password checks around cache setup =="
rg -n "getMeshProperty\\(|GRADLE_CACHE_PASSWORD|credentials|cacheUsername|cachePassword" gradle settings.gradle settings.gradle.kts settings.gradle.kts 2>/dev/null || trueRepository: meshtastic/Meshtastic-Android
Length of output: 2041
🌐 Web query:
Java Properties load key=value empty value containsKey getProperty
💡 Result:
In Java, the java.util.Properties class handles key-value pairs where both keys and values are treated as strings [1][2]. When using the load method, an entry defined as key= (with no value following the equals sign) results in a key being mapped to an empty string ("") [3]. Consequently, calling getProperty(key) on such a key will return an empty string, not null [1][4]. Regarding the behavior of containsKey versus getProperty: 1. containsKey(key): This method checks only the properties explicitly set within the Properties object itself. It returns true if the key exists (even if its associated value is an empty string) and false otherwise [5][2]. It does not search the default property list [5]. 2. getProperty(key): This method searches for the key within the current Properties object and, if not found, recursively searches through any associated default property list [1][5]. It returns the value associated with the key or null if the key is not found in either the current object or the defaults [1][4]. Because of these differences, it is possible for containsKey to return false for a specific key (if it is not in the current Properties object) while getProperty returns a non-null value (because it was found in the default properties list) [5]. Conversely, if a key is present with an empty value, containsKey will return true, and getProperty will return "" [3][5].
Citations:
- 1: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/util/Properties.html
- 2: https://docs.oracle.com/javase/tutorial/essential/environment/properties.html
- 3: https://stackoverflow.com/questions/32744507/java-properties-file-useful-case-of-empty-property-value
- 4: https://www.baeldung.com/java-properties
- 5: https://stackoverflow.com/questions/45043515/java-util-properties-containskeykey-false-getpropertykey-has-a-valid-val
🌐 Web query:
Android Gradle Plugin GroovyDslPropertyProvider propertyProperties behavior trim empty properties
💡 Result:
There is no documented component, class, or method named GroovyDslPropertyProvider in the Android Gradle Plugin or Gradle's public API [1][2][3]. Furthermore, there is no automatic "trim empty properties" behavior for properties in Gradle [4][5]. In Gradle, property behavior regarding empty values is determined by the specific property type (e.g., Property, ListProperty, MapProperty) and how you configure it [1][2]. Common sources of confusion include: 1. Handling of Absent Providers: Adding a provider that has no value (is "absent") to a collection property (like ListProperty or SetProperty) can clear the property or cause it to throw a MissingValueException when accessed, rather than being ignored [4]. 2. Lazy Configuration: If you are working with properties, it is best practice to use methods like orElse to provide a fallback value if the underlying provider is empty or absent, ensuring the property remains in a defined state [4][5]. 3. Groovy DSL Syntax: Recent changes in Gradle have deprecated the "space-assignment" syntax (e.g., property value) in favor of explicit assignment (property = value) [6][7]. If you are encountering issues with property values not being set, ensure you are using the explicit assignment syntax [6]. If you are dealing with specific property data (e.g., loading from a.properties file), ensure you are explicitly sanitizing or filtering empty strings using standard Java/Groovy methods (such as.trim or.filter {}) before assigning those values to a Gradle Property, as Gradle does not automatically perform this cleanup [5][8].
Citations:
- 1: https://docs.gradle.org/current/userguide/lazy_configuration.html
- 2: https://docs.gradle.org/current/userguide/properties_providers.html
- 3: https://docs.gradle.org/9.0.0/userguide/properties_providers.html
- 4: https://www.zacsweers.dev/gradle-footgun-adding-empty-providers-to-collection-properties/
- 5: https://docs.gradle.org/nightly/kotlin-dsl/gradle/org.gradle.api.provider/-property/index.html
- 6: Android Issue: Space-assignment syntax in Groovy DSL has been deprecated! react-native-device-info/react-native-device-info#1686
- 7: Add support for 'property value' assignment for Groovy DSL gradle/gradle#30398
- 8: Using
Provideras value forPropertyprevents it from being live gradle/gradle#19165
🌐 Web query:
Apache Commons Properties XMLParserFactory load key equals sign only properties key containsKey getProperty empty value
💡 Result:
In Apache Commons Configuration, the handling of keys and values in properties files is governed by the library's property parser, which supports standard key-value syntax [1][2]. Properties File Parsing The PropertiesConfiguration class, often used for loading properties, recognizes keys and values separated by '=', ':', or whitespace [1][3]. If a property line contains a key followed by an equals sign but no value (e.g., my.key =), this is treated as a valid property with an empty string as its value [4]. Recent updates (such as those addressing issue CONFIGURATION-847) have ensured that this distinction—between an unspecified property and a property explicitly set to an empty string—is correctly preserved [4]. containsKey and getProperty Behavior - containsKey(String key): This method returns true if the specified key exists in the configuration [5][6]. For standard key-value configurations, an entry defined as my.key = will result in containsKey returning true [4]. Note that in HierarchicalConfiguration (often used with XML), containsKey may return false if a node exists but has no associated value (only child nodes) [7]. - getProperty(String key): This method returns the raw value associated with the key [8][9]. If the key exists and is defined as an empty string (e.g., my.key =), getProperty will return an empty string ("") [4]. If the key is not present in the configuration at all, getProperty returns null [8][9]. In summary, Apache Commons Configuration explicitly treats a key defined with an equals sign but no trailing text as a key mapped to an empty string, which is detectable via containsKey (returning true) and retrievable via getProperty (returning "") [8][4][5].
Citations:
- 1: https://commons.apache.org/proper/commons-configuration/apidocs/org/apache/commons/configuration2/PropertiesConfiguration.html
- 2: https://commons.apache.org/proper/commons-configuration/apidocs/src-html/org/apache/commons/configuration2/PropertiesConfiguration.html
- 3: https://commons.apache.org/proper/commons-configuration/xref/org/apache/commons/configuration2/PropertiesConfiguration.html
- 4: [CONFIGURATION-847] Property with an empty string value was not processed apache/commons-configuration#431
- 5: https://commons.apache.org/proper/commons-configuration/javadocs/v1.10/apidocs/org/apache/commons/configuration/BaseConfiguration.html
- 6: https://commons.apache.org/proper/commons-configuration/apidocs/org/apache/commons/configuration2/AbstractConfiguration.html
- 7: https://stackoverflow.com/questions/32895398/apache-commons-configurations-containskey-returns-false-on-existing-key
- 8: https://commons.apache.org/proper/commons-configuration/apidocs/org/apache/commons/configuration2/Configuration.html
- 9: https://commons.apache.org/proper/commons-configuration/javadocs/v1.10/apidocs/org/apache/commons/configuration/Configuration.html
🌐 Web query:
Java Properties.load empty value key equals whitespace behavior trim
💡 Result:
In Java, the behavior of the java.util.Properties.load method regarding whitespace is strictly defined by its documented format [1][2]. Key Whitespace Handling: 1. Whitespace preceding the key: Ignored. The key starts at the first non-whitespace character on the line [1][3]. 2. Whitespace after the key: Any whitespace immediately following the key is skipped until the first non-whitespace character is encountered [1][2]. 3. Separators: If that first non-whitespace character is an equals sign (=) or colon (:), it is treated as the separator and is ignored, along with any subsequent whitespace characters that immediately follow it [1][2]. Value Whitespace Handling: 1. Leading whitespace in values: Once the separator is processed, all remaining characters on the line—including leading, trailing, and embedded whitespace—are preserved as part of the value [1][2]. This means leading whitespace after the separator is not trimmed; it becomes part of the property value [1][2]. 2. Trailing whitespace in values: Trailing whitespace is also considered significant and is included in the property value [4]. The Properties class does not automatically trim trailing whitespace from the value string [1][2]. 3. Empty values: If there are no characters remaining after the separator and its following whitespace, the element is set to an empty string ("") [1][2]. In summary, while the Properties.load method aggressively skips whitespace around the key and the separator, it preserves all whitespace within the value itself [1][2]. If you require trimmed values, you must manually perform the trim operation on the string returned by getProperty [1][2].
Citations:
- 1: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/util/Properties.html
- 2: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Properties.html
- 3: https://docs.oracle.com/en/java/javase/18/docs/api/java.base/java/util/Properties.html
- 4: https://bugs.openjdk.org/browse/JDK-8298042?page=com.atlassian.streams.streams-jira-plugin%3Aactivity-stream-issue-tab
Do not let blank properties shadow secrets.properties.
Properties.containsKey(key) || props.getProperty(key) maps an entry like GRADLE_CACHE_PASSWORD= to an empty string, so the lookup returns before reaching a valid value in secrets.properties. Since getMeshProperty(...)?.trim() treats empty values as absent, preserve that precedence consistently.
Proposed fix
- if (props.containsKey(key)) return props.getProperty(key)
+ def value = props.getProperty(key)?.trim()
+ if (value) return value📝 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.
| for (name in ["local.properties", "config.properties", "secrets.properties"]) { | |
| def file = new File(currentDir, name) | |
| if (file.exists()) { | |
| def props = new Properties() | |
| file.withInputStream { props.load(it) } | |
| if (props.containsKey(key)) return props.getProperty(key) | |
| } | |
| for (name in ["local.properties", "config.properties", "secrets.properties"]) { | |
| def file = new File(currentDir, name) | |
| if (file.exists()) { | |
| def props = new Properties() | |
| file.withInputStream { props.load(it) } | |
| def value = props.getProperty(key)?.trim() | |
| if (value) return value | |
| } |
🤖 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 `@gradle/build-cache.settings.gradle` around lines 24 - 30, Update the property
lookup loop in the build-cache settings so blank or whitespace-only values do
not terminate the search. In the lookup method containing the local.properties,
config.properties, and secrets.properties iteration, retrieve and trim the
property value, return it only when non-empty, and continue to later files
otherwise, preserving the existing precedence and getMeshProperty behavior.
1 participant
Running the full local verification suite (
spotlessApply spotlessCheck detekt assembleDebug test allTests) makes developer laptops nearly unusable: every forked test JVM initializes AWT on macOS, so each fork spawns a Dock icon and steals window focus, and the Gradle daemon competes with foreground apps at normal OS priority. Separately, the remote HTTP build cache was silently disabled for local builds —gradle/build-cache.settings.gradleonly searchedlocal.properties/config.propertiesfor theGRADLE_CACHE_*keys, while developers keep them in the git-ignoredsecrets.properties.🛠️ Improvements
configureTestOptions()now setsjava.awt.headless=true(plusapple.awt.UIElement=trueas a fallback) on everyTestfork project-wide, and the same flags are added to the daemon'sorg.gradle.jvmargsso in-daemon work can't spawn AWT either. No-op on headless Linux CI.org.gradle.priority=lowrenices the daemon and all its worker/test processes, keeping the desktop responsive during full suites. Negligible on dedicated CI runners where nothing competes.🧹 Cleanup
secrets.propertiesjoined the directory-walk lookup inbuild-cache.settings.gradle(folded the three per-file blocks into one loop). Nothing sensitive is committed; keys still come from git-ignored files or env vars, and CI continues to use env-var secrets.Testing Performed
spotlessApply spotlessCheck detekt assembleDebug test allTests— BUILD SUCCESSFUL, no failures, with headless forks and low priority active.--inforun and confirmed all of them exist on the cache server via authenticated GET (200), proving push and pull work.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores