Update patch for CSDP Makefile - #4654
Conversation
d-torrance
added
dependencies
|
I added a related cmake fix to a change I made in #4059. If we pass So now we also pass the build type's optimization flags. They go after the the user flags, overwriting them when they conflict, but Claude swears up and down that's cmake's convention. |
CMAKE_C_FLAGS holds only the flags meant for every configuration; the
build type's optimization level lives in the per-configuration variables,
CMAKE_C_FLAGS_RELEASE and friends. We forwarded just the former, so
unless the user exported CFLAGS we handed each dependency an empty one.
Set but empty also suppresses autoconf's -g -O2 fallback, since
${CFLAGS+y} counts an empty value as set.
How much that mattered varies by dependency. The ones with a configure
script largely fill the gap themselves: factory's puts -O3 in CXXFLAGS
when the caller does not, so it built the same either way. The ones left
with no -O at all are the three whose hand written makefile takes CFLAGS
from the command line and has no configure to fall back on: cohomcalg,
csdp and lrslib.
The per-configuration flags go last, matching the order cmake uses for
our own targets, which means that for a flag like -O, where the last one
wins, the build type has the final say over an exported CFLAGS.
CMAKE_BUILD_TYPE holds what the user typed, e.g. Release, while the
variables are spelled CMAKE_C_FLAGS_RELEASE, hence the TOUPPER; without
it the lookup names a variable that does not exist and quietly expands to
nothing.
Csdp is where this became visible: its new makefile honors a CFLAGS given
on the command line, so the empty value we were passing left it compiling
at -O0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
This looks good. I applied the patch to CSDP 6.2.0 and built it on macOS with explicit caller CFLAGS, CPPFLAGS, and LDFLAGS plus libomp and Accelerate. Both CSDP tests passed through the patched CMake target, and a Release configure forwarded the general and configuration-specific compiler flags in the expected order.
AI-assisted PR review alongside GPT 5.6-Sol
2 participants
We update our patch for the CSDP Makefile so it's more configurable.
Right now we have to do some goofy hacks to get it to work with our build, and it breaks in strange places, e.g.:
That's because setting
CFLAGSat the command line overwrote the-I../includeflag we needed to find the headers!I just uploaded the same patch for the Debian package and plan to update the spack and Homebrew packages soon, too. The changes are also submitted upstream at coin-or/Csdp#22.
🤖 AI Disclosure 🤖
Claude wrote the first draft of the Makefile patch, and together we made a ton of revisions. It also helped put the together the M2 side of it in
M2/libraries/csdp/Makefile.inandM2/cmake/build-libraries.cmake.Draft for now to test the builds.Edit: Builds passed!