From 778fba764410051af62110a45f4e820438622ca5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:11:57 +0100 Subject: [PATCH 1/6] archive: replace live-build-id and fingerprint caches These caching data structures are prone to incorrect data e.g., if some checkout is not really deterministic. The old behaviour was to never replace the files which sets the very first execution result into stone. If that was incorrect, there are no tools to purge the wrong cache except deleting the whole binary archive. Instead, Bob now always uploads the live-build-id and fingerprint caches. These files are small so there should be no performance impact. But at least the cache will reflect the last build, giving the user the possibility to replace the wrong predictions with the right ones. --- pym/bob/archive.py | 89 ++++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 39 deletions(-) diff --git a/pym/bob/archive.py b/pym/bob/archive.py index f50a39e3..6b530d80 100644 --- a/pym/bob/archive.py +++ b/pym/bob/archive.py @@ -385,7 +385,7 @@ async def downloadPackage(self, step, buildId, audit, content, caches=[], def cachePackage(self, buildId, workspace): try: - return self._openUploadFile(buildId, ARTIFACT_SUFFIX) + return self._openUploadFile(buildId, ARTIFACT_SUFFIX, False) except ArtifactExistsError: return None except (ArtifactUploadError, OSError) as e: @@ -455,7 +455,7 @@ def _downloadLocalFile(self, key, suffix): # to prevent ugly backtraces when user presses ctrl+c. signal.signal(signal.SIGINT, signal.SIG_DFL) - def _openUploadFile(self, buildId, suffix): + def _openUploadFile(self, buildId, suffix, overwrite): raise ArtifactUploadError("not implemented") async def uploadPackage(self, step, buildId, audit, content, executor=None): @@ -483,7 +483,7 @@ def _uploadPackage(self, buildId, suffix, audit, content): signal.signal(signal.SIGINT, signal.default_int_handler) try: - with self._openUploadFile(buildId, suffix) as (name, fileobj): + with self._openUploadFile(buildId, suffix, False) as (name, fileobj): self._pack(name, fileobj, audit, content) except ArtifactExistsError: return ("skipped ({} exists in archive)".format(content), SKIPPED) @@ -516,10 +516,10 @@ def _uploadLocalFile(self, key, suffix, content): signal.signal(signal.SIGINT, signal.default_int_handler) try: - with self._openUploadFile(key, suffix) as (name, fileobj): + # Meta data file uploads overwrite their targets. Thus we should + # never see an ArtifactExistsError. + with self._openUploadFile(key, suffix, True) as (name, fileobj): writeFileOrHandle(name, fileobj, content) - except ArtifactExistsError: - return ("skipped (exists in archive)", SKIPPED) except (ArtifactUploadError, OSError) as e: if self.__ignoreErrors: return ("error ("+str(e)+")", ERROR) @@ -668,9 +668,9 @@ def _openDownloadFile(self, buildId, suffix): else: raise ArtifactNotFoundError() - def _openUploadFile(self, buildId, suffix): + def _openUploadFile(self, buildId, suffix, overwrite): (packageResultPath, packageResultFile) = self._getPath(buildId, suffix) - if os.path.isfile(packageResultFile): + if not overwrite and os.path.isfile(packageResultFile): raise ArtifactExistsError() # open temporary file in destination directory @@ -684,8 +684,7 @@ def _openUploadFile(self, buildId, suffix): os.umask(oldMask) return LocalArchiveUploader( NamedTemporaryFile(dir=packageResultPath, delete=False), - self.__fileMode, - packageResultFile) + self.__fileMode, packageResultFile, overwrite) class LocalArchiveDownloader: def __init__(self, name): @@ -700,19 +699,23 @@ def __exit__(self, exc_type, exc_value, traceback): return False class LocalArchiveUploader: - def __init__(self, tmp, fileMode, destination): + def __init__(self, tmp, fileMode, destination, overwrite): self.tmp = tmp self.fileMode = fileMode self.destination = destination + self.overwrite = overwrite def __enter__(self): return (None, self.tmp) def __exit__(self, exc_type, exc_value, traceback): self.tmp.close() # atomically move file to destination at end of upload if exc_type is None: - if not isWindows(): - if self.fileMode is not None: - os.chmod(self.tmp.name, self.fileMode) + if not isWindows() and self.fileMode is not None: + os.chmod(self.tmp.name, self.fileMode) + + if self.overwrite: + os.replace(self.tmp.name, self.destination) + elif not isWindows(): # Cannot use os.rename() because it will unconditionally # replace an existing file. Instead we link the file at the # destination and unlink the temporary file. @@ -812,44 +815,47 @@ def __openDownloadFile(self, buildId, suffix): raise ArtifactDownloadError("{} {}".format(response.status, response.reason)) - def _openUploadFile(self, buildId, suffix): - (ok, result) = self.__retry(lambda: self.__openUploadFile(buildId, suffix)) + def _openUploadFile(self, buildId, suffix, overwrite): + (ok, result) = self.__retry(lambda: self.__openUploadFile(buildId, suffix, overwrite)) if ok: return result else: raise ArtifactUploadError(str(result)) - def __openUploadFile(self, buildId, suffix): + def __openUploadFile(self, buildId, suffix, overwrite): connection = self._getConnection() url = self._makeUrl(buildId, suffix) # check if already there - connection.request("HEAD", url, headers=self._getHeaders()) - response = connection.getresponse() - response.read() - if response.status == 200: - raise ArtifactExistsError() - elif response.status != 404: - raise ArtifactUploadError("HEAD {} {}".format(response.status, response.reason)) + if not overwrite: + connection.request("HEAD", url, headers=self._getHeaders()) + response = connection.getresponse() + response.read() + if response.status == 200: + raise ArtifactExistsError() + elif response.status != 404: + raise ArtifactUploadError("HEAD {} {}".format(response.status, response.reason)) # create temporary file - return SimpleHttpUploader(self, url) + return SimpleHttpUploader(self, url, overwrite) - def _putUploadFile(self, url, tmp): - (ok, result) = self.__retry(lambda: self.__putUploadFile(url, tmp)) + def _putUploadFile(self, url, tmp, overwrite): + (ok, result) = self.__retry(lambda: self.__putUploadFile(url, tmp, overwrite)) if ok: return result else: raise ArtifactUploadError(str(result)) - def __putUploadFile(self, url, tmp): + def __putUploadFile(self, url, tmp, overwrite): # Determine file length outself and add a "Content-Length" header. This # used to work in Python 3.5 automatically but was removed later. tmp.seek(0, os.SEEK_END) length = str(tmp.tell()) tmp.seek(0) headers = self._getHeaders() - headers.update({ 'Content-Length' : length, 'If-None-Match' : '*' }) + headers.update({ 'Content-Length' : length }) + if not overwrite: + headers.update({ 'If-None-Match' : '*' }) connection = self._getConnection() connection.request("PUT", url, tmp, headers=headers) response = connection.getresponse() @@ -873,17 +879,18 @@ def __exit__(self, exc_type, exc_value, traceback): return False class SimpleHttpUploader: - def __init__(self, archiver, url): + def __init__(self, archiver, url, overwrite): self.archiver = archiver self.tmp = TemporaryFile() self.url = url + self.overwrite = overwrite def __enter__(self): return (None, self.tmp) def __exit__(self, exc_type, exc_value, traceback): try: # do actual upload on regular handle close if exc_type is None: - self.archiver._putUploadFile(self.url, self.tmp) + self.archiver._putUploadFile(self.url, self.tmp, self.overwrite) finally: self.tmp.close() return False @@ -932,11 +939,11 @@ def _openDownloadFile(self, buildId, suffix): finally: if tmpName is not None: os.unlink(tmpName) - def _openUploadFile(self, buildId, suffix): + def _openUploadFile(self, buildId, suffix, overwrite): (tmpFd, tmpName) = mkstemp() os.close(tmpFd) return CustomUploader(tmpName, self._makeUrl(buildId, suffix), self.__whiteList, - self.__uploadCmd) + self.__uploadCmd, overwrite) class CustomDownloader: def __init__(self, name): @@ -948,11 +955,12 @@ def __exit__(self, exc_type, exc_value, traceback): return False class CustomUploader: - def __init__(self, name, remoteName, whiteList, uploadCmd): + def __init__(self, name, remoteName, whiteList, uploadCmd, overwrite): self.name = name self.remoteName = remoteName self.whiteList = whiteList self.uploadCmd = uploadCmd + self.overwrite = overwrite def __enter__(self): return (self.name, None) @@ -963,6 +971,8 @@ def __exit__(self, exc_type, exc_value, traceback): env = { k:v for (k,v) in os.environ.items() if k in self.whiteList } env["BOB_LOCAL_ARTIFACT"] = self.name env["BOB_REMOTE_ARTIFACT"] = self.remoteName + if self.overwrite: + env["BOB_REMOTE_OVERWRITE"] = "1" ret = subprocess.call([getBashPath(), "-ec", self.uploadCmd], stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, cwd=gettempdir(), env=env) @@ -1034,16 +1044,16 @@ def _openDownloadFile(self, buildId, suffix): finally: if client is not None: client.close() - def _openUploadFile(self, buildId, suffix): + def _openUploadFile(self, buildId, suffix, overwrite): containerClient = self.__getClient() from azure.core.exceptions import AzureError blobName = self.__makeBlobName(buildId, suffix) blobClient = None try: blobClient = containerClient.get_blob_client(blobName) - if blobClient.exists(): + if not overwrite and blobClient.exists(): raise ArtifactExistsError() - ret = AzureUploader(containerClient, blobClient) + ret = AzureUploader(containerClient, blobClient, overwrite) containerClient = blobClient = None return ret except AzureError as e: @@ -1063,9 +1073,10 @@ def __exit__(self, exc_type, exc_value, traceback): return False class AzureUploader: - def __init__(self, containerClient, blobClient): + def __init__(self, containerClient, blobClient, overwrite): self.__containerClient = containerClient self.__blobClient = blobClient + self.__overwrite = overwrite def __enter__(self): self.__tmp = TemporaryFile() @@ -1087,7 +1098,7 @@ def __upload(self): self.__tmp.seek(0, os.SEEK_END) length = self.__tmp.tell() self.__tmp.seek(0) - self.__blobClient.upload_blob(self.__tmp, length=length, overwrite=False) + self.__blobClient.upload_blob(self.__tmp, length=length, overwrite=self.__overwrite) except ResourceExistsError: raise ArtifactExistsError() except AzureError as e: From 25d5820f48d57f18e24eb4e03cee18d597e95a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:18:59 +0100 Subject: [PATCH 2/6] test: archive: verify that live-build-ids are replaced --- test/unit/test_archive.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/unit/test_archive.py b/test/unit/test_archive.py index a6dd91dc..dcd08e54 100644 --- a/test/unit/test_archive.py +++ b/test/unit/test_archive.py @@ -363,6 +363,10 @@ def __testUploadNormal(self, archive): run(archive.uploadLocalLiveBuildId(DummyStep(), UPLOAD2_ARTIFACT, b'\x00', executor=self.executor)) self.__testBuildId(UPLOAD2_ARTIFACT, b'\x00') + # Live-build-id can be replaced + run(archive.uploadLocalLiveBuildId(DummyStep(), UPLOAD2_ARTIFACT, b'\x11', executor=self.executor)) + self.__testBuildId(UPLOAD2_ARTIFACT, b'\x11') + # provoke upload errors with self.assertRaises(BuildError): run(archive.uploadLocalLiveBuildId(DummyStep(), ERROR_UPLOAD_ARTIFACT, b'\x00', executor=self.executor)) From 79d75757088cda24b0f78d5d4b7e53f287c291ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:20:02 +0100 Subject: [PATCH 3/6] jenkins: add option to always checkout The Jenkins build is usually used to provide binary artifacts. This implies that the build should be as stable as possible. If a checkout step of some recipe is not fully deterministic, though, then relying on live-build-ids may never rectify the problem. It might also hide such a problem only until the checkout is really forced for some other reason. To increase the robustness, add a "scm.always-checkout" extended option that defaults to "true". If enabled, checkout steps will always be executed and live-build-ids will only be uploaded but never used. If the user wants to rely on live-build-ids to speed up builds he can disable the scm.always-checkout option. --- pym/bob/cmds/jenkins/exec.py | 4 ++++ pym/bob/cmds/jenkins/jenkins.py | 1 + pym/bob/state.py | 7 +++++++ 3 files changed, 12 insertions(+) diff --git a/pym/bob/cmds/jenkins/exec.py b/pym/bob/cmds/jenkins/exec.py index a5fceea9..52ec7190 100644 --- a/pym/bob/cmds/jenkins/exec.py +++ b/pym/bob/cmds/jenkins/exec.py @@ -63,6 +63,8 @@ def __handleCfg(self, line): self.shareQuota = v elif k == "platform": self.platform = v + elif k == "always-checkout": + self.scmAlwaysCheckout = isTrue(v) else: raise AssertionError(line) @@ -233,6 +235,8 @@ def doJenkinsExecuteRun(argv, bobRoot): builder.setShareHandler(getShare({ 'path' : path, 'quota' : spec.shareQuota })) builder.setShareMode(True, True) + if spec.scmAlwaysCheckout: + builder.setAlwaysCheckout([".*"]) builder.cook(ir.getRoots(), False, loop) return 0 diff --git a/pym/bob/cmds/jenkins/jenkins.py b/pym/bob/cmds/jenkins/jenkins.py index 95834cf1..452046a9 100644 --- a/pym/bob/cmds/jenkins/jenkins.py +++ b/pym/bob/cmds/jenkins/jenkins.py @@ -442,6 +442,7 @@ def dumpXML(self, orig, config, date, recipesAudit=None): "upload=" + ("1" if self.__upload else "0"), "copy=" + config.artifactsCopy, "share=" + config.sharedDir, + "always-checkout=" + ("1" if config.scmAlwaysCheckout else "0"), ]) if config.sharedQuota: execCmds.append("quota=" + config.sharedQuota) diff --git a/pym/bob/state.py b/pym/bob/state.py index c7572f5d..4ae9751d 100644 --- a/pym/bob/state.py +++ b/pym/bob/state.py @@ -161,6 +161,9 @@ def setOption(self, key, value, errorHandler): elif key == "jobs.update": if value not in ("always", "description", "lazy"): errorHandler("'jobs.update' extended option has unsupported value!"); + elif key == "scm.always-checkout": + if value.lower() not in ("0", "false", "1", "true"): + errorHandler("scm.always-checkout must be any of: 0/false/1/true") elif key == "scm.git.shallow": try: num = int(value) @@ -234,6 +237,10 @@ def jobsPolicy(self): def jobsUpdate(self): return self.__options.get('jobs.update', "always") + @property + def scmAlwaysCheckout(self): + return isTrue(self.__options.get("scm.always-checkout", "1")) + @property def scmGitShallow(self): return self.__options.get("scm.git.shallow") From d0d95ca00103aa1a1cb9b0b624da07739223616f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:42:12 +0100 Subject: [PATCH 4/6] doc: document scm.always-checkout jenkins option --- doc/manpages/bob-jenkins.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/manpages/bob-jenkins.rst b/doc/manpages/bob-jenkins.rst index d6c2dd0c..8b672680 100644 --- a/doc/manpages/bob-jenkins.rst +++ b/doc/manpages/bob-jenkins.rst @@ -532,6 +532,17 @@ jobs.update otherwise. This will provide considerable speed improvements at the expense of an outdated description of the unchanged jobs. +scm.always-checkout + Boolean option (possible values: '0' or 'false' resp. '1' or 'true') that + forces the execution of checkout steps. This option is enabled by default. + If disabled, the checkout might be skipped if a matching binary artifact + can be found. + + Disabling this option can increase the build speed. On the other hand, it + might hide problems in recipes where the checkout step is not + deterministic. Note that git and svn SCMs are checked out regardless of + this option. For release builds it is best to keep the option enabled. + scm.git.shallow Instruct the Jenkins git plugin to create shallow clones with a history truncated to the specified number of commits. If the parameter is unset From 00b2d75e404acd0ab4eb3e954f6ddd7186d7b7bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:47:00 +0100 Subject: [PATCH 5/6] jenkins: do clean builds by default The Jenkins build is usually used for creating binary artifacts and release builds. This is done best by doing clean builds. If the user wants to focus on speed, the --incremental option can be used when creating a new Jenkins alias. --- pym/bob/cmds/jenkins/jenkins.py | 7 +++++-- pym/bob/state.py | 4 ++-- test/unit/test_jenkins_set_options.py | 6 +++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/pym/bob/cmds/jenkins/jenkins.py b/pym/bob/cmds/jenkins/jenkins.py index 452046a9..44a9595e 100644 --- a/pym/bob/cmds/jenkins/jenkins.py +++ b/pym/bob/cmds/jenkins/jenkins.py @@ -810,8 +810,11 @@ def doJenkinsAdd(recipes, argv): parser.add_argument('--no-sandbox', action='store_false', dest='sandbox', default=True, help="Disable sandboxing") parser.add_argument("--credentials", help="Credentials UUID for SCM checkouts") - parser.add_argument('--clean', action='store_true', default=False, - help="Do clean builds (clear workspace)") + group = parser.add_mutually_exclusive_group() + group.add_argument('--clean', action='store_true', default=True, + help="Do clean builds (clear workspace, default)") + group.add_argument('--incremental', action='store_false', dest='clean', + help="Reuse workspace for incremental builds") parser.add_argument("name", help="Symbolic name for server") parser.add_argument("url", help="Server URL") group = parser.add_mutually_exclusive_group() diff --git a/pym/bob/state.py b/pym/bob/state.py index 4ae9751d..45188c31 100644 --- a/pym/bob/state.py +++ b/pym/bob/state.py @@ -54,7 +54,7 @@ def load(cls, config): self.upload = config.get("upload", False) self.sandbox = config.get("sandbox", True) self.credentials = config.get("credentials", None) - self.clean = config.get("clean", False) + self.clean = config.get("clean", True) self.keep = config.get("keep", False) self.authtoken = config.get("authtoken", None) self.shortdescription = config.get("shortdescription", False) @@ -99,7 +99,7 @@ def reset(self): self.upload = False self.sandbox = True self.credentials = None - self.clean = False + self.clean = True self.keep = False self.authtoken = None self.shortdescription = False diff --git a/test/unit/test_jenkins_set_options.py b/test/unit/test_jenkins_set_options.py index ae42ae8e..37232d24 100644 --- a/test/unit/test_jenkins_set_options.py +++ b/test/unit/test_jenkins_set_options.py @@ -50,7 +50,7 @@ def testAllOptions(self): "--credentials", "credentials", "--authtoken", "authtoken", "--shortdescription", - "--keep", "--download", "--upload", "--no-sandbox", "--clean", + "--keep", "--download", "--upload", "--no-sandbox", "--incremental", ] self.executeBobJenkinsCmd("set-options test " + " ".join(opts)) @@ -67,7 +67,7 @@ def testAllOptions(self): self.assertTrue(c.download) self.assertTrue(c.upload) self.assertFalse(c.sandbox) - self.assertTrue(c.clean) + self.assertFalse(c.clean) def testReset(self): self.testAllOptions() @@ -85,4 +85,4 @@ def testReset(self): self.assertFalse(c.download) self.assertFalse(c.upload) self.assertTrue(c.sandbox) - self.assertFalse(c.clean) + self.assertTrue(c.clean) From b987c12a9814014782e233e0f8155978e8cc8b2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Sat, 17 Feb 2024 18:51:30 +0100 Subject: [PATCH 6/6] doc: update jenkins about clean build defaults --- doc/manpages/bob-jenkins.rst | 9 +++++---- doc/tutorial/jenkins.rst | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/doc/manpages/bob-jenkins.rst b/doc/manpages/bob-jenkins.rst index 8b672680..7cc9c366 100644 --- a/doc/manpages/bob-jenkins.rst +++ b/doc/manpages/bob-jenkins.rst @@ -26,7 +26,8 @@ Available sub-commands: bob jenkins add [-h] [-n NODES] [-o OPTIONS] [--host-platform {linux,msys,win32}] [-w] [-p PREFIX] [-r ROOT] [-D DEFINES] [--keep] [--download] [--upload] - [--no-sandbox] [--credentials CREDENTIALS] [--clean] + [--no-sandbox] [--credentials CREDENTIALS] + [--clean | --incremental] [--shortdescription | --longdescription] name url bob jenkins export [-h] name dir @@ -94,14 +95,14 @@ Options configuration will be able to trigger a build. ``--clean`` - Do clean builds. + Do clean builds. This is the default. Whenever a Jenkins job is triggered, the workspace is erased first. This will cause a fresh checkout of the sources and a clean (re)build of all packages in the job. Use this option to ensure reproducibility at the expense of speed. - Disabled by default. See ``--incremental`` for the opposite switch. + Enabled by default. See ``--incremental`` for the opposite switch. ``--credentials CREDENTIALS`` Credentials ID for git/svn SCM checkouts. @@ -158,7 +159,7 @@ Options operating system. ``--incremental`` - Reuse workspace for incremental builds. This is the default. + Reuse workspace for incremental builds. Bob will still apply the internal heuristics to make clean builds where recipes or any of the dependencies were changed. Use ``--clean`` to always diff --git a/doc/tutorial/jenkins.rst b/doc/tutorial/jenkins.rst index f578c986..78c11e4c 100644 --- a/doc/tutorial/jenkins.rst +++ b/doc/tutorial/jenkins.rst @@ -348,8 +348,8 @@ often than others, though: * ``--download``, ``--upload``: These are usually enabled so that the binary archive is populated by the Jenkins centrally. -* ``--clean``: increases build times but will make sure that indeterminism in - recipes is suppressed. +* ``--incremental``: optimize build times but flaky recipes might cause build + problems. * ``-p PREFIX, --prefix PREFIX``: you should almost always give the jobs of each project a unique prefix. * ``--shortdescription``: shortens job descriptions and may dramatically