[Draft] Updating Coinor.cmake to update deprecated method exec_program #148
Conversation
WalkthroughThe pull request updates the CMake configuration in the Coin-OR module by replacing the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Script as coinor.cmake
participant Exec as execute_process
participant Shell as Shell/Command
Script->>Exec: Invoke configuration command with WORKING_DIRECTORY, OUTPUT_VARIABLE, RESULT_VARIABLE, and ERROR_VARIABLE
Exec->>Shell: Execute configuration command
Shell-->>Exec: Return output, result, and error
Exec-->>Script: Deliver configuration result
Script->>Exec: Invoke installation command with WORKING_DIRECTORY and "-j1" argument
Exec->>Shell: Execute installation command
Shell-->>Exec: Return installation status
Exec-->>Script: Deliver installation status
Assessment against linked issues
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libraries.cmake/coinor.cmake (2)
182-191: Improved make install command implementationGood job updating the make install command to use
execute_processand specifying the job parameter as"-j1"to ensure it's treated as a single argument.For consistency with the configure command, consider adding an
ERROR_VARIABLEparameter to capture any potential error output from the make process. This would make error handling more robust.execute_process( COMMAND ${CMAKE_MAKE_PROGRAM} install "-j1" WORKING_DIRECTORY ${COINOR_DIR} # Explicitly pass as one argument OUTPUT_VARIABLE COINOR_MAKE_OUT + ERROR_VARIABLE COINOR_MAKE_ERR RESULT_VARIABLE COINOR_MAKE_SUCCESS )If you add this parameter, consider also updating line 198 to include the error output:
- message( FATAL_ERROR ${COINOR_MAKE_OUT}) + message( FATAL_ERROR ${COINOR_MAKE_OUT} ${COINOR_MAKE_ERR})
175-176: Include error output in configure failure messageFor better debugging, consider including the error output in the error message when configuration fails.
- message( FATAL_ERROR ${COINOR_CONFIGURE_OUT}) + message( FATAL_ERROR ${COINOR_CONFIGURE_OUT} ${COINOR_CONFIGURE_ERR})
| #exec_program -> execute_process | ||
| execute_process( | ||
| COMMAND | ||
| ./configure | ||
| -C | ||
| --prefix=${PROJECT_BINARY_DIR} | ||
| ${STATIC_BUILD} | ||
| ${SHARED_BUILD} | ||
| --with-lapack=no | ||
| --with-blas=no | ||
| ${COINOR_EXTRA_FLAGS_LIST} | ||
| CXX=${CMAKE_CXX_COMPILER} | ||
| CC=${CMAKE_C_COMPILER} | ||
| WORKING_DIRECTORY ${COINOR_DIR} | ||
| OUTPUT_VARIABLE COINOR_CONFIGURE_OUT | ||
| RETURN_VALUE COINOR_CONFIGURE_SUCCESS | ||
| ) | ||
| ERROR_VARIABLE COINOR_CONFIGURE_ERR | ||
| RESULT_VARIABLE COINOR_CONFIGURE_SUCCESS | ||
| ) | ||
|
|
There was a problem hiding this comment.
Good transition from exec_program to execute_process
The replacement of the deprecated exec_program with execute_process is a positive improvement. You've correctly structured the command with appropriate parameters including WORKING_DIRECTORY, OUTPUT_VARIABLE, ERROR_VARIABLE, and RESULT_VARIABLE.
However, I noticed a potential issue: on line 159, you're using ${COINOR_EXTRA_FLAGS_LIST}, but this variable is defined as COINOR_EXTRA_FLAGS on lines 134/136. This mismatch could prevent the extra flags from being applied correctly.
- ${COINOR_EXTRA_FLAGS_LIST}
+ ${COINOR_EXTRA_FLAGS}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #exec_program -> execute_process | |
| execute_process( | |
| COMMAND | |
| ./configure | |
| -C | |
| --prefix=${PROJECT_BINARY_DIR} | |
| ${STATIC_BUILD} | |
| ${SHARED_BUILD} | |
| --with-lapack=no | |
| --with-blas=no | |
| ${COINOR_EXTRA_FLAGS_LIST} | |
| CXX=${CMAKE_CXX_COMPILER} | |
| CC=${CMAKE_C_COMPILER} | |
| WORKING_DIRECTORY ${COINOR_DIR} | |
| OUTPUT_VARIABLE COINOR_CONFIGURE_OUT | |
| RETURN_VALUE COINOR_CONFIGURE_SUCCESS | |
| ) | |
| ERROR_VARIABLE COINOR_CONFIGURE_ERR | |
| RESULT_VARIABLE COINOR_CONFIGURE_SUCCESS | |
| ) | |
| #exec_program -> execute_process | |
| execute_process( | |
| COMMAND | |
| ./configure | |
| -C | |
| --prefix=${PROJECT_BINARY_DIR} | |
| ${STATIC_BUILD} | |
| ${SHARED_BUILD} | |
| --with-lapack=no | |
| --with-blas=no | |
| ${COINOR_EXTRA_FLAGS} | |
| CXX=${CMAKE_CXX_COMPILER} | |
| CC=${CMAKE_C_COMPILER} | |
| WORKING_DIRECTORY ${COINOR_DIR} | |
| OUTPUT_VARIABLE COINOR_CONFIGURE_OUT | |
| ERROR_VARIABLE COINOR_CONFIGURE_ERR | |
| RESULT_VARIABLE COINOR_CONFIGURE_SUCCESS | |
| ) |
|
Compiles on MacOS. Unsure why extra flags list is highlighted by code rabbit, doesn't make much of a difference in this scenario as it's just passing a list of arguments it seems like |
Much of the reason this is a draft and not a full PR is because a) I haven't tested it on mac os yet, and b) There are quite a few 'register' warnings when building this way (on Fedora 41), but they seem harmless and the program compiles. Intends to close #145
Summary by CodeRabbit