ADFA-2849: do not overwrite repositories in Gradle Plugin#995
ADFA-2849: do not overwrite repositories in Gradle Plugin#995itsaky-adfa merged 8 commits intostagefrom
Conversation
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
|
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:
📝 WalkthroughWalkthroughAdds a new Settings plugin (COTGSettingsPlugin) and refactors AndroidIDEInitScriptPlugin to register that Settings plugin before settings evaluation, simplify repository handling, keep daemon-log cleanup, and apply the cogo-plugin classpath and plugin to eligible subprojects. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as InitScriptPlugin
participant Settings as GradleSettings
participant COTG as COTGSettingsPlugin
participant Repo as RepositoryManagement
participant Sub as Subprojects
Init->>Settings: register beforeSettings hook
Note right of Settings: beforeSettings hook executes during settings evaluation
Settings->>COTG: apply COTGSettingsPlugin
COTG->>Repo: read StartParameter / project properties
Repo->>Repo: ensure MAVEN_LOCAL_REPOSITORY present (idempotent)
alt test env detected
COTG->>Repo: resolve additional local paths -> add repos (idempotent)
end
Init->>Init: add root project classpath for cogo-plugin
Init->>Sub: afterEvaluate -> apply cogo-plugin to eligible subprojects
Init->>Init: remove daemon logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt (2)
170-175: Consider usingGradleException/InvalidUserDataExceptioninstead ofrequire.
require(dir.isDirectory)throwskotlin.IllegalArgumentException. In Gradle plugin code, usingGradleException(orInvalidUserDataException) produces a cleaner "BUILD FAILED with an exception" summary in build output rather than an undecorated stack trace. The message could also distinguish the two failure modes.♻️ Proposed refactor
val dir = File(path) -require(dir.isDirectory) { "Repo not found: $path" } +if (!dir.exists()) throw InvalidUserDataException("Maven local repo path does not exist: $path") +if (!dir.isDirectory) throw InvalidUserDataException("Maven local repo path is not a directory: $path")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` around lines 170 - 175, Replace the use of Kotlin's require in addLocalMavenRepoIfMissing with a Gradle-specific exception to produce proper "BUILD FAILED" output: check File(path).exists() and File(path).isDirectory() and throw either org.gradle.api.InvalidUserDataException (for missing path) or org.gradle.api.GradleException (for not a directory) with clear, distinct messages; update the function (addLocalMavenRepoIfMissing) to import and throw these exceptions instead of calling require so Gradle surfaces the error correctly.
142-142: Nit:.toList()is redundant; consider documenting the POSIX-only path separator assumption.
String.split(delimiter: Char)already returnsList<String>, so the chained.toList()is a no-op.Additionally, splitting on
:works correctly on POSIX systems but would corrupt Windows-style paths (e.g.,C:\reposplits into["C", "\\repo"]). Since this only executes in test/CI environments on Linux/Android, risk is low, but a brief comment would make the constraint explicit.♻️ Proposed fix
-isTestEnv to mavenLocalRepos.split(':').toList().filter { it.isNotBlank() } +// NOTE: ':' is the POSIX multi-path separator; Windows paths are not supported here. +isTestEnv to mavenLocalRepos.split(':').filter { it.isNotBlank() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` at line 142, The expression mavenLocalRepos.split(':').toList().filter { it.isNotBlank() } contains a redundant .toList() and should be simplified to mavenLocalRepos.split(':').filter { it.isNotBlank() }; remove the .toList() from the mapping (the code around the isTestEnv mapping where mavenLocalRepos is split) and add a short comment above that split noting the POSIX-only assumption (splitting on ':' will break Windows paths like C:\repo, but this runs only in test/CI on Linux/Android).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Around line 170-175: Replace the use of Kotlin's require in
addLocalMavenRepoIfMissing with a Gradle-specific exception to produce proper
"BUILD FAILED" output: check File(path).exists() and File(path).isDirectory()
and throw either org.gradle.api.InvalidUserDataException (for missing path) or
org.gradle.api.GradleException (for not a directory) with clear, distinct
messages; update the function (addLocalMavenRepoIfMissing) to import and throw
these exceptions instead of calling require so Gradle surfaces the error
correctly.
- Line 142: The expression mavenLocalRepos.split(':').toList().filter {
it.isNotBlank() } contains a redundant .toList() and should be simplified to
mavenLocalRepos.split(':').filter { it.isNotBlank() }; remove the .toList() from
the mapping (the code around the isTestEnv mapping where mavenLocalRepos is
split) and add a short comment above that split noting the POSIX-only assumption
(splitting on ':' will break Windows paths like C:\repo, but this runs only in
test/CI on Linux/Android).
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt (2)
149-149: Redundant.toList()call —String.split()already returnsList<String>
String.split(Char)in Kotlin's stdlib returnsList<String>directly; chaining.toList()allocates a second list for no benefit.♻️ Proposed fix
- isTestEnv to mavenLocalRepos.split(':').toList().filter { it.isNotBlank() } + isTestEnv to mavenLocalRepos.split(':').filter { it.isNotBlank() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` at line 149, The mapping that builds the value for isTestEnv uses mavenLocalRepos.split(':').toList().filter { it.isNotBlank() } which unnecessarily calls .toList() because String.split(':') already returns a List<String>; remove the redundant .toList() so the expression becomes mavenLocalRepos.split(':').filter { it.isNotBlank() } (locate this in AndroidIDEInitScriptPlugin.kt where mavenLocalRepos is used to construct isTestEnv).
165-172: URI equality check may miss duplicates if external scripts register repos with non-normalized paths
File.toURI()appends a trailing slash for directories (e.g.,file:///path/to/repo/).URI.equals()performs literal string comparison without normalization, so the same physical directory registered via a differently-formatted URI (missing trailing slash, containing..segments, using a symlink path, or differing in case on case-insensitive filesystems) will not be detected as a duplicate.Within this plugin, the risk is minimal since all repository additions via
addLocalMavenRepoIfMissing()consistently useFile.toURI(). The concern applies primarily to repositories added externally through a user's own settings script, where URIs may not be normalized. For greater robustness, consider comparing canonical or normalized URIs instead of relying on literal equality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` around lines 165 - 172, The current literal URI equality in addMavenRepoIfMissing can miss duplicates due to non-normalized or filesystem-varying URIs; change the comparison to normalize/resolve both the candidate uri and existing repository URLs before comparing: for file:// URIs convert to File and use canonicalFile (or toRealPath) and compare their canonical file URIs, for non-file URIs use uri.normalize() and trim any trailing slash before comparing; update the none { it is MavenArtifactRepository && it.url == uri } check to perform this normalized/ canonicalized comparison against MavenArtifactRepository.url so duplicate repos added with different string forms are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Around line 157-158: The require check in AndroidIDEInitScriptPlugin.kt using
dir.isDirectory() has a misleading message for when the path exists but is a
file; update the validation around the val dir = File(path) and require to
differentiate cases: if dir.exists() and !dir.isDirectory() then throw/require
with a message like "Expected directory but found file: $path", if !dir.exists()
then keep "Repo not found: $path"; ensure you reference the existing val dir and
path variable and replace the single require(dir.isDirectory) call with explicit
conditional checks that produce the correct, distinct error messages.
---
Nitpick comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Line 149: The mapping that builds the value for isTestEnv uses
mavenLocalRepos.split(':').toList().filter { it.isNotBlank() } which
unnecessarily calls .toList() because String.split(':') already returns a
List<String>; remove the redundant .toList() so the expression becomes
mavenLocalRepos.split(':').filter { it.isNotBlank() } (locate this in
AndroidIDEInitScriptPlugin.kt where mavenLocalRepos is used to construct
isTestEnv).
- Around line 165-172: The current literal URI equality in addMavenRepoIfMissing
can miss duplicates due to non-normalized or filesystem-varying URIs; change the
comparison to normalize/resolve both the candidate uri and existing repository
URLs before comparing: for file:// URIs convert to File and use canonicalFile
(or toRealPath) and compare their canonical file URIs, for non-file URIs use
uri.normalize() and trim any trailing slash before comparing; update the none {
it is MavenArtifactRepository && it.url == uri } check to perform this
normalized/ canonicalized comparison against MavenArtifactRepository.url so
duplicate repos added with different string forms are detected.
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt (1)
60-70: Misleading error message when path exists but is not a directory.
File.isDirectory()returnsfalseboth when the path doesn't exist and when it's a regular file, but the error message always says "Repo not found." Consider splitting into two checks for clearer diagnostics.🛡️ Proposed fix
- require(dir.isDirectory) { "Repo not found: $path" } + require(dir.exists()) { "Repo not found: $path" } + require(dir.isDirectory) { "Repo path is not a directory: $path" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt` around lines 60 - 70, In addLocalMavenRepoIfMissing(logger, path) the single require(dir.isDirectory) message "Repo not found" is misleading for the case where the path exists but is a file; change the checks to first verify dir.exists() and throw/require with "Repo not found: {path}" if missing, then check dir.isDirectory() and throw/require with a clearer message like "Path exists but is not a directory: {path}" if not a directory, keeping the subsequent addMavenRepoIfMissing(logger, uri) call unchanged.
🧹 Nitpick comments (3)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt (1)
48-57: Minor:mavenLocalReposis always computed even whenisTestEnvisfalse.This is harmless since the result is discarded, but you could short-circuit by returning early when
isTestEnvis false to avoid the unnecessary string split. Not a functional issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt` around lines 48 - 57, getTestEnvProps currently computes mavenLocalRepos even when isTestEnv is false; modify getTestEnvProps to check isTestEnv first and return false with empty list immediately if not set, otherwise read and split the _PROPERTY_MAVEN_LOCAL_REPOSITORY value; update the logic inside getTestEnvProps (referencing isTestEnv, mavenLocalRepos, and the function name getTestEnvProps) to short-circuit and avoid calling split when unnecessary.gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt (2)
86-94:logFile.delete()return value is silently ignored.
File.delete()returnsfalseon failure. While this is acceptable for best-effort log cleanup, logging the failure would aid debugging in environments with restrictive file permissions.Proposed improvement
?.forEach { logFile -> - logger.lifecycle("deleting log: ${logFile.name}") - logFile.delete() + val deleted = logFile.delete() + if (deleted) { + logger.lifecycle("Deleted log: ${logFile.name}") + } else { + logger.warn("Failed to delete log: ${logFile.name}") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` around lines 86 - 94, The cleanup loop in AndroidIDEInitScriptPlugin using logsDir...drop(MAX_LOGFILE_COUNT).forEach currently calls logFile.delete() and ignores its boolean result; update the loop (referencing logsDir, MAX_LOGFILE_COUNT, the forEach block and logFile.delete()) to check the return value and log a warning or error via logger (e.g., logger.warn or logger.lifecycle) when delete() returns false, including the logFile.name and reason context so failures due to permissions are visible; keep the operation best-effort (do not throw), just log failures for debugging.
82-100: Else-branch log message is slightly misleading when the directory doesn't exist.When
logsDirdoesn't exist at all (e.g., fresh Gradle install or mismatched version), the message says "number of log files does not exceed…" which implies the directory was checked for file count. Consider differentiating the two cases for easier troubleshooting.Proposed improvement
} else { logger.lifecycle( - "No deletions made, number of log files does not" + - " exceed ($MAX_LOGFILE_COUNT) for gradle ($currentGradleVersion).", + "No deletions made. Logs directory does not exist or is not" + + " a directory: ${logsDir.absolutePath}", ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` around lines 82 - 100, The else-branch message is ambiguous when logsDir doesn't exist; update the conditional to distinguish the two cases: check logsDir.exists() and logsDir.isDirectory separately and log one message when the directory is missing or not a directory (mention logsDir path and currentGradleVersion) and a different message when the directory exists but file count does not exceed MAX_LOGFILE_COUNT; use the existing logsDir, MAX_LOGFILE_COUNT, currentGradleVersion and logger.lifecycle calls to report the appropriate, clear status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt`:
- Around line 60-70: In addLocalMavenRepoIfMissing(logger, path) the single
require(dir.isDirectory) message "Repo not found" is misleading for the case
where the path exists but is a file; change the checks to first verify
dir.exists() and throw/require with "Repo not found: {path}" if missing, then
check dir.isDirectory() and throw/require with a clearer message like "Path
exists but is not a directory: {path}" if not a directory, keeping the
subsequent addMavenRepoIfMissing(logger, uri) call unchanged.
---
Nitpick comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Around line 86-94: The cleanup loop in AndroidIDEInitScriptPlugin using
logsDir...drop(MAX_LOGFILE_COUNT).forEach currently calls logFile.delete() and
ignores its boolean result; update the loop (referencing logsDir,
MAX_LOGFILE_COUNT, the forEach block and logFile.delete()) to check the return
value and log a warning or error via logger (e.g., logger.warn or
logger.lifecycle) when delete() returns false, including the logFile.name and
reason context so failures due to permissions are visible; keep the operation
best-effort (do not throw), just log failures for debugging.
- Around line 82-100: The else-branch message is ambiguous when logsDir doesn't
exist; update the conditional to distinguish the two cases: check
logsDir.exists() and logsDir.isDirectory separately and log one message when the
directory is missing or not a directory (mention logsDir path and
currentGradleVersion) and a different message when the directory exists but file
count does not exceed MAX_LOGFILE_COUNT; use the existing logsDir,
MAX_LOGFILE_COUNT, currentGradleVersion and logger.lifecycle calls to report the
appropriate, clear status.
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt`:
- Around line 48-57: getTestEnvProps currently computes mavenLocalRepos even
when isTestEnv is false; modify getTestEnvProps to check isTestEnv first and
return false with empty list immediately if not set, otherwise read and split
the _PROPERTY_MAVEN_LOCAL_REPOSITORY value; update the logic inside
getTestEnvProps (referencing isTestEnv, mavenLocalRepos, and the function name
getTestEnvProps) to short-circuit and avoid calling split when unnecessary.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt (1)
93-93: Consider checking the result ofdelete().
File.delete()returnsfalseif deletion fails (e.g., due to permissions or file in use), but the result is currently ignored. For cleanup operations this may be acceptable, but logging failures could help with troubleshooting.💡 Optional: Log deletion failures
?.forEach { logFile -> - logger.lifecycle("deleting log: ${logFile.name}") - logFile.delete() + val deleted = logFile.delete() + if (deleted) { + logger.lifecycle("deleted log: ${logFile.name}") + } else { + logger.warn("failed to delete log: ${logFile.name}") + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt` at line 93, The call to logFile.delete() in AndroidIDEInitScriptPlugin.kt currently ignores the boolean result; update the cleanup to check the return value of logFile.delete() and log a warning or error when it returns false so failed deletions are visible (e.g., use the plugin logger or project.logger in the AndroidIDEInitScriptPlugin to emit a message referencing logFile.path). Ensure this check is added where logFile.delete() is invoked so failures are reported without changing control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Around line 95-100: The else branch in AndroidIDEInitScriptPlugin.kt currently
logs an inaccurate message when (!logsDir.exists() || !logsDir.isDirectory);
update the logic around logsDir so the code distinguishes the cases: if logsDir
does not exist log that the daemon logs directory for currentGradleVersion is
missing, if logsDir exists but is not a directory log that the path is not a
directory, otherwise (when it is a valid directory) log that no deletions were
made because the number of log files does not exceed MAX_LOGFILE_COUNT; use the
existing symbols logsDir, currentGradleVersion, MAX_LOGFILE_COUNT and
logger.lifecycle to implement these precise messages.
---
Nitpick comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt`:
- Line 93: The call to logFile.delete() in AndroidIDEInitScriptPlugin.kt
currently ignores the boolean result; update the cleanup to check the return
value of logFile.delete() and log a warning or error when it returns false so
failed deletions are visible (e.g., use the plugin logger or project.logger in
the AndroidIDEInitScriptPlugin to emit a message referencing logFile.path).
Ensure this check is added where logFile.delete() is invoked so failures are
reported without changing control flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt
Show resolved
Hide resolved
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt`:
- Around line 51-60: getTestEnvProps currently splits the
_PROPERTY_MAVEN_LOCAL_REPOSITORY string with a hardcoded ':' which breaks on
Windows; change the split to use the OS-aware path separator
(File.pathSeparatorChar or File.pathSeparator) so repository paths like "C:\..."
or semicolon-separated lists are parsed correctly; update the split call in
getTestEnvProps (referencing StartParameter and
_PROPERTY_MAVEN_LOCAL_REPOSITORY) to use File.pathSeparatorChar and ensure the
import/reference to java.io.File is available.
- Around line 75-83: In RepositoryHandler.addMavenRepoIfMissing, remove the
unnecessary addLast(...) wrapper around the maven call; instead, when the none {
it is MavenArtifactRepository && it.url == uri } check passes, call maven {
it.url = uri } directly (keep the logger.info("Adding maven repository: $uri")
and the same uri/Logger parameters intact) so the repository is added by the
maven(Action) call itself.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.ktgradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- gradle-plugin/src/main/java/com/itsaky/androidide/gradle/AndroidIDEInitScriptPlugin.kt
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt
Show resolved
Hide resolved
gradle-plugin/src/main/java/com/itsaky/androidide/gradle/COTGSettingsPlugin.kt
Show resolved
Hide resolved
Signed-off-by: Akash Yadav <akashyadav@appdevforall.org>
See ADFA-2849 for more details.