build(release): harden the release tools workflow (#4256) The release tools workflow could drift from dev/platforms unnoticed, hold a runner for six hours if a build hung, and only warn when a job asked for a platform that its ref lacks. It also didn't check that the Windows files are for the right CPU, though the i686 one is cross-compiled. A test now checks that the workflow's matrix and dev/platforms name the same platforms. Jobs time out after 30 minutes; builds take a few minutes. build.sh fails with an error if dev/platforms lacks the job's platform, since that's the caller's mistake; other build failures still only warn. Jobs still skip the build for refs without dev/platforms, like older release branches. verify.sh checks that each Windows file is a PE file for the triple's CPU, reading the machine type from the PE header because `file`'s descriptions change between versions. Also explains the "musleabihf" suffix in dev/platforms, and notes in RELEASING.md that a failed "Build tools" job doesn't block a release. Work towards #4216 (follow-up to #4248)
diff --git a/.github/workflows/release_build_tools.yaml b/.github/workflows/release_build_tools.yaml index 8971c6c..15a651e 100644 --- a/.github/workflows/release_build_tools.yaml +++ b/.github/workflows/release_build_tools.yaml
@@ -33,6 +33,9 @@ build: name: Build ${{ matrix.triple }} runs-on: ${{ matrix.runner }} + # Builds take a few minutes; cancel hung ones instead of holding a runner + # for the default 6 hours. + timeout-minutes: 30 strategy: # Report the result of every platform instead of stopping at the first # failure. @@ -81,15 +84,15 @@ # Release commands run main's workflows, even for patch releases of older # release branches. Skip refs without dev/platforms, since their scripts - # don't build for the job's platform, or don't exist. Refs whose - # dev/platforms lacks the job's platform fail the build, which only warns. + # don't build for the job's platform, or don't exist. If a ref's + # dev/platforms lacks the job's platform, build.sh fails. - name: Build artifacts id: build if: hashFiles('dev/platforms/BUILD.bazel') != '' run: dev/release_artifacts/build.sh "$REF" "$TRIPLE" - # build.sh only sets `files` if the build succeeded. Otherwise, it adds a - # warning instead of failing, since releases don't use the files yet. + # build.sh only sets `files` if the build succeeded. If the build failed, + # it only adds a warning, since releases don't use the files yet. - name: Verify artifacts if: steps.build.outputs.files != '' run: dev/release_artifacts/verify.sh "$TRIPLE"
diff --git a/.github/workflows/release_publish.yaml b/.github/workflows/release_publish.yaml index cfa34cc..9b5d647 100644 --- a/.github/workflows/release_publish.yaml +++ b/.github/workflows/release_publish.yaml
@@ -49,7 +49,9 @@ build_tools: # For now, this only verifies that the tools build. They aren't attached # to the release, so no other job waits on this one. A failed build only - # adds a warning to this run; a failed check of the built files fails it. + # adds a warning to this run. The run fails if a build hangs past the job + # timeout, the built files fail their checks, or the ref's dev/platforms + # lacks a job's platform. name: Build tools uses: ./.github/workflows/release_build_tools.yaml with:
diff --git a/RELEASING.md b/RELEASING.md index 29251e0..cbd9bc1 100644 --- a/RELEASING.md +++ b/RELEASING.md
@@ -71,7 +71,7 @@ The release workflow also builds `//dev/release_artifacts:artifacts_for_release` for each platform to verify it builds. For now, the files aren't attached to -the release. +the release, so a failed "Build tools" job doesn't block it. ### Manually publishing to PyPI
diff --git a/dev/platforms/BUILD.bazel b/dev/platforms/BUILD.bazel index 52880c4..90657fb 100644 --- a/dev/platforms/BUILD.bazel +++ b/dev/platforms/BUILD.bazel
@@ -11,6 +11,12 @@ package(default_visibility = ["//:__subpackages__"]) +# For the test that the release workflow's matrix matches these platforms. +exports_files( + ["BUILD.bazel"], + visibility = ["//tests/workflows:__pkg__"], +) + platform( name = "aarch64-apple-darwin", constraint_values = [ @@ -36,6 +42,10 @@ ], ) +# "musleabihf" is musl with ARM's EABI, hard-float variant: floats are passed in +# FPU registers, as in python-build-standalone's gnueabihf armv7 builds. Being +# static, it also runs on gnueabi (soft-float) systems, but not on CPUs without +# an FPU, which python-build-standalone's gnueabi builds also support. platform( name = "armv7-unknown-linux-musleabihf", constraint_values = [
diff --git a/dev/release_artifacts/build.sh b/dev/release_artifacts/build.sh index 00a81fa..3ceb0c8 100755 --- a/dev/release_artifacts/build.sh +++ b/dev/release_artifacts/build.sh
@@ -14,6 +14,13 @@ # The paths below are relative to the workspace root. cd "$(dirname "$0")/../.." +# Fail for a triple without a platform: it's the caller's mistake, unlike a +# failed build, which only warns. +if ! grep -qF "name = \"$triple\"," dev/platforms/BUILD.bazel; then + echo "::error::dev/platforms has no $triple platform" + exit 1 +fi + # Keep Git Bash on Windows from rewriting `//foo` labels into paths. export MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL="*"
diff --git a/dev/release_artifacts/verify.sh b/dev/release_artifacts/verify.sh index 8b93107..0c55db7 100755 --- a/dev/release_artifacts/verify.sh +++ b/dev/release_artifacts/verify.sh
@@ -1,8 +1,8 @@ #!/usr/bin/env bash # Checks the files built by build.sh for the platform with the Rust target -# triple TRIPLE: that Linux files are statically linked for the right CPU, and -# that macOS files don't require a newer macOS than users may have. Windows -# files aren't checked. +# triple TRIPLE: that Linux files are statically linked, that Linux and Windows +# files are for the right CPU, and that macOS files don't require a newer macOS +# than users may have. # # Usage: dev/release_artifacts/verify.sh TRIPLE set -euo pipefail @@ -54,7 +54,31 @@ exit 1 fi ;; - *-windows-*) ;; # Not checked. + *-windows-*) + # The i686 files are cross-compiled, so check that they're for the right + # CPU. PE files start with "MZ", and the PE header's offset is at 0x3c. + # The machine type follows the header's 4-byte signature. Like PE, the + # runners are little-endian, and od uses the host's byte order. + if [[ "$(head -c2 "$bin")" != MZ ]]; then + echo "::error::$bin isn't a PE file" + exit 1 + fi + pe="$(od -An -tu4 -j60 -N4 "$bin" | tr -d ' ')" + machine="$(od -An -tx2 -j"$((pe + 4))" -N2 "$bin" | tr -d ' ')" + case "$triple" in + aarch64-*) want="aa64" ;; + i686-*) want="014c" ;; + x86_64-*) want="8664" ;; + *) + echo "::error::$bin: no CPU check for $triple" + exit 1 + ;; + esac + if [[ "$machine" != "$want" ]]; then + echo "::error::$bin's PE machine type is 0x$machine, not 0x$want" + exit 1 + fi + ;; *) echo "::error::$bin: no checks for $triple" exit 1
diff --git a/tests/workflows/BUILD.bazel b/tests/workflows/BUILD.bazel index 002fe41..76656e5 100644 --- a/tests/workflows/BUILD.bazel +++ b/tests/workflows/BUILD.bazel
@@ -39,6 +39,20 @@ ) pytest_test( + name = "release_build_tools_test", + srcs = ["release_build_tools_test.py"], + data = [ + "//.github/workflows:release_build_tools.yaml", + "//dev/platforms:BUILD.bazel", + ], + target_compatible_with = NOT_WINDOWS, + deps = [ + "//python/runfiles", + "@pypi//pyyaml", + ], +) + +pytest_test( name = "workflow_permissions_test", srcs = ["workflow_permissions_test.py"], data = ["//.github/workflows:all_files"],
diff --git a/tests/workflows/release_build_tools_test.py b/tests/workflows/release_build_tools_test.py new file mode 100644 index 0000000..f9eb9e7 --- /dev/null +++ b/tests/workflows/release_build_tools_test.py
@@ -0,0 +1,36 @@ +"""Tests for the release_build_tools.yaml workflow.""" + +from __future__ import annotations + +import re + +import yaml + +from python.runfiles import runfiles + + +def test_matrix_matches_dev_platforms(): + """Each platform in dev/platforms has one matrix job, and vice versa. + + Otherwise, a platform isn't built or is built twice, or a job fails because + its platform is missing. + """ + root = runfiles.CreateOrRaise().root() / "rules_python" + workflow_path = root / ".github/workflows/release_build_tools.yaml" + workflow = yaml.safe_load(workflow_path.read_text(encoding="utf-8")) + triples = [ + entry["triple"] + for entry in workflow["jobs"]["build"]["strategy"]["matrix"]["include"] + ] + build_file = (root / "dev/platforms/BUILD.bazel").read_text(encoding="utf-8") + platforms = re.findall(r'^platform\(\s*name = "([^"]+)",', build_file, re.MULTILINE) + + assert platforms, "No platforms found in dev/platforms/BUILD.bazel" + # Fail instead of missing a platform whose call doesn't start with its name, + # e.g. because of a comment. + calls = re.findall(r"^platform\(", build_file, re.MULTILINE) + assert len(platforms) == len(calls), "Couldn't parse every platform's name" + # Sets, so that failures list the triples that differ. + assert set(triples) == set(platforms), "Matrix (left) vs. dev/platforms (right)" + duplicates = sorted({triple for triple in triples if triples.count(triple) > 1}) + assert not duplicates, f"Triples with more than one job: {duplicates}"