From 7178ce535bcb33c730157dff874533af65aca81f Mon Sep 17 00:00:00 2001 From: Timo Wilken Date: Thu, 7 Dec 2023 16:16:08 +0100 Subject: [PATCH] Generate init.sh in one place This reduces the duplication in the init.sh generation code significantly. Instead of generating shell commands that generate init.sh, scattered all over the place, generate the contents of init.sh in one place and just write them into the file when applicable. Once created, load the generated file instead of generating the same shell code twice at different escaping levels. All of this reduces the potential for errors a lot, since different places in the code don't need to be kept in sync when generating and re-generating the file. Add tests that make sure the generated file contents are vaguely sensible, and that clobbering the file during the build (as the AliEn-Runtime package does) does not mean it is mangled during subsequent builds. --- alibuild_helpers/build.py | 213 ++++++++++++++++------------- alibuild_helpers/build_template.sh | 43 ++---- tests/test_build.py | 64 +++++++-- tests/testdist/clobbering.sh | 4 + tox.ini | 4 + 5 files changed, 190 insertions(+), 138 deletions(-) create mode 100644 tests/testdist/clobbering.sh diff --git a/alibuild_helpers/build.py b/alibuild_helpers/build.py index feee7cf4..b3bcfefb 100644 --- a/alibuild_helpers/build.py +++ b/alibuild_helpers/build.py @@ -8,7 +8,7 @@ from alibuild_helpers.utilities import prunePaths from alibuild_helpers.utilities import resolve_store_path from alibuild_helpers.utilities import format, parseDefaults, readDefaults -from alibuild_helpers.utilities import getPackageList +from alibuild_helpers.utilities import getPackageList, asList from alibuild_helpers.utilities import validateDefaults from alibuild_helpers.utilities import Hasher from alibuild_helpers.utilities import yamlDump @@ -33,6 +33,7 @@ import concurrent.futures import importlib +import json import socket import os import re @@ -339,6 +340,97 @@ def better_tarball(spec, old, new): return old if hashes.index(old_hash) < hashes.index(new_hash) else new +def generate_initdotsh(package, specs, architecture, post_build=False): + """Return the contents of the given package's etc/profile/init.sh as a string. + + If post_build is true, also generate variables pointing to the package + itself; else, only generate variables pointing at it dependencies. + """ + spec = specs[package] + # Allow users to override ALIBUILD_ARCH_PREFIX if they manually source + # init.sh. This is useful for development off CVMFS, since we have a + # slightly different directory hierarchy there. + lines = [': "${ALIBUILD_ARCH_PREFIX:=%s}"' % architecture] + + # Generate the part which sources the environment for all the dependencies. + # We guarantee that a dependency is always sourced before the parts + # depending on it, but we do not guarantee anything for the order in which + # unrelated components are activated. + # These variables are also required during the build itself, so always + # generate them. + lines.extend(( + '[ -z "${{{bigpackage}_REVISION}}" ] && ' + '. "$WORK_DIR/$ALIBUILD_ARCH_PREFIX"/{package}/{version}-{revision}/etc/profile.d/init.sh' + ).format( + bigpackage=dep.upper().replace("-", "_"), + package=quote(specs[dep]["package"]), + version=quote(specs[dep]["version"]), + revision=quote(specs[dep]["revision"]), + ) for dep in spec.get("requires", ())) + + if post_build: + bigpackage = package.upper().replace("-", "_") + + # Set standard variables related to the package itself. These should only + # be set once the build has actually completed. + lines.extend(line.format( + bigpackage=bigpackage, + package=quote(spec["package"]), + version=quote(spec["version"]), + revision=quote(spec["revision"]), + hash=quote(spec["hash"]), + commit_hash=quote(spec["commit_hash"]), + ) for line in ( + 'export {bigpackage}_ROOT="$WORK_DIR/$ALIBUILD_ARCH_PREFIX"/{package}/{version}-{revision}', + "export {bigpackage}_VERSION={version}", + "export {bigpackage}_REVISION={revision}", + "export {bigpackage}_HASH={hash}", + "export {bigpackage}_COMMIT={commit_hash}", + )) + + # Generate the part which sets the environment variables related to the + # package itself. This can be variables set via the "env" keyword in the + # metadata or paths which get concatenated via the "{append,prepend}_path" + # keys. These should only be set once the build has actually completed, + # since the paths referred to will only exist then. + + # First, output a sensible error message if types are wrong. + for key in ("env", "append_path", "prepend_path"): + dieOnError(not isinstance(spec.get(key, {}), dict), + "Tag `%s' in %s should be a dict." % (key, package)) + + # Set "env" variables. + # We only put the values in double-quotes, so that they can refer to other + # shell variables or do command substitution (e.g. $(brew --prefix ...)). + lines.extend('export {}="{}"'.format(key, value) + for key, value in spec.get("env", {}).items() + if key != "DYLD_LIBRARY_PATH") + + # Append paths to variables, if requested using append_path. + # Again, only put values in double quotes so that they can refer to other variables. + lines.extend('export {key}="${key}:{value}"' + .format(key=key, value=":".join(asList(value))) + for key, value in spec.get("append_path", {}).items() + if key != "DYLD_LIBRARY_PATH") + + # First convert all values to list, so that we can use .setdefault().insert() below. + prepend_path = {key: asList(value) + for key, value in spec.get("prepend_path", {}).items()} + # By default we add the .../bin directory to PATH and .../lib to LD_LIBRARY_PATH. + # Prepend to these paths, so that our packages win against system ones. + for key, value in (("PATH", "bin"), ("LD_LIBRARY_PATH", "lib")): + prepend_path.setdefault(key, []).insert(0, "${}_ROOT/{}".format(bigpackage, value)) + lines.extend('export {key}="{value}${{{key}+:${key}}}"' + .format(key=key, value=":".join(value)) + for key, value in prepend_path.items() + if key != "DYLD_LIBRARY_PATH") + + # Return string without a trailing newline, since we expect call sites to + # append that (and the obvious way to inesrt it into the build tempate is by + # putting the "%(initdotsh_*)s" on its own line, which has the same effect). + return "\n".join(lines) + + def doBuild(args, parser): if args.remoteStore.startswith("http"): syncHelper = HttpRemoteSync(args.remoteStore, args.architecture, args.workDir, args.insecure) @@ -927,81 +1019,6 @@ def doBuild(args, parser): debug("Found tarball in %s" % spec["cachedTarball"] if spec["cachedTarball"] else "No cache tarballs found") - # Generate the part which sources the environment for all the dependencies. - # Notice that we guarantee that a dependency is always sourced before the - # parts depending on it, but we do not guaranteed anything for the order in - # which unrelated components are activated. - dependencies = "ALIBUILD_ARCH_PREFIX=\"${ALIBUILD_ARCH_PREFIX:-%s}\"\n" % args.architecture - dependenciesInit = "echo ALIBUILD_ARCH_PREFIX=\"\\${ALIBUILD_ARCH_PREFIX:-%s}\" >> $INSTALLROOT/etc/profile.d/init.sh\n" % args.architecture - for dep in spec.get("requires", []): - depSpec = specs[dep] - depInfo = { - "architecture": args.architecture, - "package": dep, - "version": depSpec["version"], - "revision": depSpec["revision"], - "bigpackage": dep.upper().replace("-", "_") - } - dependencies += format("[ -z ${%(bigpackage)s_REVISION+x} ] && source \"$WORK_DIR/$ALIBUILD_ARCH_PREFIX/%(package)s/%(version)s-%(revision)s/etc/profile.d/init.sh\"\n", - **depInfo) - dependenciesInit += format('echo [ -z \\${%(bigpackage)s_REVISION+x} ] \\&\\& source \\${WORK_DIR}/\\${ALIBUILD_ARCH_PREFIX}/%(package)s/%(version)s-%(revision)s/etc/profile.d/init.sh >> \"$INSTALLROOT/etc/profile.d/init.sh\"\n', - **depInfo) - dependenciesDict = {} - for dep in spec.get("full_requires", []): - depSpec = specs[dep] - depInfo = { - "architecture": args.architecture, - "package": dep, - "version": depSpec["version"], - "revision": depSpec["revision"], - "hash": depSpec["hash"] - } - dependenciesDict[dep] = depInfo - dependenciesJSON = str(dependenciesDict) - - # Generate the part which creates the environment for the package. - # This can be either variable set via the "env" keyword in the metadata - # or paths which get appended via the "append_path" one. - # By default we append LD_LIBRARY_PATH, PATH - environment = "" - dieOnError(not isinstance(spec.get("env", {}), dict), - "Tag `env' in %s should be a dict." % p) - for key,value in spec.get("env", {}).items(): - if key == "DYLD_LIBRARY_PATH": - continue - environment += format("echo 'export %(key)s=\"%(value)s\"' >> $INSTALLROOT/etc/profile.d/init.sh\n", - key=key, - value=value) - basePath = "%s_ROOT" % p.upper().replace("-", "_") - - pathDict = spec.get("append_path", {}) - dieOnError(not isinstance(pathDict, dict), - "Tag `append_path' in %s should be a dict." % p) - for pathName,pathVal in pathDict.items(): - pathVal = isinstance(pathVal, list) and pathVal or [ pathVal ] - if pathName == "DYLD_LIBRARY_PATH": - continue - environment += format("\ncat << \\EOF >> \"$INSTALLROOT/etc/profile.d/init.sh\"\nexport %(key)s=$%(key)s:%(value)s\nEOF", - key=pathName, - value=":".join(pathVal)) - - # Same thing, but prepending the results so that they win against system ones. - defaultPrependPaths = { "LD_LIBRARY_PATH": "$%s/lib" % basePath, - "PATH": "$%s/bin" % basePath } - pathDict = spec.get("prepend_path", {}) - dieOnError(not isinstance(pathDict, dict), - "Tag `prepend_path' in %s should be a dict." % p) - for pathName,pathVal in pathDict.items(): - pathDict[pathName] = isinstance(pathVal, list) and pathVal or [ pathVal ] - for pathName,pathVal in defaultPrependPaths.items(): - pathDict[pathName] = [ pathVal ] + pathDict.get(pathName, []) - for pathName,pathVal in pathDict.items(): - if pathName == "DYLD_LIBRARY_PATH": - continue - environment += format("\ncat << \\EOF >> \"$INSTALLROOT/etc/profile.d/init.sh\"\nexport %(key)s=%(value)s${%(key)s+:$%(key)s}\nEOF", - key=pathName, - value=":".join(pathVal)) - # The actual build script. referenceStatement = "" if "reference" in spec: @@ -1034,33 +1051,35 @@ def doBuild(args, parser): else: cachedTarball = spec["cachedTarball"] - - cmd = format(cmd_raw, - dependencies=dependencies, - dependenciesInit=dependenciesInit, - dependenciesJSON=dependenciesJSON, - develPrefix=develPrefix, - environment=environment, - workDir=workDir, - configDir=abspath(args.configDir), - incremental_recipe=spec.get("incremental_recipe", ":"), - sourceDir=source and (dirname(source) + "/") or "", - sourceName=source and basename(source) or "", - referenceStatement=referenceStatement, - gitOptionsStatement="" if args.docker else - "export GIT_CLONE_SPEEDUP=" + quote(" ".join(clone_speedup_options())), - requires=" ".join(spec["requires"]), - build_requires=" ".join(spec["build_requires"]), - runtime_requires=" ".join(spec["runtime_requires"]) - ) - scriptDir = join(workDir, "SPECS", args.architecture, spec["package"], spec["version"] + "-" + spec["revision"]) err, out = getstatusoutput("mkdir -p %s" % scriptDir) dieOnError(err, "Failed to create script dir %s: %s" % (scriptDir, out)) - writeAll("%s/build.sh" % scriptDir, cmd) writeAll("%s/%s.sh" % (scriptDir, spec["package"]), spec["recipe"]) + writeAll("%s/build.sh" % scriptDir, cmd_raw % { + "dependenciesJSON": json.dumps({dep: { + "architecture": args.architecture, + "package": dep, + "version": specs[dep]["version"], + "revision": specs[dep]["revision"], + "hash": specs[dep]["hash"], + } for dep in spec.get("full_requires", ())}), + "initdotsh_deps": generate_initdotsh(p, specs, args.architecture, post_build=False), + "initdotsh_full": generate_initdotsh(p, specs, args.architecture, post_build=True), + "develPrefix": develPrefix, + "workDir": workDir, + "configDir": abspath(args.configDir), + "incremental_recipe": spec.get("incremental_recipe", ":"), + "sourceDir": (dirname(source) + "/") if source else "", + "sourceName": basename(source) if source else "", + "referenceStatement": referenceStatement, + "gitOptionsStatement": "" if args.docker else + "export GIT_CLONE_SPEEDUP=" + quote(" ".join(clone_speedup_options())), + "requires": " ".join(spec["requires"]), + "build_requires": " ".join(spec["build_requires"]), + "runtime_requires": " ".join(spec["runtime_requires"]), + }) banner("Building %s@%s", spec["package"], args.develPrefix if "develPrefix" in args and spec["package"] in develPkgs diff --git a/alibuild_helpers/build_template.sh b/alibuild_helpers/build_template.sh index 8ab91e75..9ef0d3d7 100644 --- a/alibuild_helpers/build_template.sh +++ b/alibuild_helpers/build_template.sh @@ -12,9 +12,6 @@ set +h function hash() { true; } export WORK_DIR="${WORK_DIR_OVERRIDE:-%(workDir)s}" -# From our dependencies -%(dependencies)s - # Insert our own wrapper scripts into $PATH, patched to use the system OpenSSL, # instead of the one we build ourselves. export PATH=$WORK_DIR/wrapper-scripts:$PATH @@ -80,26 +77,21 @@ fi mkdir -p "$INSTALLROOT" "$BUILDROOT" "$BUILDDIR" "$WORK_DIR/INSTALLROOT/$PKGHASH/$PKGPATH" cd "$WORK_DIR/INSTALLROOT/$PKGHASH" -cat <<\EOF | tr \' \" >"$INSTALLROOT/.full-dependencies" +cat <<\EOF > "$INSTALLROOT/.full-dependencies" %(dependenciesJSON)s EOF +# Add "source" command for dependencies to init.sh. +# Install init.sh now, so that it is available for debugging in case the build fails. mkdir -p "$INSTALLROOT/etc/profile.d" -BIGPKGNAME=`echo "$PKGNAME" | tr [:lower:] [:upper:] | tr - _` rm -f "$INSTALLROOT/etc/profile.d/init.sh" - -# Adds "source" command for dependencies to init.sh. -# Install init.sh now, so that it is available for debugging in case the build fails. -%(dependenciesInit)s - -cat << EOF >> $INSTALLROOT/etc/profile.d/init.sh -export ${BIGPKGNAME}_ROOT=\${WORK_DIR}/\${ALIBUILD_ARCH_PREFIX}/$PKGNAME/$PKGVERSION-$PKGREVISION -export ${BIGPKGNAME}_VERSION=$PKGVERSION -export ${BIGPKGNAME}_REVISION=$PKGREVISION -export ${BIGPKGNAME}_HASH=$PKGHASH -export ${BIGPKGNAME}_COMMIT=${COMMIT_HASH} +cat <<\EOF > "$INSTALLROOT/etc/profile.d/init.sh" +%(initdotsh_deps)s EOF +# Apply dependency initialisation now, but skip setting the variables below until after the build. +. "$INSTALLROOT/etc/profile.d/init.sh" + # Add support for direnv https://github.com/direnv/direnv/ # # This is beneficial for all the cases where the build step requires some @@ -116,9 +108,6 @@ source_up unset DYLD_LIBRARY_PATH EOF -# Add environment variables to init.sh (e.g. PATH). -%(environment)s - cd "$BUILDROOT" ln -snf $PKGHASH $WORK_DIR/BUILD/$PKGNAME-latest if [[ $DEVEL_PREFIX ]]; then @@ -198,14 +187,10 @@ fi # Regenerate init.sh, in case the package build clobbered it. This # particularly happens in the AliEn-Runtime package, since it copies other # packages into its installroot wholesale. +mkdir -p "$INSTALLROOT/etc/profile.d" rm -f "$INSTALLROOT/etc/profile.d/init.sh" -%(dependenciesInit)s -cat << EOF >> "$INSTALLROOT/etc/profile.d/init.sh" -export ${BIGPKGNAME}_ROOT=\${WORK_DIR}/\${ALIBUILD_ARCH_PREFIX}/$PKGNAME/$PKGVERSION-$PKGREVISION -export ${BIGPKGNAME}_VERSION=$PKGVERSION -export ${BIGPKGNAME}_REVISION=$PKGREVISION -export ${BIGPKGNAME}_HASH=$PKGHASH -export ${BIGPKGNAME}_COMMIT=${COMMIT_HASH} +cat <<\EOF > "$INSTALLROOT/etc/profile.d/init.sh" +%(initdotsh_full)s EOF cd "$WORK_DIR/INSTALLROOT/$PKGHASH" @@ -221,8 +206,10 @@ source_up # dynamic loader unset DYLD_LIBRARY_PATH EOF -# Add environment variables to init.sh (e.g. PATH). -%(environment)s + +cat <<\EOF > "$INSTALLROOT/.full-dependencies" +%(dependenciesJSON)s +EOF cd "$WORK_DIR/INSTALLROOT/$PKGHASH/$PKGPATH" # Find which files need relocation. diff --git a/tests/test_build.py b/tests/test_build.py index d1ea2f7b..da46f0d0 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -19,7 +19,7 @@ from alibuild_helpers.cmd import is_string from alibuild_helpers.utilities import parseRecipe, resolve_tag -from alibuild_helpers.build import doBuild, storeHashes +from alibuild_helpers.build import doBuild, storeHashes, generate_initdotsh TEST_DEFAULT_RELEASE = """\ @@ -307,20 +307,21 @@ def test_coverDoBuild(self, mock_debug, mock_glob, mock_sys, mock_git_git): call(list(fetch_args), directory=fetch_dir, check=fetch_check, prompt=False), ], any_order=True) + def setup_spec(self, script): + """Parse the alidist recipe in SCRIPT and return its spec.""" + err, spec, recipe = parseRecipe(lambda: script) + self.assertIsNone(err) + spec["recipe"] = recipe.strip("\n") + spec.setdefault("tag", spec["version"]) + spec["tag"] = resolve_tag(spec) + return spec + def test_hashing(self): """Check that the hashes assigned to packages remain constant.""" - def setup_spec(script): - err, spec, recipe = parseRecipe(lambda: script) - self.assertIsNone(err) - spec["recipe"] = recipe.strip("\n") - spec.setdefault("tag", spec["version"]) - spec["tag"] = resolve_tag(spec) - return spec - - default = setup_spec(TEST_DEFAULT_RELEASE) - zlib = setup_spec(TEST_ZLIB_RECIPE) - root = setup_spec(TEST_ROOT_RECIPE) - extra = setup_spec(TEST_EXTRA_RECIPE) + default = self.setup_spec(TEST_DEFAULT_RELEASE) + zlib = self.setup_spec(TEST_ZLIB_RECIPE) + root = self.setup_spec(TEST_ROOT_RECIPE) + extra = self.setup_spec(TEST_EXTRA_RECIPE) default["commit_hash"] = "0" for spec, refs in ((zlib, TEST_ZLIB_GIT_REFS), (root, TEST_ROOT_GIT_REFS), @@ -361,6 +362,43 @@ def setup_spec(script): self.assertEqual(len(extra["remote_hashes"]), 3) self.assertEqual(extra["local_hashes"][0], TEST_EXTRA_BUILD_HASH) + def test_initdotsh(self): + """Sanity-check the generated init.sh for a few variables.""" + specs = { + # Add some attributes that are normally set by doBuild(), but + # required by generate_initdotsh(). + spec["package"]: dict(spec, revision="1", commit_hash="424242", hash="010101") + for spec in map(self.setup_spec, ( + TEST_DEFAULT_RELEASE, + TEST_ZLIB_RECIPE, + TEST_ROOT_RECIPE, + TEST_EXTRA_RECIPE, + )) + } + + setup_initdotsh = generate_initdotsh("ROOT", specs, "slc7_x86-64", post_build=False) + complete_initdotsh = generate_initdotsh("ROOT", specs, "slc7_x86-64", post_build=True) + + # We only generate init.sh for ROOT, so Extra should not appear at all. + self.assertNotIn("Extra", setup_initdotsh) + self.assertNotIn("Extra", complete_initdotsh) + + # Dependencies must be loaded both for this build and for subsequent ones. + self.assertIn('. "$WORK_DIR/$ALIBUILD_ARCH_PREFIX"/zlib/v1.2.3-1/etc/profile.d/init.sh', setup_initdotsh) + self.assertIn('. "$WORK_DIR/$ALIBUILD_ARCH_PREFIX"/zlib/v1.2.3-1/etc/profile.d/init.sh', complete_initdotsh) + + # ROOT-specific variables must not be set during ROOT's build yet... + self.assertNotIn("export ROOT_VERSION=", setup_initdotsh) + self.assertNotIn("export ROOT_TEST_1=", setup_initdotsh) + self.assertNotIn("export APPEND_ROOT_1=", setup_initdotsh) + self.assertNotIn("export PREPEND_ROOT_1=", setup_initdotsh) + + # ...but they must be set once ROOT's build has completed. + self.assertIn("export ROOT_VERSION=v6-08-30", complete_initdotsh) + self.assertIn('export ROOT_TEST_1="root test 1"', complete_initdotsh) + self.assertIn("export APPEND_ROOT_1=", complete_initdotsh) + self.assertIn("export PREPEND_ROOT_1=", complete_initdotsh) + if __name__ == '__main__': unittest.main() diff --git a/tests/testdist/clobbering.sh b/tests/testdist/clobbering.sh new file mode 100644 index 00000000..1cc5c718 --- /dev/null +++ b/tests/testdist/clobbering.sh @@ -0,0 +1,4 @@ +package: clobbering +version: "1" +--- +echo exit 1 > "$INSTALLROOT/etc/profile.d/init.sh" diff --git a/tox.ini b/tox.ini index 9d5f87aa..d137ef8a 100644 --- a/tox.ini +++ b/tox.ini @@ -99,6 +99,10 @@ commands = sh -c 'coverage run --source={toxinidir} -a {toxinidir}/aliBuild -c {toxinidir}/tests/testdist build broken6 --force-unknown-architecture --no-system --disable GCC-Toolchain 2>&1 | tee /dev/stderr | grep "while scanning a quoted scalar"' sh -c 'coverage run --source={toxinidir} -a {toxinidir}/aliBuild -c {toxinidir}/tests/testdist build broken7 --force-unknown-architecture --no-system --disable GCC-Toolchain 2>&1 | tee /dev/stderr | grep "Malformed entry prefer_system"' + # Make sure that etc/profile.d/init.sh is re-written properly, even if the package build overwrites it. + # In particular, AliEn-Runtime does this, so we must handle this. + sh -c 'coverage run --source={toxinidir} -a {toxinidir}/aliBuild -c {toxinidir}/tests/testdist build clobbering -a {env:ARCHITECTURE} --no-system --no-remote-store --disable GCC-Toolchain >&2 && WORK_DIR=$PWD/sw . sw/{env:ARCHITECTURE}/clobbering/1-local1/etc/profile.d/init.sh' + coverage run --source={toxinidir} -a {toxinidir}/aliBuild build zlib -a {env:ARCHITECTURE} --no-system --disable GCC-Toolchain alienv -a {env:ARCHITECTURE} q alienv -a {env:ARCHITECTURE} setenv zlib/latest -c bash -c '[[ $LD_LIBRARY_PATH == */zlib/* ]]'