Revert "Revert "Modify symlink_forest to rerun when soong_build has changed.""

This reverts commit aa5cc2cd6a.

Reason for revert: Fixed the bug in the original draft.
There was an error in how we compared the file's MTime to the
file's contents.

Bug: 300288299
Test: presubmits
Test: build/soong/tests/run_integration_tests.sh

Change-Id: I9e53432c0842c0b9fc13fe20d30ce9af37640c7f

bp2build/symlink_forest.go

@@ -22,7 +22,6 @@ import (
 	"regexp"
 	"sort"
 	"strconv"
-	"strings"
 	"sync"
 	"sync/atomic"
 
@@ -32,19 +31,12 @@ import (
 )
 
 // A tree structure that describes what to do at each directory in the created
-// symlink tree. Currently it is used to enumerate which files/directories
+// symlink tree. Currently, it is used to enumerate which files/directories
 // should be excluded from symlinking. Each instance of "node" represents a file
 // or a directory. If excluded is true, then that file/directory should be
 // excluded from symlinking. Otherwise, the node is not excluded, but one of its
 // descendants is (otherwise the node in question would not exist)
 
-// This is a version int written to a file called symlink_forest_version at the root of the
-// symlink forest. If the version here does not match the version in the file, then we'll
-// clean the whole symlink forest and recreate it. This number can be bumped whenever there's
-// an incompatible change to the forest layout or a bug in incrementality that needs to be fixed
-// on machines that may still have the bug present in their forest.
-const symlinkForestVersion = 2
-
 type instructionsNode struct {
 	name     string
 	excluded bool // If false, this is just an intermediate node
@@ -193,7 +185,7 @@ func symlinkIntoForest(topdir, dst, src string) uint64 {
 	srcPath := shared.JoinPath(topdir, src)
 	dstPath := shared.JoinPath(topdir, dst)
 
-	// Check if a symlink already exists.
+	// Check whether a symlink already exists.
 	if dstInfo, err := os.Lstat(dstPath); err != nil {
 		if !os.IsNotExist(err) {
 			fmt.Fprintf(os.Stderr, "Failed to lstat '%s': %s", dst, err)
@@ -240,44 +232,49 @@ func isDir(path string, fi os.FileInfo) bool {
 	return false
 }
 
-// maybeCleanSymlinkForest will remove the whole symlink forest directory if the version recorded
+// Returns the mtime of the soong_build binary to determine whether we should
-// in the symlink_forest_version file is not equal to symlinkForestVersion.
+// force symlink_forest to re-execute
-func maybeCleanSymlinkForest(topdir, forest string, verbose bool) error {
+func getSoongBuildMTime() (int64, error) {
-	versionFilePath := shared.JoinPath(topdir, forest, "symlink_forest_version")
+	binaryPath, err := os.Executable()
-	versionFileContents, err := os.ReadFile(versionFilePath)
+	if err != nil {
-	if err != nil && !os.IsNotExist(err) {
+		return 0, err
-		return err
 	}
-	versionFileString := strings.TrimSpace(string(versionFileContents))
+
-	symlinkForestVersionString := strconv.Itoa(symlinkForestVersion)
+	info, err := os.Stat(binaryPath)
-	if err != nil || versionFileString != symlinkForestVersionString {
+	if err != nil {
-		if verbose {
+		return 0, err
-			fmt.Fprintf(os.Stderr, "Old symlink_forest_version was %q, current is %q. Cleaning symlink forest before recreating...\n", versionFileString, symlinkForestVersionString)
-		}
-		err = os.RemoveAll(shared.JoinPath(topdir, forest))
-		if err != nil {
-			return err
-		}
 	}
-	return nil
+
+	return info.ModTime().UnixMilli(), nil
 }
 
-// maybeWriteVersionFile will write the symlink_forest_version file containing symlinkForestVersion
+// cleanSymlinkForest will remove the whole symlink forest directory
-// if it doesn't exist already. If it exists we know it must contain symlinkForestVersion because
+func cleanSymlinkForest(topdir, forest string) error {
-// we checked for that already in maybeCleanSymlinkForest
+	return os.RemoveAll(shared.JoinPath(topdir, forest))
-func maybeWriteVersionFile(topdir, forest string) error {
+}
-	versionFilePath := shared.JoinPath(topdir, forest, "symlink_forest_version")
+
-	_, err := os.Stat(versionFilePath)
+// This returns whether symlink forest should clean and replant symlinks.
+// It compares the mtime of this executable with the mtime of the last-run
+// soong_build binary. If they differ, then we should clean and replant.
+func shouldCleanSymlinkForest(topdir string, forest string, soongBuildMTime int64) (bool, error) {
+	mtimeFilePath := shared.JoinPath(topdir, forest, "soong_build_mtime")
+	mtimeFileContents, err := os.ReadFile(mtimeFilePath)
 	if err != nil {
-		if !os.IsNotExist(err) {
+		if os.IsNotExist(err) {
-			return err
+			// This is likely the first time this has run with this functionality - clean away!
-		}
+			return true, nil
-		err = os.WriteFile(versionFilePath, []byte(strconv.Itoa(symlinkForestVersion)+"\n"), 0666)
+		} else {
-		if err != nil {
+			return false, err
-			return err
 		}
 	}
-	return nil
+	return strconv.FormatInt(soongBuildMTime, 10) != string(mtimeFileContents), nil
+}
+
+func writeSoongBuildMTimeFile(topdir, forest string, mtime int64) error {
+	mtimeFilePath := shared.JoinPath(topdir, forest, "soong_build_mtime")
+	contents := []byte(strconv.FormatInt(mtime, 10))
+
+	return os.WriteFile(mtimeFilePath, contents, 0666)
 }
 
 // Recursively plants a symlink forest at forestDir. The symlink tree will
@@ -473,12 +470,26 @@ func PlantSymlinkForest(verbose bool, topdir string, forest string, buildFiles s
 		symlinkCount: atomic.Uint64{},
 	}
 
-	err := maybeCleanSymlinkForest(topdir, forest, verbose)
+	// Check whether soong_build has been modified since the last run
+	soongBuildMTime, err := getSoongBuildMTime()
 	if err != nil {
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(1)
 	}
 
+	shouldClean, err := shouldCleanSymlinkForest(topdir, forest, soongBuildMTime)
+
+	if err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		os.Exit(1)
+	} else if shouldClean {
+		err = cleanSymlinkForest(topdir, forest)
+		if err != nil {
+			fmt.Fprintln(os.Stderr, err)
+			os.Exit(1)
+		}
+	}
+
 	instructions := instructionsFromExcludePathList(exclude)
 	go func() {
 		context.wg.Add(1)
@@ -491,11 +502,10 @@ func PlantSymlinkForest(verbose bool, topdir string, forest string, buildFiles s
 		deps = append(deps, dep)
 	}
 
-	err = maybeWriteVersionFile(topdir, forest)
+	err = writeSoongBuildMTimeFile(topdir, forest, soongBuildMTime)
 	if err != nil {
 		fmt.Fprintln(os.Stderr, err)
 		os.Exit(1)
 	}
-
 	return deps, context.mkdirCount.Load(), context.symlinkCount.Load()
 }

tests/run_integration_tests.sh

@@ -10,6 +10,7 @@ TOP="$(readlink -f "$(dirname "$0")"/../../..)"
 "$TOP/build/soong/tests/persistent_bazel_test.sh"
 "$TOP/build/soong/tests/soong_test.sh"
 "$TOP/build/soong/tests/stale_metrics_files_test.sh"
+"$TOP/build/soong/tests/symlink_forest_rerun_test.sh"
 "$TOP/prebuilts/build-tools/linux-x86/bin/py3-cmd" "$TOP/build/bazel/ci/rbc_dashboard.py" aosp_arm64-userdebug
 
 # The following tests build against the full source tree and don't rely on the

tests/symlink_forest_rerun_test.sh

@@ -0,0 +1,43 @@
+#!/bin/bash -eu
+
+set -o pipefail
+
+# Tests that symlink forest will replant if soong_build has changed
+# Any change to the build system should trigger a rerun
+
+source "$(dirname "$0")/lib.sh"
+
+function test_symlink_forest_reruns {
+  setup
+
+  mkdir -p a
+  touch a/g.txt
+  cat > a/Android.bp <<'EOF'
+filegroup {
+    name: "g",
+    srcs: ["g.txt"],
+  }
+EOF
+
+  run_soong g
+
+  mtime=`cat out/soong/workspace/soong_build_mtime`
+  # rerun with no changes - ensure that it hasn't changed
+  run_soong g
+  newmtime=`cat out/soong/workspace/soong_build_mtime`
+  if [[ ! "$mtime" == "$mtime" ]]; then
+     fail "symlink forest reran when it shouldn't have"
+  fi
+
+  # change exit codes to force a soong_build rebuild.
+  sed -i 's/os.Exit(1)/os.Exit(2)/g' build/soong/bp2build/symlink_forest.go
+
+  run_soong g
+  newmtime=`cat out/soong/workspace/soong_build_mtime`
+  if [[ "$mtime" == "$newmtime" ]]; then
+     fail "symlink forest did not rerun when it should have"
+  fi
+
+}
+
+scan_and_run_tests