Repository navigation
Support compilation w/ clang 23 - #4694
Merged
Merged
Conversation
d-torrance
added
the
build issue
MichaelABurr
approved these changes
Sep 15, 2026
d-torrance
force-pushed
the
clang-23
branch
from
September 15, 2026 19:37
edfcbc2 to
549e6a5
Compare
d-torrance
force-pushed
the
clang-23
branch
from
September 15, 2026 19:55
549e6a5 to
2fe88bc
Compare
MichaelABurr
approved these changes
Sep 15, 2026
The patch step's stamp lives in the build directory, but SOURCE_DIR was the submodule checkout shared by every build directory, so a second configure re-ran the patch on an already patched tree. GNU patch's --batch assumes such a patch is reversed and silently reverts it, which brings back the clang 23 failure with nothing pointing at the cause. fflas_ffpack was the only external project that both used a submodule as its source and patched it. Stage the submodule into libraries/fflas_ffpack/source instead, as the autotools build does (UNTARCMD in libraries/Makefile.library.in), so the stamp and the tree it guards are created together. This also keeps autogen.sh from writing into the submodule. Checkouts that have already built this branch have a patched submodule; run "git -C M2/submodules/fflas_ffpack checkout -- ." once before rebuilding, or the copy inherits the patch and it gets reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
M2_HOST_PREFIX/include was passed through add_compile_options, so it landed in CMake's FLAGS slot as a plain -I. Imported targets -- PkgConfig::FFLAS_FFPACK, TBB::tbb -- contribute their includes to the INCLUDES slot as -isystem, and our own prefix arrives there too, by way of the fflas-ffpack.pc we install under usr-host. A -I naming a directory that is also given as -isystem is discarded, so our prefix kept whatever position it held in the -isystem chain, which on a Homebrew machine is behind /opt/homebrew/include. M2-engine then compiled against Homebrew's fflas-ffpack rather than the copy we had just built and installed. Use include_directories(BEFORE SYSTEM ...) so our prefix leads the -isystem chain instead. Both keywords are needed: SYSTEM without BEFORE still loses to an earlier -isystem, and BEFORE without SYSTEM leaves the -I to be discarded as a duplicate. Marking it SYSTEM is accurate, since that directory holds third-party dependency headers rather than ours, and pkg-config already presents them that way. Directory scope covers d/, bin/ and the unit tests, not just the engine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
|
This version now works for me (fflas_ffpack patches, doesn't appear to re-patch, although I'm not sure I tested that well...). |
mikestillman
approved these changes
Sep 16, 2026
mikestillman
left a comment
mikestillman
left a comment
Member
There was a problem hiding this comment.
How does the mathic update work? Does that need to be done separately?
Otherwise this looks good!
Member
Author
|
I merged the mathic update on the mathic repo (Macaulay2/mathic#28) before updating the submodule on the M2 side, so it's all set. |
Closed
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.
Clang 23 requires including some more headers explicitly, e.g.,
<iterator>forstd::back_inserter.We had a couple issues in Macaulay2, as well as mathic and fflas-ffpack.
🤖 AI Disclosure 🤖
No AI used!I ended up using Claude for some cmake things I couldn't figure out