Merge pull request #521 from jkloetzke/track-updates

Run changed checkoutScript in build-only mode

Author: jkloetzke

doc/manual/policies.rst

@@ -563,3 +563,30 @@ Old behavior
 New behavior
    Bob checks if the ``commit`` and / or ``tag`` is on the configured ``branch`` and
    performs a checkout of the ``commit`` on a local ``branch``.
+
+fixImportScmVariant
+~~~~~~~~~~~~~~~~~~~
+
+Introduced in: 0.23
+
+Bob uses the concept of a :term:`Variant-Id` to track *how* a package is built.
+This includes the sub-directory in which a particular SCM is checked out. So if
+the ``dir`` attribute of an SCM changes, the respective Variant-Id of the
+package changes too. Bob versions before 0.23 contained a bug where the ``dir``
+attribute of an ``import`` SCM was not included in the Variant-Id calculation.
+This can cause build failures or wrongly used binary artifacts if just the
+``dir`` attribute of an ``import`` SCM is changed.
+
+Fixing the bug will affect the :term:`Variant-Id` of all packages that use an
+``import`` SCM. This implies that binary artifacts of such packages will need
+to be built again. It also transitively affects packages that depend on
+packages that utilize an ``import`` SCM.
+
+Old behavior
+   Changes to the ``dir`` attribute of an ``import`` SCM do not cause rebuilds
+   of the affected package. Wrong sharing of binary artifacts for such packages
+   may occur.
+
+New behavior
+   Changes to the ``dir`` attribute of an ``import`` SCM behave the same as for
+   any other SCM.

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/input.py

@@ -2985,6 +2985,7 @@ class RecipeSet:
                 schema.Optional('scmIgnoreUser') : bool,
                 schema.Optional('pruneImportScm') : bool,
                 schema.Optional('gitCommitOnBranch') : bool,
+                schema.Optional('fixImportScmVariant') : bool,
             },
             error="Invalid policy specified! Maybe your Bob is too old?"
         ),
@@ -3112,6 +3113,11 @@ def __init__(self):
                 InfoOnce("gitCommitOnBranch policy not set. Will not check if commit / tag is on configured branch.",
                     help="See http://bob-build-tool.readthedocs.io/en/latest/manual/policies.html#gitcommitonbranch for more information.")
             ),
+            'fixImportScmVariant' : (
+                "0.22.1.dev34",
+                InfoOnce("fixImportScmVariant policy not set. Recipe variant calculation w/ import SCM is boguous.",
+                    help="See http://bob-build-tool.readthedocs.io/en/latest/manual/policies.html#fiximportscmvariant for more information.")
+            ),
         }
         self.__buildHooks = {}
         self.__sandboxOpts = {}

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):
 
@@ -549,6 +564,7 @@ def fromRecipeSet(cls, recipeSet):
             'tidyUrlScm' : recipeSet.getPolicy('tidyUrlScm'),
             'sandboxFingerprints' : recipeSet.getPolicy('sandboxFingerprints'),
             'gitCommitOnBranch' : recipeSet.getPolicy('gitCommitOnBranch'),
+            'fixImportScmVariant' : recipeSet.getPolicy('fixImportScmVariant'),
         }
         self.__data['archiveSpec'] = recipeSet.archiveSpec()
         self.__data['envWhiteList'] = sorted(recipeSet.envWhiteList())

pym/bob/scm/__init__.py

@@ -51,6 +51,7 @@ def getScm(spec, overrides=[], recipeSet=None):
     elif scm == "import":
         return ImportScm(spec, overrides,
             recipeSet and recipeSet.getPolicy('pruneImportScm'),
+            recipeSet and recipeSet.getPolicy('fixImportScmVariant'),
             recipeSet and recipeSet.getProjectRoot())
     elif scm == "svn":
         return SvnScm(spec, overrides)

pym/bob/scm/imp.py

@@ -127,13 +127,14 @@ class ImportScm(Scm):
 
     SCHEMA = schema.Schema({**__SCHEMA, **DEFAULTS})
 
-    def __init__(self, spec, overrides=[], pruneDefault=None, projectRoot=""):
+    def __init__(self, spec, overrides=[], pruneDefault=None, fixDigestBug=False, projectRoot=""):
         super().__init__(spec, overrides)
         self.__url = spec["url"]
         self.__dir = spec.get("dir", ".")
         self.__prune = spec.get("prune", pruneDefault or False)
         self.__data = spec.get("__data")
         self.__projectRoot = spec.get("__projectRoot", projectRoot)
+        self.__fixDigestBug = fixDigestBug
 
     def getProperties(self, isJenkins):
         ret = super().getProperties(isJenkins)
@@ -162,7 +163,10 @@ async def invoke(self, invoker):
             unpackTree(self.__data, dest)
 
     def asDigestScript(self):
-        return self.__url
+        if self.__fixDigestBug:
+            return self.__url + " " + self.__dir
+        else:
+            return self.__url
 
     def getDirectory(self):
         return self.__dir

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: |