⬆️ Olb v1.7 #40
⬆️ Olb v1.7 #40
Conversation
micheltakken
added
dependencies
There was a problem hiding this comment.
I hope you don't mind that I had a look at your draft PR.
Unfortunately, the version on main doesn't compile for me at the moment because of the OpenLB version used there (a missing include on Linux with GCC 14.2.1) and this upgrade seems to resolve the issue.
After the minor fixes I added in the other comments, it compiled for me (but no guarantees that it actually does what it's supposed to do :) ). I'd love to see this get merged so that I can work with the project without dirty workarounds in the dependencies.
| IF (WIN32) | ||
| set(patch_file ${CMAKE_CURRENT_BINARY_DIR}/_deps/lbm-src/src/core/singleton.h) | ||
| set(lbm_patch sed -i -f ${CMAKE_CURRENT_SOURCE_DIR}/script.sed ${patch_file}) | ||
| IF (!WIN32) |
There was a problem hiding this comment.
This check does not work for me on Linux and is always false. Using NOT instead of the exclamation mark seems to work - but you could also simply swap the branches to avoid the negation altogether.
Since the majority of the if/else/endif in this file seem to be lower-case, maybe also consider using that instead for consistency reasons?
| IF (!WIN32) | |
| if (NOT WIN32) |
| set(patch_file ${CMAKE_CURRENT_BINARY_DIR}/_deps/lbm-src/src/core/singleton.h) | ||
| set(lbm_patch sed -i -f ${CMAKE_CURRENT_SOURCE_DIR}/script.sed ${patch_file}) | ||
| IF (!WIN32) | ||
| set(CMAKE_CXX_FLAGS "-O3 -Wall -march=native -mtune=native") |
There was a problem hiding this comment.
Without this line, the Eigen CMake module will complain because it fails to compile its compiler detection code. Must be some CMake weirdness that the flags bleed down into the dependencies. :)
The issue seems to be resolved on master of Eigen, but they do not seem to provide fixed tags anymore. :(
It could be considered to switch to a commit hash as a fixed reference point for a newer version. The dropped support of C++03 shouldn't matter too much since this repository uses C++17 anyway.
However, it works as-is - just pretty confusing at first if it doesn't work anymore just because this line is missing.
| ${EXTERNAL_DIR}/zlib/zlib.h | ||
| ${EXTERNAL_DIR}/zlib/zutil.h | ||
| set(LBM_HEADER_LIST | ||
| incl/unistd.h |
There was a problem hiding this comment.
This file prevents compilation on Linux (io.h does not exist on Unix systems). Maybe move it to incl/windows/unistd.h and then use if (WIN32) add_subdirectory(windows) endif()?
You can also have multiple calls to target_sources such that you only need the one if and the header will then get added to all necessary targets in the subdirectory CMakeLists.txt. Same for the target_include_directories.
| ${EXTERNAL_DIR}/tinyxml/tinyxml.cpp | ||
| ${EXTERNAL_DIR}/tinyxml/tinystr.cpp | ||
| ${EXTERNAL_DIR}/tinyxml/tinyxmlerror.cpp | ||
| ${EXTERNAL_DIR}/tinyxml/tinyxmlparser.cpp |
There was a problem hiding this comment.
The tinyxml library sources unfortunately seem to be necessary for the definitions of the class TiXmlDocument which is used in OpenLB.
This looks like such a pain because OpenLB can only be built via Makefiles. :(
OpenLB v1.7
Update to OpenLB version 1.7