Conversation
Author
|
Toolchain Quick Test failed because cmake.org is down. |
There was a problem hiding this comment.
🟡 Changes recommended
Preserve the existing MKL i386/ia32 mapping and clarify or restore the changed OpenBLAS fallback behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Replaces OpenBLAS-based architecture probing with normalized uname -m detection and simplifies toolchain architecture handling.
Changes:
- Adds
SYSTEM_ARCHnormalization across toolchain installers. - Updates OpenBLAS target and fallback behavior.
- Removes obsolete OpenBLAS variables and probe script.
File summaries
| File | Summary |
|---|---|
toolchain/scripts/stage3/install_elpa.sh |
Uses normalized architecture for CPU feature detection. |
toolchain/scripts/stage2/install_openblas.sh |
Updates target selection and fallback behavior; fallback policy requires clarification. |
toolchain/scripts/stage2/install_mkl.sh |
Uses SYSTEM_ARCH; currently rejects i386 before the prior ia32 mapping. |
toolchain/scripts/stage1/install_openmpi.sh |
Uses normalized architecture for compatibility flags. |
toolchain/scripts/stage0/setup_buildtools.sh |
Removes the OpenBLAS architecture probe. |
toolchain/scripts/stage0/install_cmake.sh |
Selects CMake artifacts by system architecture. |
toolchain/scripts/package_versions.sh |
Removes OpenBLAS-dependent checksum selection. |
toolchain/scripts/get_openblas_arch.sh |
Removes the obsolete probe script. |
toolchain/scripts/common_vars.sh |
Defines normalized host architecture detection. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Growl1234
force-pushed
the
toolchain
branch
3 times, most recently
from
September 15, 2026 14:03
87ac0cb to
40564f4
Compare
Growl1234
force-pushed
the
toolchain
branch
from
September 15, 2026 14:05
40564f4 to
bbb1972
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace
OPENBLAS_ARCHwith a lightweight host architecture detection based onuname -m, normalizing only known equivalent names such asx86_64/amd64andaarch64/arm64.Also remove
OPENBLAS_LIBCOREandget_openblas_arch.sh. For native builds, OpenBLAS already performs its own CPU/core detection whenTARGETis not specified, while the existingDYNAMIC_ARCHfallback is preserved.This removes an unnecessary OpenBLAS download/probe from the early toolchain stages and simplifies the architecture handling.
See also cp2k/cp2k#6023.