build: track state for build-only checkout step updates

The tracking of recipe changes was not sufficient for build-only updates
of checkouts that involve the import-SCM or recipes that utilize
checkoutUpdateIf. We were over-cautious and prevented running these
updates if *anything* related to the checkout step changed.

To make Bobs behaviour more predictable, track the related state more
closely. Updates of checkouts in build-only mode are now only prevented
if an import-SCM is changed. On the other side, changes to the
checkoutScript will trigger the update in build-only mode even if the
script is deterministic.

Fixes #506.

Author: jkloetzke

pym/bob/builder.py

@@ -65,17 +65,67 @@ def hashWorkspace(step):
     return hashDirectory(step.getStoragePath(),
         os.path.join(os.path.dirname(step.getWorkspacePath()), "cache.bin"))
 
+CHECKOUT_STATE_VARIANT_ID = None    # Key in checkout directory state for step variant-id
+CHECKOUT_STATE_BUILD_ONLY = 1       # Key for checkout state of build-only builds
+
+# Keys in checkout getDirectoryState that are not directories
+CHECKOUT_NON_DIR_KEYS = {CHECKOUT_STATE_VARIANT_ID, CHECKOUT_STATE_BUILD_ONLY}
+
 def compareDirectoryState(left, right):
     """Compare two directory states while ignoring the SCM specs.
 
     The SCM specs might change even though the digest stays the same (e.g. the
     URL changes but the commit id stays the same).  This function filters the
     spec to detect real changes.
+
+    It also compares the CHECKOUT_STATE_VARIANT_ID to detect recipe changes. In
+    contrast, the CHECKOUT_STATE_BUILD_ONLY sub-state is ignored as this is
+    only relevant for build-only builds that have their dedicated functions
+    below.
     """
-    left  = { d : v[0] for d, v in left.items()  }
-    right = { d : v[0] for d, v in right.items() }
+    left  = { d : v[0] for d, v in left.items()  if d != CHECKOUT_STATE_BUILD_ONLY }
+    right = { d : v[0] for d, v in right.items() if d != CHECKOUT_STATE_BUILD_ONLY }
     return left == right
 
