From aa5cc2cd6af5da7c65914500fc6e11c6a130060c Mon Sep 17 00:00:00 2001 From: Mark Dacek Date: Mon, 2 Oct 2023 23:40:33 +0000 Subject: [PATCH] Revert "Modify symlink_forest to rerun when soong_build has changed." This reverts commit 23a4120c57ada5dba31669db16281cd1e37045c1. Reason for revert: broke soong_integration Change-Id: I4d51841756675b3745244d23e13aefda0916614b --- bp2build/symlink_forest.go | 79 +++++++++++++++--------------- tests/run_integration_tests.sh | 1 - tests/symlink_forest_rerun_test.sh | 42 ---------------- 3 files changed, 40 insertions(+), 82 deletions(-) delete mode 100755 tests/symlink_forest_rerun_test.sh diff --git a/bp2build/symlink_forest.go b/bp2build/symlink_forest.go index 06e63ca4d..966b94a80 100644 --- a/bp2build/symlink_forest.go +++ b/bp2build/symlink_forest.go @@ -22,6 +22,7 @@ import ( "regexp" "sort" "strconv" + "strings" "sync" "sync/atomic" @@ -31,12 +32,19 @@ 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 @@ -185,7 +193,7 @@ func symlinkIntoForest(topdir, dst, src string) uint64 { srcPath := shared.JoinPath(topdir, src) dstPath := shared.JoinPath(topdir, dst) - // Check whether a symlink already exists. + // Check if 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) @@ -232,49 +240,44 @@ func isDir(path string, fi os.FileInfo) bool { return false } -// Returns the hash of the soong_build binary to determine whether we should -// force symlink_forest to re-execute -// This is similar to a version number increment - but that shouldn't be required -// for every update to this file -func getSoongBuildMTime() int64 { - binaryPath, err := os.Executable() - if err != nil { - fmt.Fprintf(os.Stderr, "error finding executable path %s\n", err) - os.Exit(1) - } - - info, err := os.Stat(binaryPath) - if err != nil { - fmt.Fprintf(os.Stderr, "error stating executable path %s\n", err) - } - - return info.ModTime().UnixMilli() -} - -// maybeCleanSymlinkForest will remove the whole symlink forest directory if the soong_build -// binary has changed since the last execution. -func maybeCleanSymlinkForest(topdir, forest string, verbose bool, soongBuildMTime int64) error { - mtimeFilePath := shared.JoinPath(topdir, forest, "soong_build_mtime") - mtimeFileContents, err := os.ReadFile(mtimeFilePath) +// maybeCleanSymlinkForest will remove the whole symlink forest directory if the version recorded +// in the symlink_forest_version file is not equal to symlinkForestVersion. +func maybeCleanSymlinkForest(topdir, forest string, verbose bool) error { + versionFilePath := shared.JoinPath(topdir, forest, "symlink_forest_version") + versionFileContents, err := os.ReadFile(versionFilePath) if err != nil && !os.IsNotExist(err) { return err } - - if string(soongBuildMTime) != string(mtimeFileContents) { + versionFileString := strings.TrimSpace(string(versionFileContents)) + symlinkForestVersionString := strconv.Itoa(symlinkForestVersion) + if err != nil || versionFileString != symlinkForestVersionString { + if verbose { + 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 } -func writeSoongBuildMTimeFile(topdir, forest string, mtime int64) error { - hashFilePath := shared.JoinPath(topdir, forest, "soong_build_mtime") - contents := []byte(strconv.FormatInt(mtime, 10)) - - return os.WriteFile(hashFilePath, contents, 0666) +// maybeWriteVersionFile will write the symlink_forest_version file containing symlinkForestVersion +// if it doesn't exist already. If it exists we know it must contain symlinkForestVersion because +// we checked for that already in maybeCleanSymlinkForest +func maybeWriteVersionFile(topdir, forest string) error { + versionFilePath := shared.JoinPath(topdir, forest, "symlink_forest_version") + _, err := os.Stat(versionFilePath) + if err != nil { + if !os.IsNotExist(err) { + return err + } + err = os.WriteFile(versionFilePath, []byte(strconv.Itoa(symlinkForestVersion)+"\n"), 0666) + if err != nil { + return err + } + } + return nil } // Recursively plants a symlink forest at forestDir. The symlink tree will @@ -470,10 +473,7 @@ func PlantSymlinkForest(verbose bool, topdir string, forest string, buildFiles s symlinkCount: atomic.Uint64{}, } - // Check whether soong_build has been modified since the last run - soongBuildMTime := getSoongBuildMTime() - - err := maybeCleanSymlinkForest(topdir, forest, verbose, soongBuildMTime) + err := maybeCleanSymlinkForest(topdir, forest, verbose) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) @@ -491,10 +491,11 @@ func PlantSymlinkForest(verbose bool, topdir string, forest string, buildFiles s deps = append(deps, dep) } - err = writeSoongBuildMTimeFile(topdir, forest, soongBuildMTime) + err = maybeWriteVersionFile(topdir, forest) if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } + return deps, context.mkdirCount.Load(), context.symlinkCount.Load() } diff --git a/tests/run_integration_tests.sh b/tests/run_integration_tests.sh index 234999376..6b9ff8b93 100755 --- a/tests/run_integration_tests.sh +++ b/tests/run_integration_tests.sh @@ -10,7 +10,6 @@ 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 diff --git a/tests/symlink_forest_rerun_test.sh b/tests/symlink_forest_rerun_test.sh deleted file mode 100755 index b704222e2..000000000 --- a/tests/symlink_forest_rerun_test.sh +++ /dev/null @@ -1,42 +0,0 @@ -#!/bin/bash -eu - -set -o pipefail - -# Tests that symlink_Forest will rerun if soong_build has schanged - -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