Align macOS CI with container build: derive configuration at runtime #96
Conversation
…ternals, and buildcache Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
…time Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Updated to derive all configuration from spack.sh and spack-packages.sh at runtime using bash source and GitHub Actions outputs. The workflow now automatically extracts versions and cherry-picks instead of hardcoding them (commit a82b16e) |
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
Co-authored-by: wdconinc <4656391+wdconinc@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aligns the macOS CI workflow with the container build configuration by dynamically extracting Spack configuration from source files rather than hardcoding values. This creates a single source of truth and ensures consistency across different build environments.
Key Changes:
- Extracts Spack versions and cherry-picks dynamically from
spack.shandspack-packages.shat runtime - Applies cherry-picks (3 for spack, 61 for spack-packages) using extracted commit hashes with validation filtering
- Configures buildcache to use OCI mirror with extracted version:
oci://ghcr.io/eic/spack-${VERSION}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Checkout and configure spack-packages | ||
| run: | | ||
| cd ${{ github.workspace }}/spack | ||
| git clone https://github.com/spack/spack-packages.git var/spack/repos/spack-packages |
There was a problem hiding this comment.
The git clone command lacks error handling and doesn't specify a depth. If the clone fails due to network issues or if the repository is unavailable, the subsequent cd and git checkout commands will fail with confusing error messages. Consider adding --depth 1 for faster cloning since only a specific version is needed, and ensure the clone succeeds before proceeding.
| echo "${{ steps.spack-config.outputs.spack-cherrypicks }}" | grep -E '^[a-f0-9]{40}$' | while read -r commit; do | ||
| echo "Cherry-picking ${commit}" | ||
| git cherry-pick "${commit}" | ||
| done |
There was a problem hiding this comment.
The grep pattern '^[a-f0-9]{40}$' will silently filter out invalid lines including the heredoc delimiters (---) and any malformed commit hashes. While this provides robustness, it also means that if the configuration files are accidentally corrupted or contain invalid data, the workflow will silently skip those entries without warning. Consider adding a validation step that warns if any lines are filtered out, or explicitly count and verify the expected number of cherry-picks were applied.
| spack env create ci spack-environment/ci | ||
| spack env activate ci | ||
| spack -e ci compiler find | ||
| spack -e ci external find llvm |
There was a problem hiding this comment.
The spack external find llvm command is added without the --not-buildable flag, unlike cmake. This means spack may still attempt to build llvm from source if the external installation doesn't meet requirements. Based on the PR description mentioning "Find llvm externally in addition to cmake, matching container build strategy", consider whether --not-buildable should also be applied to llvm to enforce using only the external installation.
| spack -e ci external find llvm | |
| spack -e ci external find --not-buildable llvm |
| echo "Cherry-picking ${commit}" | ||
| git cherry-pick "${commit}" | ||
| done | ||
There was a problem hiding this comment.
This step changes directory multiple times (cd commands on lines 76, 78, and 89). Each cd in a multi-line run block affects subsequent commands in that block. While the logic appears correct, the final cd ${{ github.workspace }}/spack on line 89 before spack repo add could be fragile. Consider adding verification that you're in the correct directory, or restructure to avoid multiple directory changes.
| - name: Register spack-packages repository | |
| run: | |
| echo "${{ steps.spack-config.outputs.spack-cherrypicks }}" | grep -E '^[a-f0-9]{40}$' | while read -r commit; do | ||
| echo "Cherry-picking ${commit}" | ||
| git cherry-pick "${commit}" | ||
| done |
There was a problem hiding this comment.
The cherry-pick loop uses a pipe with while read, which runs in a subshell. If any cherry-pick fails, the error won't cause the workflow step to fail because the exit code is lost in the subshell. This could result in silently skipped cherry-picks and divergent behavior from the container builds.
Consider using a for loop with process substitution or command substitution to properly propagate errors. For example:
while IFS= read -r commit; do
echo "Cherry-picking ${commit}"
git cherry-pick "${commit}" || exit 1
done < <(echo "${{ steps.spack-config.outputs.spack-cherrypicks }}" | grep -E '^[a-f0-9]{40}$')Or use command substitution with a for loop:
for commit in $(echo "${{ steps.spack-config.outputs.spack-cherrypicks }}" | grep -E '^[a-f0-9]{40}$'); do
echo "Cherry-picking ${commit}"
git cherry-pick "${commit}"
done| echo "${{ steps.spack-config.outputs.spackpackages-cherrypicks }}" | grep -E '^[a-f0-9]{40}$' | while read -r commit; do | ||
| echo "Cherry-picking ${commit}" | ||
| git cherry-pick "${commit}" | ||
| done |
There was a problem hiding this comment.
The same cherry-pick error handling issue exists here. The pipe with while read runs in a subshell, so if any cherry-pick fails, the error won't cause the workflow step to fail. This could result in silently skipped cherry-picks.
Apply the same fix as suggested for the spack cherry-picks: use process substitution or command substitution with proper error propagation.
| done | ||
| cd ${{ github.workspace }}/spack | ||
| spack repo add var/spack/repos/spack-packages |
There was a problem hiding this comment.
The spack repo add command could fail if the repository path is invalid or if there are configuration issues. This would prevent the workflow from proceeding correctly but wouldn't be immediately obvious from the error message. Consider adding error context or verification that the repository was successfully added.
| spack repo add var/spack/repos/spack-packages | |
| if ! spack repo add var/spack/repos/spack-packages; then | |
| echo "ERROR: Failed to add Spack repository 'var/spack/repos/spack-packages'." >&2 | |
| echo "Current Spack repositories:" >&2 | |
| spack repo list >&2 || true | |
| exit 1 | |
| fi | |
| # Verify that the repository was successfully registered | |
| if ! spack repo list | grep -q 'var/spack/repos/spack-packages'; then | |
| echo "ERROR: Spack repository 'var/spack/repos/spack-packages' does not appear in 'spack repo list' after adding." >&2 | |
| spack repo list >&2 || true | |
| exit 1 | |
| fi |
The macOS CI workflow was using different spack configuration than the container builds, risking inconsistent behavior and package versions across environments.
Changes:
oci://ghcr.io/eic/spack-${VERSION}The workflow now maintains a single source of truth for spack configuration in the repository's existing configuration files.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.