Skip to content

Conversation

@wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Nov 15, 2025

Briefly, what does this PR introduce?

Needs:

This PR upgrades spack to v1.1.0, https://github.com/spack/spack/releases/tag/v1.1.0.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

Copilot AI review requested due to automatic review settings November 15, 2025 00:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the Spack package manager version from v1.0.2 to v1.1.0 to incorporate the latest upstream changes and improvements from the Spack project.

  • Updates the SPACK_VERSION variable to reference the v1.1.0 release
  • Maintains existing cherry-picks for custom patches
  • No breaking changes or behavior modifications

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wdconinc wdconinc changed the title Upgrade spack to v1.1.0 Upgrade Spack to v1.1.0 Nov 15, 2025
@wdconinc

This comment was marked as resolved.

@wdconinc

This comment was marked as off-topic.

@wdconinc wdconinc force-pushed the spack-1.1.0 branch 8 times, most recently from 8cc24d5 to 8d3576f Compare November 21, 2025 21:46
Copilot AI review requested due to automatic review settings November 26, 2025 01:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings November 26, 2025 02:19
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings November 26, 2025 14:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wdconinc wdconinc enabled auto-merge (squash) December 24, 2025 22:42
Copilot AI review requested due to automatic review settings December 25, 2025 14:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wdconinc wdconinc force-pushed the spack-1.1.0 branch 2 times, most recently from 28f7657 to d4a6845 Compare December 27, 2025 20:57
Copilot AI review requested due to automatic review settings December 27, 2025 20:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +51 to +55
# their complex dependency trees or compatibility requirements. The new additions (py-dask-awkward,
# py-dask-histogram, py-distributed, py-requests) are included because they may be required in multiple
# versions or configurations by different packages in the environment, and allowing duplicates prevents
# build failures or conflicts during concretization. This list should be reviewed periodically to ensure
# only necessary packages are included.
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The documentation mentions that py-dask-awkward, py-dask-histogram, py-distributed, and py-requests are "new additions" to the allowlist. However, based on the spack environment specifications:

  • py-dask-histogram is explicitly used in multiple environments (ci, cuda, tf, xl, etc.)
  • py-dask is explicitly used in multiple environments
  • py-distributed is NOT found in any environment specification
  • py-requests is NOT found in any environment specification
  • py-dask-awkward is NOT found in any environment specification

The comment should be updated to accurately reflect which packages are actually used in the environments. If py-distributed, py-requests, and py-dask-awkward are transitive dependencies that need to be on the allowlist, the documentation should clarify this rather than stating they "may be required in multiple versions or configurations by different packages" without evidence in the environment specs.

Suggested change
# their complex dependency trees or compatibility requirements. The new additions (py-dask-awkward,
# py-dask-histogram, py-distributed, py-requests) are included because they may be required in multiple
# versions or configurations by different packages in the environment, and allowing duplicates prevents
# build failures or conflicts during concretization. This list should be reviewed periodically to ensure
# only necessary packages are included.
# their complex dependency trees or compatibility requirements. Some of these packages (such as py-dask
# and py-dask-histogram) are explicitly used in multiple Spack environments, which can naturally lead to
# multiple concretizations. Others (py-dask-awkward, py-distributed, py-requests) are not directly listed
# in the environment specifications but may appear as transitive dependencies or in alternative
# configurations of those packages; allowing duplicates for them helps avoid concretization conflicts or
# build failures. This list should be reviewed periodically to ensure only necessary packages are included.

Copilot uses AI. Check for mistakes.
spack concretize --force
spack --color=never find --long --no-groups --show-concretized --format "{name}" \
| uniq -D -f2 | grep -v -w -e "\(epic\|llvm\|py-setuptools\|py-urllib3\)" \
| uniq -D -f2 | grep -Evw "(${SPACK_DUPLICATE_ALLOWLIST})" \
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The use of the build argument SPACK_DUPLICATE_ALLOWLIST directly inside a double-quoted shell command in grep -Evw "(${SPACK_DUPLICATE_ALLOWLIST})" allows shell metacharacters (such as ", ;, or backticks) in the argument value to break out of the intended pattern and inject arbitrary shell commands at image build time. An attacker who can influence the Docker build arguments (for example in a multi-tenant CI/build service that exposes SPACK_DUPLICATE_ALLOWLIST as a user-configurable parameter) could exploit this to execute arbitrary commands in the build environment. To fix this, ensure that SPACK_DUPLICATE_ALLOWLIST is not interpolated directly into the shell command without sanitization/escaping, or restrict it to a safe, predefined set of values rather than arbitrary patterns.

Copilot uses AI. Check for mistakes.
sed -i "/# JUGGLER_VERSION$/ s/@[^' ]*/@git.${JUGGLER_SHA}=main/" /opt/spack-environment/packages.yaml
spack deconcretize -y --all juggler
fi
spack concretize --force
Copy link

Copilot AI Dec 27, 2025

Choose a reason for hiding this comment

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

The repeated use of grep -Evw "(${SPACK_DUPLICATE_ALLOWLIST})" with the untrusted build argument SPACK_DUPLICATE_ALLOWLIST inside a double-quoted shell command enables shell injection if the argument contains characters like ", ;, or backticks. In environments where users can set Docker build arguments but not modify the Dockerfile itself, an attacker could craft a malicious value for SPACK_DUPLICATE_ALLOWLIST to break out of the grep invocation and run arbitrary commands in the image build context. Mitigate this by ensuring SPACK_DUPLICATE_ALLOWLIST cannot contain shell metacharacters (e.g., by validating against a strict pattern or fixed options) or by restructuring the command to avoid direct interpolation into the shell.

Suggested change
spack concretize --force
spack concretize --force
# Validate SPACK_DUPLICATE_ALLOWLIST to avoid shell injection via grep pattern
case "${SPACK_DUPLICATE_ALLOWLIST}" in
(*[!A-Za-z0-9_|-]*)
echo "Invalid SPACK_DUPLICATE_ALLOWLIST: only characters [A-Za-z0-9_|-] are allowed" >&2
exit 1
;;
esac

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants