Refactor the CMake 3rdparty/Vc-related bits #1903
Conversation
brndnmtthws
commented
May 8, 2024
•
brndnmtthws
commented
- Include the 3rdparty/ dir from top-level
- Set includes/libs at top-level
- Remove the inner CMake version requirement
- Fix deprecated use of CMake's exec_program()
github-actions
bot
added
3rdparty
✅ Deploy Preview for conkyweb canceled.
|
* Include the 3rdparty/ dir from top-level * Set includes/libs at top-level * Remove the inner CMake version requirement * Fix deprecated use of CMake's exec_program()
brndnmtthws
force-pushed
the
cmake-vc-tweaks
branch
from
803820a
to
08b79c9
Compare
| set(conky_libs ${conky_libs} Vc) | ||
| set(conky_includes ${conky_includes} "${CMAKE_CURRENT_SOURCE_DIR}/Vc") | ||
|
|
||
| set(conky_libs ${conky_libs} PARENT_SCOPE) | ||
| set(conky_includes ${conky_includes} PARENT_SCOPE) |
There was a problem hiding this comment.
Why not keep 3rdparty related build configuration in 3rdparty/CMakeLists.txt?
There was a problem hiding this comment.
I dislike this because it feels like the dependency is going in the wrong direction (from lower -> upper scope). The child shouldn't know about the parent (i.e., you should be able to take the code out of 3rdparty/ and move it elsewhere and it should work all the same).
| add_subdirectory(3rdparty) | ||
| add_subdirectory(3rdparty/toluapp) | ||
| set(conky_libs ${conky_libs} toluapp_lib_static) | ||
| set(conky_libs ${conky_libs} Vc) | ||
| set(conky_includes ${conky_includes} "${CMAKE_SOURCE_DIR}/3rdparty/Vc") |
There was a problem hiding this comment.
And move toluapp from here there?
It's simpler to have to modify a single file when adding/removing dependencies than doing so in 2 places.
So I'd change one of the following:
- Put all thirdparty configuration in thirdparty to avoid cluttering main CMake file.
- Some
add_subdirectoryin the future might require 10sets before and hugemark_as_advancedas Eigen did. I personally prefer minimally invasive approach because its a lot less work to update sources that way.
- Some
- Completely remove
3rdparty/CMakeLists.txt - Something in-between those two options is to
set(vc_includes "${CMAKE_CURRENT_SOURCE_DIR}/Vc" PARENT_SCOPE)in3rdparty/CMakeLists.txtand then doset(conky_includes ${conky_includes} ${vc_includes}), for both Vc and toluapp.- This separates details on where 3rdparty libraries keep their libs and includes from conky configuration.
There was a problem hiding this comment.
I'd like to maintain the hierarchy. I'm okay with removing 3rdparty/CMakeLists.txt entirely, but I prefer keeping the paths within 3rdparty/ isolated such that they don't depend paths higher up the chain.
There was a problem hiding this comment.
Also this isn't a hill I'm willing to die on, more a matter of preference.
There was a problem hiding this comment.
I'd like to maintain the hierarchy. I'm okay with removing 3rdparty/CMakeLists.txt entirely, but I prefer keeping the paths within 3rdparty/ isolated such that they don't depend paths higher up the chain.
I get what you mean.
Also this isn't a hill I'm willing to die on, more a matter of preference.
I die on every hill (lack of experience), even when wrong sometimes, so sorry if I'm being too nit-picky.
After looking at the comment again, I noticed how the tone might've come off wrong - the two comments are a single question but I posted them in a thread to reference multiple parts of the code, didn't mean it as "Why this? AND YOU DID THIS?", so I'm sorry if you received it that way, that wasn't my intention. I should've use ...see next... at the end or something like that.
I value your input and opinion, you've got more experience than me and have worked with OSS and this project much longer than I.