+def checkoutsFromState(state):
+    """Return only the tuples related to SCMs from the checkout state."""
+    return [ (k, v) for k, v in state.items() if k not in CHECKOUT_NON_DIR_KEYS ]
+
+def checkoutBuildOnlyState(checkoutStep, inputHashes):
+    """Obtain state for build-only checkout updates.
+
+    The assumption is that we can run updates as long as all local SCMs stayed
+    the same. As this is just for build-only builds, we can assume that
+    dependencies have been setup correctly (direct deps, tools).
+
+    Because of the fixImportScmVariant bug, we include the directory too. This
+    should not have been necessary otherwise. Needs to return a tuple because
+    that's what is expected in the checkout SCM state (will be silently
+    upgraded to a tuple by BobState otherwise).
+    """
+    return ("\n".join("{} {}".format(scm.getDirectory(), scm.asDigestScript())
+                      for scm in checkoutStep.getScmList()
+                      if scm.isLocal()),
+            checkoutStep.getUpdateScriptDigest(),
+            inputHashes)
+
+def checkoutBuildOnlyStateCompatible(left, right):
+    """Returns True if it's safe to run build-only checkout updates.
+
+    Current policy is to just require that local SCMs are compatible. A
+    different step variant-id, changed update script or dependency changes do
+    *not* prohibit an in-place update.
+    """
+    left = left.get(CHECKOUT_STATE_BUILD_ONLY, (None, None, None))[0]
+    right = right.get(CHECKOUT_STATE_BUILD_ONLY, (None, None, None))[0]
+    return left == right
+
+def checkoutBuildOnlyStateChanged(left, right):
+    """Returns True if the update script has changed"""
+    left = left.get(CHECKOUT_STATE_BUILD_ONLY, None)
+    right = right.get(CHECKOUT_STATE_BUILD_ONLY, None)
+    return left != right
+
 def dissectPackageInputState(oldInputBuildId):
     """Take a package step input hashes and convert them to a common
     representation.
@@ -1016,17 +1066,25 @@ async def _cookCheckoutStep(self, checkoutStep, depth):
         checkoutExecuted = False
         checkoutDigest = checkoutStep.getVariantId()
         checkoutState = checkoutStep.getScmDirectories().copy()
-        checkoutState[None] = (checkoutDigest, None)
+        checkoutState[CHECKOUT_STATE_VARIANT_ID] = (checkoutDigest, None)
+        checkoutState[CHECKOUT_STATE_BUILD_ONLY] = checkoutBuildOnlyState(checkoutStep, checkoutInputHashes)
         if self.__buildOnly and (BobState().getResultHash(prettySrcPath) is not None):
-            inputChanged = checkoutInputHashes != BobState().getInputHashes(prettySrcPath)
+            inputChanged = checkoutBuildOnlyStateChanged(checkoutState, oldCheckoutState)
             rehash = lambda: hashWorkspace(checkoutStep)
-            if not compareDirectoryState(checkoutState, oldCheckoutState):
+            if checkoutStep.mayUpdate(inputChanged, BobState().getResultHash(prettySrcPath), rehash):
+                if checkoutBuildOnlyStateCompatible(checkoutState, oldCheckoutState):
+                    with stepExec(checkoutStep, "UPDATE",
+                                  "{} {}".format(prettySrcPath, overridesString)) as a:
+                        await self._runShell(checkoutStep, "checkout", a, mode=InvocationMode.UPDATE)
+                    newCheckoutState = oldCheckoutState.copy()
+                    newCheckoutState[CHECKOUT_STATE_BUILD_ONLY] = checkoutState[CHECKOUT_STATE_BUILD_ONLY]
+                    BobState().setDirectoryState(prettySrcPath, newCheckoutState)
+                else:
+                    stepMessage(checkoutStep, "UPDATE", "WARNING: recipe changed - cannot update ({})"
+                        .format(prettySrcPath), WARNING)
+            elif not compareDirectoryState(checkoutState, oldCheckoutState):
                 stepMessage(checkoutStep, "CHECKOUT", "WARNING: recipe changed but skipped due to --build-only ({})"
                     .format(prettySrcPath), WARNING)
-            elif checkoutStep.mayUpdate(inputChanged, BobState().getResultHash(prettySrcPath), rehash):
-                with stepExec(checkoutStep, "UPDATE",
-                              "{} {}".format(prettySrcPath, overridesString)) as a:
-                    await self._runShell(checkoutStep, "checkout", a, mode=InvocationMode.UPDATE)
             else:
                 stepMessage(checkoutStep, "CHECKOUT", "skipped due to --build-only ({}) {}".format(prettySrcPath, overridesString),
                     SKIPPED, IMPORTANT)
@@ -1036,8 +1094,7 @@ async def _cookCheckoutStep(self, checkoutStep, depth):
 
             if self.__cleanCheckout:
                 # check state of SCMs and invalidate if the directory is dirty
-                for (scmDir, (scmDigest, scmSpec)) in oldCheckoutState.copy().items():
-                    if scmDir is None: continue
+                for (scmDir, (scmDigest, scmSpec)) in checkoutsFromState(oldCheckoutState):
                     if scmDigest != checkoutState.get(scmDir, (None, None))[0]: continue
                     if not os.path.exists(os.path.join(prettySrcPath, scmDir)): continue
                     if scmMap[scmDir].status(checkoutStep.getWorkspacePath()).dirty:
@@ -1050,8 +1107,8 @@ async def _cookCheckoutStep(self, checkoutStep, depth):
                 not compareDirectoryState(checkoutState, oldCheckoutState) or
                 (checkoutInputHashes != BobState().getInputHashes(prettySrcPath))):
                 # Switch or move away old or changed source directories
-                for (scmDir, (scmDigest, scmSpec)) in oldCheckoutState.copy().items():
-                    if (scmDir is not None) and (scmDigest != checkoutState.get(scmDir, (None, None))[0]):
+                for (scmDir, (scmDigest, scmSpec)) in checkoutsFromState(oldCheckoutState):
+                    if scmDigest != checkoutState.get(scmDir, (None, None))[0]:
                         scmPath = os.path.normpath(os.path.join(prettySrcPath, scmDir))
                         canSwitch = (scmDir in scmMap) and scmDigest and \
                                      scmSpec is not None and \
@@ -1087,8 +1144,8 @@ async def _cookCheckoutStep(self, checkoutStep, depth):
                 # workspace. Do it before we store the new SCM state to
                 # check again if the step is rerun.
                 if not checkoutStep.JENKINS:
-                    for scmDir in checkoutState.keys():
-                        if scmDir is None or scmDir == ".": continue
+                    for scmDir in [ k for k,v in checkoutsFromState(checkoutState)]:
+                        if scmDir == ".": continue
                         if scmDir in oldCheckoutState: continue
                         scmPath = os.path.normpath(os.path.join(prettySrcPath, scmDir))
                         if os.path.exists(scmPath):
@@ -1100,7 +1157,7 @@ async def _cookCheckoutStep(self, checkoutStep, depth):
                 # record the SCM directories as some checkouts might already
                 # succeeded before the step ultimately fails.
                 BobState().setDirectoryState(prettySrcPath,
-                    { d:s for (d,s) in checkoutState.items() if d is not None })
+                    { d:s for (d,s) in checkoutState.items() if d != CHECKOUT_STATE_VARIANT_ID })
 
                 # Forge checkout result before we run the step again.
                 # Normally the correct result is set directly after the

pym/bob/cmds/build/clean.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: GPL-3.0-or-later
 
-from ...builder import LocalBuilder
+from ...builder import LocalBuilder, checkoutsFromState
 from ...input import RecipeSet
 from ...scm import getScm, ScmTaint, ScmStatus
 from ...share import getShare
@@ -55,7 +55,6 @@ def walk(package):
     return paths
 
 def checkSCM(workspace, scmDir, scmSpec, verbose):
-    if scmDir is None: return True
     if scmSpec is not None:
         status = getScm(scmSpec).status(workspace)
     else:
@@ -78,7 +77,7 @@ def checkSCM(workspace, scmDir, scmSpec, verbose):
 def checkRegularSource(workspace, verbose):
     ret = True
     state = BobState().getDirectoryState(workspace, True)
-    for (scmDir, (scmDigest, scmSpec)) in state.items():
+    for (scmDir, (scmDigest, scmSpec)) in checkoutsFromState(state):
         if not checkSCM(workspace, scmDir, scmSpec, verbose):
             ret = False
 

pym/bob/cmds/build/status.py

@@ -3,7 +3,7 @@
 #
 # SPDX-License-Identifier: GPL-3.0-or-later
 
-from ...builder import LocalBuilder
+from ...builder import LocalBuilder, checkoutsFromState
 from ...input import RecipeSet
 from ...scm import getScm, ScmTaint, ScmStatus
 from ...state import BobState
@@ -112,8 +112,7 @@ def __showCheckoutStep(self, pp, checkoutStep):
         # First scan old checkout state. This is what the user is most
         # interested in. The recipe might have changed compared to the
         # persisted state!
-        for (scmDir, (scmDigest, scmSpec)) in oldCheckoutState.items():
-            if scmDir is None: continue # checkoutScript state -> ignored
+        for (scmDir, (scmDigest, scmSpec)) in checkoutsFromState(oldCheckoutState):
             if not os.path.exists(os.path.join(workspace, scmDir)): continue
 
             if scmDigest == checkoutState.get(scmDir, (None, None))[0]:
@@ -208,10 +207,10 @@ def showAllDirs(self, showAttic):
                 BobState().delDirectoryState(workspace)
                 continue
 
-            # Upgrade from old format without scmSpec. Drop None dir.
+            # Upgrade from old format without scmSpec.
             dirState = sorted(
                 (dir, state) if isinstance(state, tuple) else (dir, (state, None))
-                for dir,state in dirState.items() if dir is not None)
+                for dir,state in checkoutsFromState(dirState))
             for (scmDir, (scmDigest, scmSpec)) in dirState:
                 scmDir = os.path.join(workspace, scmDir)
                 if scmSpec is not None:

pym/bob/intermediate.py

@@ -298,7 +298,7 @@ def mayUpdate(self, inputChanged, oldHash, rehash):
             return True
         if not self.getUpdateScript():
             return False
-        if not self.isUpdateDeterministic():
+        if not self.isUpdateDeterministic() or inputChanged:
             return True
         return rehash() != oldHash
 
@@ -389,6 +389,21 @@ def calcLiveBuildId(self):
             h.update(liveBId)
         return h.digest()
 
+    def getUpdateScriptDigest(self):
+        """Return a digest that tracks relevant changes to the update script behaviour"""
+        h = hashlib.sha1()
+        script = self.getUpdateScript()
+        if script:
+            h.update(struct.pack("<I", len(script)))
+            h.update(script.encode("utf8"))
+        else:
+            h.update(b'\x00\x00\x00\x00')
+        h.update(struct.pack("<I", len(self.__data['digestEnv'])))
+        for (key, val) in sorted(self.__data['digestEnv'].items()):
+            h.update(struct.pack("<II", len(key), len(val)))
+            h.update((key+val).encode('utf8'))
+        return h.digest()
+
 
 class PackageIR(AbstractIR):
 

pym/bob/state.py

@@ -289,8 +289,9 @@ class _BobState():
     #  5 -> 6: build state stores predicted live-build-ids too
     #  6 -> 7: amended directory state for source steps, store attic directories
     #  7 -> 8: normalize attic directories
+    #  8 -> 9: checkout directory state tracks import scm / update script state
     MIN_VERSION = 2
-    CUR_VERSION = 8
+    CUR_VERSION = 9
 
     VERSION_SINCE_ATTIC_TRACKED = 7
 

test/black-box/checkout-update/recipes/deterministic-root.yaml

@@ -2,6 +2,7 @@ root: True
 inherit: [update, no-update]
 checkoutDeterministic: True
 checkoutUpdateIf: True
+checkoutVars: [DUMMY]
 checkoutScript: |
     bumpCounter "root.txt"
 buildScript: |

test/black-box/checkout-update/run.sh

@@ -25,7 +25,25 @@ read -r CNT_ROOT_NEW < dev/src/deterministic-root/1/workspace/root.txt
 read -r CNT_NO_UPDATE_NEW < dev/src/deterministic-root/1/workspace/no-update.txt
 expect_not_equal "$CNT_ROOT_BASE" "$CNT_ROOT_NEW"
 expect_equal "$CNT_NO_UPDATE_BASE" "$CNT_NO_UPDATE_NEW"
+CNT_ROOT_BASE="$CNT_ROOT_NEW"
 
+# Changing the checkout script will run the deterministic checkout again
+run_bob dev deterministic-root --build-only -DDUMMY=foo
+read -r CNT_ROOT_NEW < dev/src/deterministic-root/1/workspace/root.txt
+read -r CNT_NO_UPDATE_NEW < dev/src/deterministic-root/1/workspace/no-update.txt
+expect_not_equal "$CNT_ROOT_BASE" "$CNT_ROOT_NEW"
+expect_equal "$CNT_NO_UPDATE_BASE" "$CNT_NO_UPDATE_NEW"
+CNT_ROOT_BASE="$CNT_ROOT_NEW"
+
+# Running with the same checkout script won't run the deterministic checkout
+# again
+run_bob dev deterministic-root --build-only -DDUMMY=foo
+read -r CNT_ROOT_NEW < dev/src/deterministic-root/1/workspace/root.txt
+read -r CNT_NO_UPDATE_NEW < dev/src/deterministic-root/1/workspace/no-update.txt
+expect_equal "$CNT_ROOT_BASE" "$CNT_ROOT_NEW"
+expect_equal "$CNT_NO_UPDATE_BASE" "$CNT_NO_UPDATE_NEW"
+
+### indeterministic tests #####################################################
 
 # checkout sources and remember state of indeterministic checkout
 run_bob dev indeterministic-root --build-only
@@ -48,3 +66,4 @@ read -r CNT_ROOT_NEW < dev/src/indeterministic-root/1/workspace/root.txt
 read -r CNT_NO_UPDATE_NEW < dev/src/indeterministic-root/1/workspace/no-update.txt
 expect_not_equal "$CNT_ROOT_BASE" "$CNT_ROOT_NEW"
 expect_equal "$CNT_NO_UPDATE_BASE" "$CNT_NO_UPDATE_NEW"
+CNT_ROOT_BASE="$CNT_ROOT_NEW"

test/black-box/import-scm/root.yaml

@@ -2,5 +2,6 @@ root: True
 checkoutSCM:
     scm: import
     url: src
+    dir: "${IMPORT_DIR:-.}"
 buildScript: cp -a "$1/"* .
 packageScript: cp -a "$1/"* .

test/black-box/import-scm/run.sh

@@ -9,7 +9,7 @@ cleanup
 check_result()
 {
 	local DATA
-	read -r DATA < "$prj/output/result.txt"
+	read -r DATA < "$prj/output/${2:-.}/result.txt"
 	[[ "$1" = "$DATA" ]] || { echo "Mismatch: $1 <> $DATA" ; exit 1; }
 }
 
@@ -38,3 +38,19 @@ check_result "2"
 echo "3" > "$prj/src/result.txt"
 run_bob -C "$prj" dev --destination output root --build-only
 check_result "3"
+
+# The import SCM is even updated when the checkoutScript changes!
+# But the checkoutScript itself must not run.
+echo 'checkoutScript: "rm -f result.txt"' >>"$prj/recipes/root.yaml"
+echo "4" > "$prj/src/result.txt"
+run_bob -C "$prj" dev --destination output root --build-only
+check_result "4"
+
+# But when changing the import SCM itself, Bob will refuse to update the
+# workspace on build-only builds. This can only be rectified by running without
+# --build-only.
+echo "5" > "$prj/src/result.txt"
+run_bob -C "$prj" dev --destination output root -DIMPORT_DIR=sub --build-only
+check_result "4"
+run_bob -C "$prj" dev --destination output root -DIMPORT_DIR=sub
+check_result "5" "sub"