soong_ui ninja.go: comment and refactor.

On top of improving the comments, I also refactored the status checker
to simplify and clarify its use case to be a ninja stuckness checker,
since it doesn't appear to have other purposes.

Test: TH presubmit
Bug: b/173474588
Change-Id: I2cf51a1ebf16071a24a1c13c06c7b1adf60256de

ui/build/ninja.go

@@ -27,10 +27,16 @@ import (
 	"android/soong/ui/status"
 )
 
+// Constructs and runs the Ninja command line with a restricted set of
+// environment variables. It's important to restrict the environment Ninja runs
+// for hermeticity reasons, and to avoid spurious rebuilds.
 func runNinja(ctx Context, config Config) {
 	ctx.BeginTrace(metrics.PrimaryNinja, "ninja")
 	defer ctx.EndTrace()
 
+	// Sets up the FIFO status updater that reads the Ninja protobuf output, and
+	// translates it to the soong_ui status output, displaying real-time
+	// progress of the build.
 	fifo := filepath.Join(config.OutDir(), ".ninja_fifo")
 	nr := status.NewNinjaReader(ctx, ctx.Status.StartTool(), fifo)
 	defer nr.Close()
@@ -64,8 +70,12 @@ func runNinja(ctx Context, config Config) {
 		"-w", "missingdepfile=err")
 
 	cmd := Command(ctx, config, "ninja", executable, args...)
+
+	// Set up the nsjail sandbox Ninja runs in.
 	cmd.Sandbox = ninjaSandbox
 	if config.HasKatiSuffix() {
+		// Reads and executes a shell script from Kati that sets/unsets the
+		// environment Ninja runs in.
 		cmd.Environment.AppendFromKati(config.KatiEnvFile())
 	}
 
@@ -78,8 +88,8 @@ func runNinja(ctx Context, config Config) {
 		cmd.Args = append(cmd.Args, strings.Fields(extra)...)
 	}
 
-	logPath := filepath.Join(config.OutDir(), ".ninja_log")
 	ninjaHeartbeatDuration := time.Minute * 5
+	// Get the ninja heartbeat interval from the environment before it's filtered away later.
 	if overrideText, ok := cmd.Environment.Get("NINJA_HEARTBEAT_INTERVAL"); ok {
 		// For example, "1m"
 		overrideDuration, err := time.ParseDuration(overrideText)
@@ -88,18 +98,22 @@ func runNinja(ctx Context, config Config) {
 		}
 	}
 
-	// Filter the environment, as ninja does not rebuild files when environment variables change.
+	// Filter the environment, as ninja does not rebuild files when environment
+	// variables change.
 	//
-	// Anything listed here must not change the output of rules/actions when the value changes,
-	// otherwise incremental builds may be unsafe. Vars explicitly set to stable values
-	// elsewhere in soong_ui are fine.
+	// Anything listed here must not change the output of rules/actions when the
+	// value changes, otherwise incremental builds may be unsafe. Vars
+	// explicitly set to stable values elsewhere in soong_ui are fine.
 	//
-	// For the majority of cases, either Soong or the makefiles should be replicating any
-	// necessary environment variables in the command line of each action that needs it.
+	// For the majority of cases, either Soong or the makefiles should be
+	// replicating any necessary environment variables in the command line of
+	// each action that needs it.
 	if cmd.Environment.IsEnvTrue("ALLOW_NINJA_ENV") {
 		ctx.Println("Allowing all environment variables during ninja; incremental builds may be unsafe.")
 	} else {
 		cmd.Environment.Allow(append([]string{
+			// Set the path to a symbolizer (e.g. llvm-symbolizer) so ASAN-based
+			// tools can symbolize crashes.
 			"ASAN_SYMBOLIZER_PATH",
 			"HOME",
 			"JAVA_HOME",
@@ -108,14 +122,19 @@ func runNinja(ctx Context, config Config) {
 			"OUT_DIR",
 			"PATH",
 			"PWD",
+			// https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE
 			"PYTHONDONTWRITEBYTECODE",
 			"TMPDIR",
 			"USER",
 
 			// TODO: remove these carefully
+			// Options for the address sanitizer.
 			"ASAN_OPTIONS",
+			// The list of Android app modules to be built in an unbundled manner.
 			"TARGET_BUILD_APPS",
+			// The variant of the product being built. e.g. eng, userdebug, debug.
 			"TARGET_BUILD_VARIANT",
+			// The product name of the product being built, e.g. aosp_arm, aosp_flame.
 			"TARGET_PRODUCT",
 			// b/147197813 - used by art-check-debug-apex-gen
 			"EMMA_INSTRUMENT_FRAMEWORK",
@@ -162,6 +181,7 @@ func runNinja(ctx Context, config Config) {
 	cmd.Environment.Set("DIST_DIR", config.DistDir())
 	cmd.Environment.Set("SHELL", "/bin/bash")
 
+	// Print the environment variables that Ninja is operating in.
 	ctx.Verboseln("Ninja environment: ")
 	envVars := cmd.Environment.Environ()
 	sort.Strings(envVars)
@@ -169,17 +189,21 @@ func runNinja(ctx Context, config Config) {
 		ctx.Verbosef("  %s", envVar)
 	}
 
-	// Poll the ninja log for updates; if it isn't updated enough, then we want to show some diagnostics
+	// Poll the Ninja log for updates regularly based on the heartbeat
+	// frequency. If it isn't updated enough, then we want to surface the
+	// possibility that Ninja is stuck, to the user.
 	done := make(chan struct{})
 	defer close(done)
 	ticker := time.NewTicker(ninjaHeartbeatDuration)
 	defer ticker.Stop()
-	checker := &statusChecker{}
+	ninjaChecker := &ninjaStucknessChecker{
+		logPath: filepath.Join(config.OutDir(), ".ninja_log"),
+	}
 	go func() {
 		for {
 			select {
 			case <-ticker.C:
-				checker.check(ctx, config, logPath)
+				ninjaChecker.check(ctx, config)
 			case <-done:
 				return
 			}
@@ -190,37 +214,36 @@ func runNinja(ctx Context, config Config) {
 	cmd.RunAndStreamOrFatal()
 }
 
-type statusChecker struct {
-	prevTime time.Time
+// A simple struct for checking if Ninja gets stuck, using timestamps.
+type ninjaStucknessChecker struct {
+	logPath     string
+	prevModTime time.Time
 }
 
-func (c *statusChecker) check(ctx Context, config Config, pathToCheck string) {
-	info, err := os.Stat(pathToCheck)
-	var newTime time.Time
+// Check that a file has been modified since the last time it was checked. If
+// the mod time hasn't changed, then assume that Ninja got stuck, and print
+// diagnostics for debugging.
+func (c *ninjaStucknessChecker) check(ctx Context, config Config) {
+	info, err := os.Stat(c.logPath)
+	var newModTime time.Time
 	if err == nil {
-		newTime = info.ModTime()
+		newModTime = info.ModTime()
 	}
-	if newTime == c.prevTime {
-		// ninja may be stuck
-		dumpStucknessDiagnostics(ctx, config, pathToCheck, newTime)
+	if newModTime == c.prevModTime {
+		// The Ninja file hasn't been modified since the last time it was
+		// checked, so Ninja could be stuck. Output some diagnostics.
+		ctx.Verbosef("ninja may be stuck; last update to %v was %v. dumping process tree...", c.logPath, newModTime)
+
+		// The "pstree" command doesn't exist on Mac, but "pstree" on Linux
+		// gives more convenient output than "ps" So, we try pstree first, and
+		// ps second
+		commandText := fmt.Sprintf("pstree -pal %v || ps -ef", os.Getpid())
+
+		cmd := Command(ctx, config, "dump process tree", "bash", "-c", commandText)
+		output := cmd.CombinedOutputOrFatal()
+		ctx.Verbose(string(output))
+
+		ctx.Verbosef("done\n")
 	}
-	c.prevTime = newTime
-}
-
-// dumpStucknessDiagnostics gets called when it is suspected that Ninja is stuck and we want to output some diagnostics
-func dumpStucknessDiagnostics(ctx Context, config Config, statusPath string, lastUpdated time.Time) {
-
-	ctx.Verbosef("ninja may be stuck; last update to %v was %v. dumping process tree...", statusPath, lastUpdated)
-
-	// The "pstree" command doesn't exist on Mac, but "pstree" on Linux gives more convenient output than "ps"
-	// So, we try pstree first, and ps second
-	pstreeCommandText := fmt.Sprintf("pstree -pal %v", os.Getpid())
-	psCommandText := "ps -ef"
-	commandText := pstreeCommandText + " || " + psCommandText
-
-	cmd := Command(ctx, config, "dump process tree", "bash", "-c", commandText)
-	output := cmd.CombinedOutputOrFatal()
-	ctx.Verbose(string(output))
-
-	ctx.Verbosef("done\n")
+	c.prevModTime = newModTime
 }