Conversation
WalkthroughReplaces many Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30–60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries.cmake/coinor.cmake(2 hunks)
🔇 Additional comments (2)
libraries.cmake/coinor.cmake (2)
134-143: Per-language COINOR_ flag split looks good*The separation into
COINOR_CXXFLAGS,COINOR_CFLAGS,COINOR_FFLAGS, andCOINOR_EXTRA_ARGSis clear and makes the platform-specific configuration easier to reason about. No issues from a CMake/build perspective here.
206-218:make installexecute_process wiring and logging look solidThe switch to
execute_processfor${CMAKE_MAKE_PROGRAM} installwithCOINOR_MAKE_OUT/COINOR_MAKE_ERRcapture, plus appending both streams to${LOGFILE}, is a clear improvement overexec_program. The status/error messaging on failure also looks appropriate.Also applies to: 221-221
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libraries.cmake/coinor.cmake (2)
168-201: Use the more robust RESULT_VARIABLE check pattern for execute_process
execute_processcan setRESULT_VARIABLEeither to an integer exit code or to a non-numeric error string (e.g. for timeouts or spawn failures).(cmake.org)
To handle both cases reliably, CMake docs and common usage recommend the idiomif(STATUS AND NOT STATUS EQUAL 0)rather than onlyif(NOT STATUS EQUAL 0)when checkingRESULT_VARIABLE.(stackoverflow.com)Here that would slightly improve robustness for both the configure and make steps:
- if( NOT COINOR_CONFIGURE_SUCCESS EQUAL 0) + if(COINOR_CONFIGURE_SUCCESS AND NOT COINOR_CONFIGURE_SUCCESS EQUAL 0) message( STATUS "Configure COIN-OR library .. failed") message( STATUS ${COINOR_CONFIGURE_ERR}) message( FATAL_ERROR ${COINOR_CONFIGURE_OUT}) else() message( STATUS "Configure COIN-OR library .. done") endif() ## make install message( STATUS "Building and installing COIN-OR library (make install).. ") @@ - if( NOT COINOR_MAKE_SUCCESS EQUAL 0) + if(COINOR_MAKE_SUCCESS AND NOT COINOR_MAKE_SUCCESS EQUAL 0) message( STATUS "Building and installing COIN-OR library (make install) .. failed") message( STATUS ${COINOR_MAKE_ERR}) message( FATAL_ERROR ${COINOR_MAKE_OUT}) else() message( STATUS "Building and installing COIN-OR library (make install) .. done") endif()This keeps behavior unchanged for normal non-zero exit codes, but is safer if CMake ever returns a descriptive error string in
COINOR_*_SUCCESS.If you want to double-check how
RESULT_VARIABLEbehaves in your exact CMake version, you can run a tiny CMake script that callsexecute_processwith a bad command and prints the captured status, then confirm the above pattern behaves as expected.Also applies to: 210-224, 242-245
170-179: Env save/restore fixes earlier leak; optionally track “defined vs empty” explicitlySaving
CXXFLAGS/CFLAGS/FFLAGSinto_ORIG_*and restoring them after the configure + make steps cleanly addresses the earlier concern about leaking these flags into laterexecute_processcalls, and logging bothCOINOR_CONFIGURE_OUTandCOINOR_CONFIGURE_ERRto${LOGFILE}completes the diagnostics story.(cmake.org)One minor edge case: using
STREQUAL ""to decide whether tounset(ENV{...})cannot distinguish between an env var that was truly undefined and one that was defined but empty. If you ever care about that distinction, you could track “had this env var” separately:- set(_ORIG_CXXFLAGS "$ENV{CXXFLAGS}") - set(_ORIG_CFLAGS "$ENV{CFLAGS}") - set(_ORIG_FFLAGS "$ENV{FFLAGS}") + if(DEFINED ENV{CXXFLAGS}) + set(_ORIG_CXXFLAGS "$ENV{CXXFLAGS}") + set(_HAD_CXXFLAGS TRUE) + else() + set(_ORIG_CXXFLAGS "") + set(_HAD_CXXFLAGS FALSE) + endif() + if(DEFINED ENV{CFLAGS}) + set(_ORIG_CFLAGS "$ENV{CFLAGS}") + set(_HAD_CFLAGS TRUE) + else() + set(_ORIG_CFLAGS "") + set(_HAD_CFLAGS FALSE) + endif() + if(DEFINED ENV{FFLAGS}) + set(_ORIG_FFLAGS "$ENV{FFLAGS}") + set(_HAD_FFLAGS TRUE) + else() + set(_ORIG_FFLAGS "") + set(_HAD_FFLAGS FALSE) + endif() @@ - if(_ORIG_CXXFLAGS STREQUAL "") - unset(ENV{CXXFLAGS}) - else() - set(ENV{CXXFLAGS} "${_ORIG_CXXFLAGS}") - endif() - if(_ORIG_CFLAGS STREQUAL "") - unset(ENV{CFLAGS}) - else() - set(ENV{CFLAGS} "${_ORIG_CFLAGS}") - endif() - if(_ORIG_FFLAGS STREQUAL "") - unset(ENV{FFLAGS}) - else() - set(ENV{FFLAGS} "${_ORIG_FFLAGS}") - endif() + if(_HAD_CXXFLAGS) + set(ENV{CXXFLAGS} "${_ORIG_CXXFLAGS}") + else() + unset(ENV{CXXFLAGS}) + endif() + if(_HAD_CFLAGS) + set(ENV{CFLAGS} "${_ORIG_CFLAGS}") + else() + unset(ENV{CFLAGS}) + endif() + if(_HAD_FFLAGS) + set(ENV{FFLAGS} "${_ORIG_FFLAGS}") + else() + unset(ENV{FFLAGS}) + endif()If you don’t have cases where empty-but-defined flags matter, the current implementation is perfectly acceptable and already a clear improvement over the previous behavior.
If you’re unsure whether any of your CI or local environments rely on
CXXFLAGS/CFLAGS/FFLAGSbeing explicitly set to an empty string, it’s worth spot-checking by printing those env vars before invoking CMake in one representative build.Also applies to: 225-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libraries.cmake/coinor.cmake(2 hunks)
🔇 Additional comments (1)
libraries.cmake/coinor.cmake (1)
132-143: *Per-language COINOR_FLAGS / COINOR_EXTRA_ARGS split is coherentThe Darwin vs non-Darwin split for
COINOR_CXXFLAGS,COINOR_CFLAGS,COINOR_FFLAGS, andCOINOR_EXTRA_ARGSis consistent and lines up with how you feedCOINOR_CXXFLAGSinto the configure environment below. I don’t see issues with this refactor in isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CMakeLists.txt (2)
145-159: Add compiler version checks for Windows/MSVC to complete C++20 enforcement.C++20 standard is enforced globally (line 141–142), but compiler version checks are only applied on UNIX (line 145). Windows/MSVC builds lack a minimum version guard, risking silent failures on older toolchains that cannot support C++20.
Add a complementary MSVC version check to ensure Visual Studio 2019 (toolset 14.2) or later, which is the minimum for reasonable C++20 support.
elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS "10.0.0") message(FATAL_ERROR "g++: OpenMS-contrib requires version 10.0.0 or later to build but found ${CMAKE_CXX_COMPILER_VERSION}") endif() endif() +elseif (MSVC) + if(MSVC_VERSION VERSION_LESS "1920") + message(FATAL_ERROR "MSVC: OpenMS-contrib requires Visual Studio 2019 (toolset 14.2, MSVC_VERSION ≥ 1920) or later for C++20 support but found ${MSVC_VERSION}") + endif() +endif()
90-91: Replace remainingexec_programcalls inlibraries.cmake/glpk.cmakewithexecute_process.The codebase still contains 5 active
exec_programcalls in glpk.cmake (lines 22, 37, 73, 98, 115), contradicting the migration goal noted at CMakeLists.txt:90–91. These must be replaced withexecute_processto complete the deprecation migration. The NOTE comment indicates past concerns about Windows argument handling withexecute_process—verify functionality on Windows before merging.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
136-142: Remove the deprecatedCMAKE_ALLOW_LOOSE_LOOP_CONSTRUCTSflag.
CMAKE_ALLOW_LOOSE_LOOP_CONSTRUCTSis a CMake 2.x–era backward-compatibility setting no longer needed in CMake 3.10+ (your minimum required version per line 134). It has no effect and should be removed to keep the build system clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)libraries.cmake/eigen.cmake(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (4)
libraries.cmake/eigen.cmake (4)
11-14: LGTM!Unquoting
ZIP_ARGSvalues creates proper CMake lists, which is the correct approach when these arguments will be expanded as separate command-line arguments inOPENMS_SMARTEXTRACT.
24-26: LGTM!Good addition of diagnostic messages. These will be helpful for troubleshooting build configuration issues.
38-48: LGTM!Good enhancements:
TIMEOUT 300prevents indefinite hangs during configuration.- Capturing the exit code in
RESULT_VARIABLEand displaying stdout/stderr on failure significantly improves debugging.
65-75: LGTM!Consistent with the configuration step enhancements. The 5-minute timeout is appropriate for installing Eigen headers.
Replace the outdated
exec_programwithexecute_processSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.