Skip to content

Align macOS CI with container build: derive configuration at runtime #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wdconinc merged 5 commits into from
Dec 19, 2025
Merged

Align macOS CI with container build: derive configuration at runtime #96

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3e79e6a
Initial plan
Copilot Dec 18, 2025
adfc3d0
Align macOS CI with container build: spack versions, cherry-picks, ex…
Copilot Dec 18, 2025
a82b16e
Derive spack configuration from spack.sh and spack-packages.sh at run…
Copilot Dec 18, 2025
68a7cd3
Fix cherry-pick loops to filter heredoc delimiters and empty lines
Copilot Dec 18, 2025
2cd3d9e
Improve cherry-pick filtering and add proper quoting for security
Copilot Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 56 additions & 24 deletions .github/workflows/macos-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,48 +32,80 @@ jobs:
with:
submodules: recursive

- name: Extract Spack configuration
id: spack-config
run: |
# Source spack.sh to get SPACK_VERSION and SPACK_CHERRYPICKS
source spack.sh
echo "spack-version=${SPACK_VERSION}" >> $GITHUB_OUTPUT
echo "spack-cherrypicks<<EOF" >> $GITHUB_OUTPUT
echo "${SPACK_CHERRYPICKS}" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT

# Source spack-packages.sh to get SPACKPACKAGES_VERSION and SPACKPACKAGES_CHERRYPICKS
source spack-packages.sh
echo "spackpackages-version=${SPACKPACKAGES_VERSION}" >> $GITHUB_OUTPUT
echo "spackpackages-cherrypicks<<EOF" >> $GITHUB_OUTPUT
echo "${SPACKPACKAGES_CHERRYPICKS}" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT

# Extract buildcache URL from mirrors.yaml.in (simplified - get the version)
echo "buildcache-version=${SPACKPACKAGES_VERSION}" >> $GITHUB_OUTPUT

- name: Setup Spack
uses: spack/setup-spack@v2.1.1
with:
ref: develop
ref: ${{ steps.spack-config.outputs.spack-version }}
color: true
path: ${{ github.workspace }}/spack

- name: Apply Spack cherry-picks
run: |
cd ${{ github.workspace }}/spack
git config user.name "GitHub Actions"
git config user.email "actions@github.com"

# Apply cherry-picks from spack.sh (match only valid git commit hashes)
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
Comment on lines +69 to +72
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +72
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

- 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
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
cd var/spack/repos/spack-packages
git checkout ${{ steps.spack-config.outputs.spackpackages-version }}
git config user.name "GitHub Actions"
git config user.email "actions@github.com"

# Apply cherry-picks from spack-packages.sh (match only valid git commit hashes)
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
Comment on lines +84 to +87
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- name: Register spack-packages repository
run: |

Copilot uses AI. Check for mistakes.
cd ${{ github.workspace }}/spack
spack repo add var/spack/repos/spack-packages
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.

- name: Add Spack environment
run: |
spack env create ci spack-environment/ci
spack env activate ci
spack -e ci compiler find
spack -e ci external find llvm
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
spack -e ci external find llvm
spack -e ci external find --not-buildable llvm

Copilot uses AI. Check for mistakes.
spack -e ci external find --not-buildable cmake

- name: Concretize
run: |
spack -e ci concretize -f

- name: Setup buildcache access
uses: spack/github-actions-buildcache@v2
with:
mode: readwrite
key: macos-${{ matrix.compiler }}-ci
- name: Configure buildcache
run: |
spack -e ci mirror add eic oci://ghcr.io/eic/spack-${{ steps.spack-config.outputs.buildcache-version }}
spack -e ci buildcache keys --install --trust

- name: Install
run: |
spack -e ci install --no-check-signature --show-log-on-error

- name: Push to buildcache
if: always()
run: |
spack -e ci buildcache push --update-index --only=package --unsigned ${{ github.workspace }}/buildcache-macos

- name: Show environment info
if: always()
run: |
spack -e ci find -lv
spack -e ci graph --dot > ci-graph.dot

- name: Upload environment graph
if: always()
uses: actions/upload-artifact@v4
with:
name: ci-environment-graph-${{ matrix.compiler }}
path: ci-graph.dot