ADFA-4128: 0902 review round on quickbuild:daemon

Akash's 2 September round, led by the argfile break he verified against
build-tools 37.0.0.

- A link whose resource paths contain whitespace keeps the inline -R pairs
  whatever the input count. The argfile format splits on whitespace and has no
  escape for it, so one space truncated that input and every later one - and
  the project directory reaches these paths unsanitised, with "My Application"
  the default new-project name. Pinned by a test that fails without the guard.
  #1721 (comment)
- The inline path deletes a link-inputs.txt an earlier link left behind, which
  nothing else swept.
  #1721 (comment)
- The unswept-output warning reaches the daemon log. It went to compileLog,
  which defaults to a no-op and which DaemonService cannot pass without also
  taking kotlinc's verbose channel, so the warning has its own parameter.
  #1721 (comment)
- javac's options are assembled by an internal function and --release is
  asserted on the argv. The host JDK emits the same class file version either
  way, so no compile-and-read test can fail when the flag is dropped.
  #1721 (comment)
- CollectingLogger's KDoc no longer says the result is built from warnings as
  well as errors; -nowarn means no real compile drives that channel.
  #1721 (comment)

Not fixed here: the FinalStripper/ClassOpener parity test and the
DaemonService reconfigure-mutates-the-live-session deferral. Both are
ticket-only follow-ups; the ticket text is drafted with this round's replies.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017o3nPrBbGi2XYkMUGavG2A

Author: fryanpan

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/DaemonService.kt

@@ -105,6 +105,11 @@ class DaemonService(
 						(request.classpath + androidJarPath).map(::File),
 						outDir.toPath(),
 						compilerPluginJars = request.compilerPlugins.map(::File),
+						// Only the compiler's own warnings about the build (an output stem it
+						// cannot derive, so a stale class goes unswept). compileLog is left
+						// alone: it carries kotlinc's verbose channel, which would bury the
+						// daemon log.
+						warn = log,
 					),
 				dexTool = DexTool(File(d8JarPath), File(androidJarPath), request.minApi),
 				aapt2Link = Aapt2Link(File(aapt2Path), File(androidJarPath)),

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/compile/IncrementalCompiler.kt

@@ -54,13 +54,16 @@ private typealias OutputSnapshot = Map<String, Pair<Long, Long>>
  * @param compileLog takes each level-tagged compiler log line as it is produced and retains
  *   nothing, since a session-lifetime copy of the engine's verbose debug channel is real memory
  *   on a 2-4 GB phone.
