Fix: Replace file(GLOB) with explicit source lists in CMakeLists (#203)#314
Fix: Replace file(GLOB) with explicit source lists in CMakeLists (#203)#314
Conversation
Replace file(GLOB) with explicit source lists in src/cpp, tests/cpp, and examples/cpp CMakeLists.txt to avoid stale builds when files are added or renamed. Resolves #203.
|
Thank you, @Jiya-Rathi, to clarify, the notes that you copied and pasted above are coming from issue #203, which @rouson created some months ago, and they are not your notes. Also, the suggestion is that new cpp files need to explicitly be added to one of the corresponding lists (and not all three lists). This is, your proposed solution and implementation here, will required that all new C++ contributions (the .cpp files) be added to either src/cpp/CMakeLists.txt or tests/cpp/CMakeLists.txt or and examples/cpp/CMakeLists.txt lists. This also protects against the use of a wildcard (GLOB) which is a more vulnerable solution. |
Tony-Drummond
left a comment
There was a problem hiding this comment.
Jiya's proposed PR ensures that only intentional files in src/cpp, tests/cpp and examples/cpp are used in the build. A better way to identified software pieces that are now deprecated and a more intentional software engineering practice.
Notice that if other CPP subdirectories are created (e.g. utils/cpp or timestepping/cpp), similar lists will need to be created.
Also, this change will need to update the Contributing to MOLE pages.
|
Looks good. During yesterday's meeting, we discussed the benefit of explicitly listing the .cpp files associated with a build instead of using the GLOB filename expansion mechanism. |
cpaolini
left a comment
There was a problem hiding this comment.
Looks good. During yesterday's meeting, we discussed the benefit of explicitly listing the .cpp files associated with a build instead of using the GLOB filename expansion mechanism.
Replace file(GLOB) with explicit source lists in src/cpp, tests/cpp, and examples/cpp CMakeLists.txt to avoid stale builds when files are added or renamed. Resolves #203.
What type of PR is this? (check all applicable)
Description
TL/DR: I recommend replacing the file(GLOB ...) command here with a list of the files to build. I imagine the better approach will be something like the following with the caveat that both my cmake skills and my C++ skills are rusty:
add_library(MOLE SHARED
divergence.cpp
gradient.cpp
interpol.cpp
laplacian.cpp
mixedbc.cpp
robinbc.cpp
utils.cpp
)
My understanding from discussions with the CMake developers years ago was that listing each file to be compiled is the best practice. If I recall correctly, one reason relates to what happens when adding or renaming files. CMake originally stood for "Cross-platform Makefile generator," so rather than being a build system itself, CMake generates build systems, e.g., Makefiles or Ninja files. Once the build system has been generated, the list of files to build is fixed. The only way to pick up new files is to regenerate the build system by rerunning cmake. This could lead to confusing behavior if the person doing the rebuilding is unaware of this issue.
Related Issues & Documents
QA Instructions, Screenshots, Recordings
Do a cmake.. and make -j from build and look for any errors. After this update, any new C++ files will have to be manually added to src/cpp/CMakeLists.txt, tests/cpp/CMakeLists.txt , and examples/cpp/CMakeLists.txt to build and run them.
Added/updated tests?
We encourage you to test all code included with MOLE, including examples.
have not been included
Read Contributing Guide and Code of Conduct
Are there any post deployment tasks we need to perform?
What gif best describes this PR or how it makes you feel?