-
Notifications
You must be signed in to change notification settings - Fork 112
CMake Support #392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CMake Support #392
Conversation
|
Added support for the remainder of addons. However, some of them are broken and can not compile even when using Make. |
|
Also, a nice benefit is that I managed to compile PhysiCell on Windows without mingw. I just needed to change |
drbergman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look, on the whole, good to go. I cannot comment on the value they add since I don't understand how to make use of these. A few other comments below.
Looking forward to getting the power of HPC compute right at our fingertips!
Expanded .gitignore to include all project executables.
|
I get: On my Mac, CMake determines my default compiler is clang (which lacks OpenMP), whereas previously, our Makefiles would check for an env var PHYSICELL_CPP and use it if present (primarily intended for Mac brew g++). |
Yeah, so CMake supports this, but under a different env variable name. Or a more modern but verbose variant: Or you can brew install libomp and try if CMake can find it :) |
The latter is always set, so we can rely on it in addons CMakeLists
|
Following up: @asmelko was able to fix some of the issues I was experiencing on a Mac build, so it all seems to be working for me - at least using a brew installed g++. I'm confident it could also be possible to build using Apple's native clang++ (with a brew installed libomp). At least on a Mac, I think it should be possible to just use the generated Makefile and resulting executable in the root dir (rather than down in /build) and, after modifying the Finally, on Windows, I encountered the same issues I had when I attempted to use cmake on a previous version of PhysiCell (but were all fixable then and likely now):
|
This is for MSVC compilation. Its OpenMP version does not support all pragmas that are used in the project. Also, MSVC was missing some includes containing exports, causing link errors.
@rheiland There is another make/cmake functionality, which could be used for moving binaries to specified dirs (such as repo root), which is install. I can add install functionality for each target, which will "install" them into the repo root. I will be happy to hear your thoughts on this.
Let me start with that there are 2 compilers for windows: Nevertheless, I added a commit that enables building PhysiCell for both Windows compilers. Firstly, I enable OpenMP only if a compiler supports versions >=4, and secondly, I solved Still, PhysiBoSS uses way too much UNIX routines, so that requires a separate PR for a direct Windows support. Therefore, you still need to configure cmake followingly, when using a Windows compiler: |
Unsure if _WIN is defined together with MINGW, so implementing it in a safer way.
I don't recall having any issues with building an earlier PhysiCell release with cmake and Regardless, the following tries to capture my current attempt to use cmake on Windows. It's a virgin machine - no C++ compiler, so I thought it might be possible to just install clang and then have cmake use it: But cmake required installing ninja (and then editing my PATH env var to point to it): Then, try to run cmake with clang: Unsure what to make of this, so I installed Visual Studio Community edition (~5GB). Then re-try the cmake with clang build: And then: |
|
@rheiland Thanks a lot for being a testing dummy! :)
I am adding OpenMP >=4.0 requirement only for this kind of errors that we get when compiling with As my optimizations porting will be advancing, I may experiment with lowering the OpenMP version requirement and fixing some of these errors if it brings us interesting Windows portability. Finally, I added a missing |
|
Using your latest code, I now get this and will try to find reasons/alternatives, but welcome comments: |
|
@rheiland If you are referring to CMake Warning for unused variables, I would not bother by that too much. But if you mean a not found OpenMP support, that is concerning. I installed clang and LLVM toolset via visual studio installer and for me, OpenMP is found: Maybe you also have a more behaving clang compiler version under |
This reverts commit c8ef390.
|
Added GitHub actions for Building and Testing using CMake. Everything passes on Windows, MacOS, Ubuntu. |
|
I tested this on my macOS and it worked. However, I think we should highlight the use of |
|
Imagine a common physicell flow: make biorobots # configures makefiles to build biorobots
make # builds biorobotsAfter these 2 steps, one has biorobots exe ready in the root of the project with config files, custom module and main copied along. Now this is the proposed flow: # runs cmake configure step, ordering that "build" is the build directory and that the top-level cmake file is in "." this directory
cmake -B build -S .
# runs cmake build step, looking for build instructions in "build" dir, building "only" biorobots
cmake --build build --target biorobotsAlthough it looks same, there are already major differences:
Question 1: Is this new executable location acceptable? There is a notion of "cmake install" that can copy the built executables to a specified location, but that is an additional step and commonly used to install "everything". Not sure if we can install just target. If needed, I can explore this. Next, to change the compiler, one changes to (first) configure step to: cmake -B build -S . -DCMAKE_CXX_COMPILER=g++-11This will use g++-11 for all targets built in this build tree. Question 2: Is this new flow acceptable? Setting the compiler like this is standard in the CMake world. It offers more flexibility, especially when dealing with multiple targets and configurations. However, I think it is possible to fetch the content of PHYSICELL_CPP env var and use it to set CMAKE_CXX_COMPILER within the cmake files, if that is preferred. Next, I copied the github test and build workflows and adapted them to this new flow. They are working fine. Question 3: Is this new workflow acceptable for the CI as well? It is more robust and scalable, but I want to ensure it fits within your existing processes. Next, I added conditional inclusion of dfba and libroadrunner in the configure step. This means that the addons wont even configure (i.e., show as buildable targets) unless the user explicitly requests them by setting Question 4: Is this conditional inclusion acceptable? Is the naming acceptable? Finally, let me describe the CMake architecture briefly:
All sample-projects-related CMakeLists.txt are almost identical. The ideal flow of adding a new sample project would be just to copy Question 5: Is this CMake architecture acceptable? I think it could be possible to have just one CMakeLists.txt for all sample projects that loops over all subdirectories, but that would be more complex and less explicit. I opted for explicitness here. Question 6: Are C++ flags set in the top-level CMakeLists.txt acceptable? Question 7: Is minimal CMake version 3.26 acceptable? Maybe for some systems it is too new? I can try to lower it if needed. Please all, try this new flow and provide feedback on the above questions. If all is acceptable, let's merge. |
|
Q1: I would try to do a cmake install, to put the executable in the root folder. At least during a transition period, not to brake PhysiCell Studio default behavior. Q2: Ok for me. This is the default usage of cmake, let's keep it standard. Q3: We will have to adapt the CI, create a new one for cmake. That way, we test both, and make sure both work. EDIT: you already did create new ones, so perfect ! Q4: Yes. Maybe PhysiBoSS should also be off by default ? If I remember correctly, this is still not working for Windows MSVC. Q5: Yes, this is the good structure for cmake. For the addons, I'll work on the PhysiBoSS one to update MaBoSS version, and ensure MSVC compatibility. Q6: Do we want to have more verbosity, such as Wall or pedantic ? Or is it by default ? Q7: Oldest Ubuntu LTS which is still maintained (22.04) ships 3.22.1. So maybe make it compatible with that one ? |
|
Suppose a modeler is C++-savvy and includes additional custom C++ files besides custom.cpp. We will want clear instructions for how they should modify the cmake file(s). |
|
Testing on Windows:
I'm just trying to minimize what a modeler needs to do to build. |
|
@rheiland, I investigated why MSVC OpenMP 2.0 could not compile PhysiCell and it was a simple fix. I just had to use signed integers in omp pragmas. |
|
Regarding the documentation - point me to the most suitable place in the repo, and I can draft something! |
Add CMakeLists.txt support to all sample projects and to PhysiBoSS intracellular project.
To try the new functionality, run:
The top-level CMakeLists.txt just adds various subdirectories containing other CMake files.
Directories
BioFVM,coreandmodulesdefine libraries, that will be (re)used to build sample projects.In
sample_projectsdirectory, each sample project contains the same CMakeLists.txt which is responsible for building that project.Intracellular sample projects are slightly different. They reference the library defined in
addonsdirectory. As of now, it is just boolean sample projects ported to CMake. See CMakeLists.txt inaddons/PhysiBoSSfor more info about how the external dependencies are fetched.Currently, the biggest change compared to Make build is that CMake-built executables are located under the specified build directory. For example, biorobots executable and config files built under
buildwill be located inbuild/sample_projects/biorobots/directory.