+ * @param warn takes this class's own warnings about the build - not the engine's - so they can
+ *   reach the daemon log without the verbose channel coming with them.
  */
 @OptIn(ExperimentalBuildToolsApi::class)
 class IncrementalCompiler(
 	classpathJars: List<File>,
 	private val workDir: Path,
 	compilerPluginJars: List<File> = emptyList(),
 	private val compileLog: (String) -> Unit = {},
+	private val warn: (String) -> Unit = {},
 ) : AutoCloseable {
 	/** Outcome of one compile. */
 	sealed interface Result {
@@ -466,7 +469,7 @@ class IncrementalCompiler(
 					// outside a java/ or kotlin/ root, as with extraSourceRoots, has no
 					// derivable stem. Logged anyway, because a silently unswept output is the
 					// stale-class bug this sweep exists to prevent.
-					compileLog(
+					warn(
 						"w: cannot derive a class output stem for ${javaFile.path}; its stale outputs are not swept",
 					)
 					return@forEach
@@ -665,8 +668,9 @@ class IncrementalCompiler(
 	 * `internal` rather than private so severity routing is unit-testable - the daemon passes
 	 * `-nowarn`, so no real compile can drive the warn channel from a test.
 	 *
-	 * Errors and warnings are kept because the compile's result is built from them, and they
-	 * die with the compile. Every line is only forwarded, never accumulated.
+	 * Errors are kept because the compile's result is built from them, and they die with the
+	 * compile. Warnings are collected the same way but cannot reach the result: the daemon
+	 * passes `-nowarn`. Every line is only forwarded, never accumulated.
 	 *
 	 * @property emit takes each line already tagged with its level.
 	 */

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/compile/JavaCompileStep.kt

@@ -55,30 +55,45 @@ object JavaCompileStep {
 		val fileManager = compiler.getStandardFileManager(collector, Locale.ROOT, StandardCharsets.UTF_8)
 		fileManager.use { manager ->
 			val units = manager.getJavaFileObjectsFromFiles(javaSources)
-			val options =
-				listOf(
-					"-classpath",
-					classpath.joinToString(File.pathSeparator) { it.absolutePath },
-					"-d",
-					outputDir.absolutePath,
-					// Annotation processing is a full-Gradle-build concern;
-					// running processors here would silently diverge from the real build.
-					"-proc:none",
-					"-encoding",
-					"UTF-8",
-					// Pin bytecode AND platform APIs to the same level kotlinc targets
-					// (-jvm-target). Without this a daemon running on JDK 21 emits major-65
-					// classes next to Kotlin's major-61 in one tree, and java.* resolves
-					// against the running JDK's own modules instead of release-17 signatures.
-					"--release",
-					IncrementalCompiler.JVM_TARGET,
-				)
+			val options = javacOptions(classpath, outputDir)
 			val task = compiler.getTask(StringWriter(), manager, collector, options, null, units)
 			val success = task.call()
 			return Result(success, collector.diagnostics.map { it.toProtocol() })
 		}
 	}
 
+	/**
+	 * The javac options for one compile.
+	 *
+	 * `internal` so the flags are testable: the host JDK compiles this code to the same class
+	 * file version with or without `--release`, so nothing else can tell whether it was passed.
+	 *
+	 * @param classpath the compile classpath, joined with the platform separator.
+	 * @param outputDir the shared Kotlin/Java output tree.
+	 * @return the option list handed to [javax.tools.JavaCompiler.getTask].
+	 */
+	internal fun javacOptions(
+		classpath: List<File>,
+		outputDir: File,
+	): List<String> =
+		listOf(
+			"-classpath",
+			classpath.joinToString(File.pathSeparator) { it.absolutePath },
+			"-d",
+			outputDir.absolutePath,
+			// Annotation processing is a full-Gradle-build concern;
+			// running processors here would silently diverge from the real build.
+			"-proc:none",
+			"-encoding",
+			"UTF-8",
+			// Pin bytecode AND platform APIs to the same level kotlinc targets
+			// (-jvm-target). Without this a daemon running on JDK 21 emits major-65
+			// classes next to Kotlin's major-61 in one tree, and java.* resolves
+			// against the running JDK's own modules instead of release-17 signatures.
+			"--release",
+			IncrementalCompiler.JVM_TARGET,
+		)
+
 	private fun javax.tools.Diagnostic<out JavaFileObject>.toProtocol(): Diagnostic =
 		Diagnostic(
 			severity =

quickbuild/daemon/src/main/kotlin/org/appdevforall/cotg/quickbuild/daemon/res/Aapt2Link.kt

@@ -71,6 +71,9 @@ class Aapt2Link(
 		 * boundary is testable.
 		 */
 		internal const val ARGFILE_THRESHOLD = 100
+
+		/** Name of the `@argfile` written beside the link output. */
+		internal const val ARGFILE_NAME = "link-inputs.txt"
 	}
 
 	/** Outcome of one relink. */
@@ -241,9 +244,12 @@ class Aapt2Link(
 	 * @return the full argv, aapt2's own path included as element 0. Past [ARGFILE_THRESHOLD]
 	 *   resource inputs, the whole input list moves into an `@argfile` next to [linkedApk],
 	 *   passed as a single `-R @file`. aapt2 expands the file into its whitespace-split paths
-	 *   (flags cannot ride along - it rejects them as "missing required flag -o"), every entry
-	 *   keeps `-R` overlay semantics in file order, and the space-free scratch paths never trip
-	 *   the splitting. Both halves of that expansion are pinned by the argfile relink test.
+	 *   (flags cannot ride along - it rejects them as "missing required flag -o") and every
+	 *   entry keeps `-R` overlay semantics in file order. Whitespace has no escape in that
+	 *   format, so an input path containing any keeps the inline `-R` pairs whatever the
+	 *   count: the project directory reaches these paths unsanitised, and the default new
+	 *   project is called "My Application". Both halves of the expansion are pinned by the
+	 *   argfile relink test.
 	 * @throws IOException when the argfile cannot be written; [relink] turns that into a
 	 *   [Result.Failed].
 	 */
@@ -270,16 +276,27 @@ class Aapt2Link(
 			arguments += listOf("--stable-ids", stableIds.absolutePath)
 		}
 		val resourceInputs = libraryResources + flatFiles
-		if (resourceInputs.size <= ARGFILE_THRESHOLD) {
+		val argfile = File(linkedApk.absoluteFile.parentFile, ARGFILE_NAME)
+		if (resourceInputs.size <= ARGFILE_THRESHOLD || resourceInputs.any(::hasWhitespace)) {
+			// A previous link may have left one behind; it is stale the moment the inputs
+			// change, and nothing else deletes it.
+			argfile.delete()
 			resourceInputs.forEach { arguments += listOf("-R", it.absolutePath) }
 			return arguments
 		}
-		val argfile = File(linkedApk.absoluteFile.parentFile, "link-inputs.txt")
 		argfile.writeText(resourceInputs.joinToString("\n") { it.absolutePath })
 		arguments += listOf("-R", "@${argfile.absolutePath}")
 		return arguments
 	}
 
+	/**
+	 * Whether [file]'s absolute path holds whitespace, which the argfile format cannot carry.
+	 *
+	 * @param file one resource input, named by its absolute path in the argv either way.
+	 * @return true when the path must be passed inline as its own `-R` argument.
+	 */
+	private fun hasWhitespace(file: File): Boolean = file.absolutePath.any { it.isWhitespace() }
+
 	/**
 	 * Checks that [linkedApk] actually contains a resource table before it ships as the
 	 * payload - a missing entry means aapt2 produced malformed output despite exit 0. Entry

quickbuild/daemon/src/test/kotlin/org/appdevforall/cotg/quickbuild/daemon/compile/JavaCompileStepTest.kt

@@ -75,4 +75,17 @@ class JavaCompileStepTest {
 		assertThat(result.diagnostics.map { it.severity }).doesNotContain(Diagnostic.Severity.ERROR)
 		assertThat(result.diagnostics.any { it.line == null && it.column == null }).isTrue()
 	}
+
+	@Test
+	fun `javac is pinned to the same release kotlinc targets`() {
+		// The host JDK produces the same class file version either way, so a compile-and-read
+		// test cannot fail if the flag is dropped; the argv is where it is observable.
+		val options = JavaCompileStep.javacOptions(listOf(File(tempDir, "dep.jar")), outputDir())
+
+		val release = options.indexOf("--release")
+		assertThat(release).isAtLeast(0)
+		assertThat(options[release + 1]).isEqualTo(IncrementalCompiler.JVM_TARGET)
+		// Annotation processors belong to the full Gradle build, not this one.
+		assertThat(options).contains("-proc:none")
+	}
 }

quickbuild/daemon/src/test/kotlin/org/appdevforall/cotg/quickbuild/daemon/res/Aapt2LinkTest.kt

@@ -391,6 +391,46 @@ class Aapt2LinkTest {
 		assertThat(lines.last()).isEqualTo(projectFlat.absolutePath)
 	}
 
+	@Test
+	fun `a resource path containing a space keeps every input inline`() {
+		// The argfile format splits on whitespace and has no escape for it, so one space in
+		// one path silently truncates that input and every later one. The project directory
+		// reaches these paths unsanitised and the default new project is "My Application",
+		// which makes this the common case rather than an odd one.
+		val link = Aapt2Link(File(tempDir, "aapt2"), File(tempDir, "android.jar"))
+		val spaced = File(tempDir, "My Application/merged_res")
+		val libraryResources = (1..Aapt2Link.ARGFILE_THRESHOLD + 5).map { File(spaced, "r$it.arsc.flat") }
+
+		val arguments =
+			link.buildLinkArguments(
+				linkedApk = File(workDir, "linked-res.apk"),
+				manifest = manifest,
+				flatFiles = emptyList(),
+				stableIds = null,
+				libraryResources = libraryResources,
+			)
+
+		assertThat(arguments.count { it == "-R" }).isEqualTo(libraryResources.size)
+		assertThat(arguments.filter { it.startsWith("@") }).isEmpty()
+		assertThat(arguments).containsAtLeastElementsIn(libraryResources.map { it.absolutePath })
+	}
+
+	@Test
+	fun `the inline path clears an argfile a previous link left behind`() {
+		val link = Aapt2Link(File(tempDir, "aapt2"), File(tempDir, "android.jar"))
+		val stale = File(workDir, Aapt2Link.ARGFILE_NAME).apply { writeText("/stale/r1.arsc.flat") }
+
+		link.buildLinkArguments(
+			linkedApk = File(workDir, "linked-res.apk"),
+			manifest = manifest,
+			flatFiles = listOf(File(tempDir, "compiled/values_strings.arsc.flat")),
+			stableIds = null,
+			libraryResources = emptyList(),
+		)
+
+		assertThat(stale.exists()).isFalse()
+	}
+
 	@Test
 	@EnabledIf("org.appdevforall.cotg.quickbuild.daemon.TestSdk#aapt2ToolchainAvailable")
 	fun `relink links through an @argfile when the flat count crosses the threshold`() {