Skip to content
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

Experimental CMake support #2620

Closed
wants to merge 36 commits into from
Closed

Experimental CMake support #2620

wants to merge 36 commits into from

Conversation

daljit46
Copy link
Member

This adds experimental support for building MRtrix using CMake. At the moment, it's just a quick proof of concept, but it should compile fine (at least on Windows and Linux, MacOS still needs some work).

Why CMake?

CMake is the defacto standard tool for building cross-platform C++ projects as it's the most widely adopted build system in the C++ world. It is open source and it supports all the major operating systems (Windows, Linux and MacOS).

One of the significant benefits of using CMake is that integrates well with most existing C++ IDEs (e.g. Qt Creator, Visual Studio, Visual Studio Code, CLion, KDevelop, Eclipse, etc...). This integration allows developers to build and debug their code within their chosen IDE with little to no setup or configuration. CMake also nicely interoperates with other tools such as code analysis tools, test frameworks and package managers.

Also using CMake will allow us to slim down the current code for building MRtrix since it will automatically handle some of the stuff the current Python script is doing manually (e.g. handling of different build types, running Qt's MOC and resource compiler, compilation instructions, etc...).

Furthermore, since CMake is so widely known, it can make the build process more accessible to other developers, reducing the barrier to entry for new contributors to the project. The current Python script is heavily specialized for the needs of MRtrix, but may feel somewhat foreign to newcomers.

A good resource for CMake is An introduction to Modern CMake.

Some specific issues that are probably worth discussing:

  • Project structure. We should design a suitable project structure that plays nicely with CMake idioms. Ideally, each subcomponent of the project should have its own CMakeLists.txt file. Effectively this shouldn't be too disruptive since the current codebase is already organised to facilitate this, but further considerations are probably needed.

  • Minimum CMake version. Ideally we want to use a modern version of CMake to make use of best practices and nice quality of life features. The latest version at this point in time is 3.26.3, but this may not be availble on all systems by default. In practice, this may not be too much of a concern as users can install the latest CMake from pip (pip install cmake) or download official prebuilt binaries.

  • Globbing. CMake allows the use of "globbing" to generate a list of files that match a certain regex expression (e.g. file(GLOB SRCS *.cpp) will generate a list containing all cpp files in the current directory), which can then be used to build those files. This approach is similar to how the current Python script works, but it is generally considered bad practice (see Why is cmake file GLOB evil and here). We should consider whether it makes sense to use this in MRtrix even if it goes against conventional wisdom.

  • Package manager. I integrated vcpkg with this project to handle thirdparty dependencies. MRtrix currently uses header only libraries so this is not necessary. However, a package manager may make versioning and future dependencies easier to maintain.

  • Installation of build artifacts. CMake promotes a clean delineation between source trees, build trees and installation trees. The current build system doesn't really distinguish between build and install stages and handles things quite differently. So this also needs to be discussed.

Any feedback on this experiment is very welcome!

@daljit46
Copy link
Member Author

daljit46 commented Apr 27, 2023

I have spent some time investigating on whether it'd be possible to maintain the current build workflow for domain specific sources in src. As we discussed on Wednesday, the current CMake code builds everything in src as a static library which then gets then linked by the various command executables. This works reasonably well, but one clear issue with this approach is that a modification of anything inside src triggers a relinking of the static library for every command (which may take some time). If we consider this workflow to be essential, then this approach is far from ideal.

The current Python build script avoids this by using a custom dependency logic which only rebuilds the affected object files by the change and then rebuilds only commands using those objects. CMake doesn't provide an analogous workflow (I've asked on the cpplang Slack forum about this and a CMake dev confirmed to me that indeed CMake doesn't support such mechanism because it would be practically impossible to create something that works with every build system that CMake needs to support on all platforms). Also see here.

I think it may be worthwile splitting the static library into multiple static libraries using the existing folder structure inside src (so instead of having one mrtrix-main library, we would have mrtrix-connectome, mrtrix-dwi, mrtrix-degibbs, etc...). Then we can link individual commands to the various static libraries depending on the functionality they need. This is clearly more verbose than the current approach, but it will minimise the radius of impact of a given change inside src. Furthermore, this will provide a clearer view of the dependencies of each command.

Let me know if anyone has any thoughts about this.

@jdtournier jdtournier force-pushed the cmake_experimental branch 2 times, most recently from 825ebc6 to c2c6e08 Compare May 5, 2023 09:10
@jdtournier jdtournier force-pushed the cmake_experimental branch 3 times, most recently from cda6d57 to a496eec Compare May 5, 2023 20:10
@daljit46
Copy link
Member Author

daljit46 commented May 25, 2023

As we discussed in our last meeting, I thought it might be a good to give an update on the various considerations that we've made to build MRtrix using CMake and different possible routes that we've considered.

Currently, the codebase is structured primarily into three parts: the core library (built by default as a shared library), the commands (in cmd) built as separate executables and the src folder which contains are shared amongst various commands to varying degrees and non-uniformly. The build script is specifically tailored to build each command by automatically determining the sources files that are needed for each command, and then determines what libraries need to be linked to each command by recursively parsing the header files included in the source of each command. This works well, build times are fast and use of parallelism allows making full use of the hardware. Also, recompilation following a change is minimised (although only to a certain extent).

Switching to CMake presents various challenges. In CMake, everything is built around the idea of "targets". In general, a target is either a library (shared or static) or an executable. The idea is that the developer explicitly specifies the dependencies between targets and CMake will take care of generating the appropriate linking and building mechanisms for the chosen generator by the developer (note that CMake is a build system generator and not really a build system).

Now clearly, our codebase isn't structured to fit this paradigm: we don't specify the dependencies between targets, the build script does it automatically. In our codebase we only have one library (the core library) but the commands (executables) are built using a custom logic injection (in the build script) that is somewhat hidden to the developer. In CMake world, everything should be explicitly mentioned and even techniques like "globbing" a folder of source files and assigning it to target is considered bad practice (for good reasons as CMake needs to work with a lot of different build systems). The developer is invited to manually list each dependency of a given target.
A major difficulty for us is this: how do we tell CMake that for example what libraries mrview need to link against or what include directories it requires? Our custom build script can do it, but there is no easy way to specify this when using CMake.

In light of this here are a few approaches that are possible (there are many details I purposefully left out):

  1. The first approach I considered was aimed at completely minimising any structural changes to the codebase. It consisted in simply building everything inside src as a static library (the core shared library in turn was linked to this library) which gets then linked to every command. This worked fine but it had one major obstacle: every time you made a change to source file in src, you would need to relink it against all the commands (which can take a few seconds or more depending on your hardware and platform). The problem could be mitigated (only to an extent) by splitting the static library into smaller libraries (for example by following the structure inside src) and thus minimising the radius of impact of any change in src (although this may raise other issues). Another disadvantage was that each command linked against Qt, but that could be potentially avoided by separating the gui code from the headless code.

  2. The second approach would be to build everything inside core and src as a shared library (or even two separate shared libraries). This is essentially the same approach as the first one with the advantage that changes to sources in the shared library will not necessarily trigger (unless there is an API change) a relinking of every command (CMake does relink by default, but we can tell it not to). This should avoid the problem mentioned in the first approach. On the top of my head I can't think of major disadvantages of taking this route (well at least from a build standpoint), but it will mean that we would now ship the entire codebase as a library (other than the commands) while previously this was limited to core.

  3. The third approach would consist in manually specifying for each command target the list of source files required and the list of dependencies it needs. This essentially avoids the relinking problem in 1 (as there would be no library to link against and each command executable will only compile what it needs). The explicit nature of this approach can appear to be tedious (and initially it would be) as this does require the developer to be responsible for any change or addition to the codebase (e.g. by requiring to manually specify the dependencies of a new command).

  4. The fourth approach we have considered (suggested by @jdtournier) and the one that we are currently experimenting with is to create a Python script that automates the manual process described in 3. Essentially, before running CMake one would have to run a Python script that will generate a CMake file that explicitly lists the dependencies of each command. To do this, the scripts iterate through each command and figures out the needed dependencies use the header includes of each source. This approach takes the burden of manually specifying each command by relegating the responsibility of updating/changing the cmake files to the script. The obvious downside is that we have now introduced a third "CMake generation" stage that needs to happen prior to the CMake configure stage. This is non-standard behaviour that is foreign to most codebases and doesn't follow the idiomatic CMake way of doing things (and it does also share some of the fragility of the current build script).

@daljit46
Copy link
Member Author

daljit46 commented Jun 7, 2023

I have spent time further investigating options 1 and 2 further.

Our primary concern with building everything in srcas a static library was that the static library would need to be relinked against all commands for any change inside src. It seems that this worry was somewhat misplaced. When I first tested this approach I was using the standard GNU linker on Linux, but I recently I tested the same approach using lld, the LLVM linker. I found that using lld, the time taken to relink all executables is less than 2 seconds a second on my MacBook 16 2023 (using the instructions here) compared to 5–6 seconds using the Apple linker. I've also tested this on Ubuntu (using -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=lld). On my Acer laptop with an i5-1135G7 @ 2.40GHz, I found that the linking time is about 3 seconds. In fact, on Linux, you can get even better linking times using mold. These timings are very good in my opinion and should be quite negligible in our typical workflow.

The only platform where linking times are a concern is MSYS2 on Windows. Unfortunately, the MSYS2 "emulation" slows down the linker and (using lld) I found that it takes about 17 seconds to relink all command on my Acer laptop. However, since this is not the main platform that we use to develop MRtrix (on WSL there is no problem and furthermore we might be able to directly use clang on Windows once we get rid of our POSIX specific code), I'm not sure how big of a concern this is.

I have also tested the second approach more thoroughly and built everything inside src as a shared library and as predicted this works well (and avoids the unnecessary relinking as predicted). I think we should seriously consider this approach, because it seems the easier to maintain and use out of all the 4 approaches outlined above. MRtrix already ships with the core library built as a dll, and thus it wouldn't be too dramatic to ship the entire codebase other than the commands as a shared library too.

Currently, I've encountered some problems using approach 4 since object libraries in CMake don't have the same status as static (or shared) libraries and the generated code by the Python script runs into cyclic dependencies issues. I'm not sure whether there is a particularly good solution for the problem, but more investigation is needed to determine this. Also, in light of the issues just mentioned, I'm less convinced that this approach is actually worthwhile from both a usability and implementation point of view.

@daljit46
Copy link
Member Author

daljit46 commented Jun 12, 2023

As mentioned above, using the Python script (approach 4 above) to generate the list of targets in CMake, runs into cyclic dependencies issues. The script generates a list of objects (for all .cpp files in src) and a list of targets (for all commands). The list of objects is produced using CMake object libraries, and CMake only handles cyclic dependencies between static libraries (but not object libraries).
Now there is one possible workaround for this issue that can work. The reason why the Python scripts generates a list of objects is to ensure that an object is compiled only once (CMake builds source files separately for each target, even if the same source file has been previously built for another target). One can directly list the targets by directly specifying the sources each of them needs. Of course, this would mean that if two commands use the same source, this will need to be recompiled twice, leading to slow build times. However, if one uses a tool like ccache or sccache then this problem wouldn't be there since the compilation of .cpp files will be cached and if the compiler needs to recompile the same object again then it will be retrieved from the cache.
Thus, full compilation times will be almost as fast (there would a minor hit due to retrieving the cached objects) and incremental builds would be practically unaffected.

@daljit46
Copy link
Member Author

OK, so it turns that the cyclic dependency issues weren't actually a problem. The python build script (unnecessarily) linked object libraries for the cpp sources to each other. I realised that this wasn't necessary, as all required object files will be included in the target commands anyway. I tried to build (non-gui) commands without the unnecessary linking and things seem to work.

@Lestropie
Copy link
Member

The other aspect that needs to be reconsidered here is the Python API and algorithm-based scripts. From my latest test of cmake_experimental_shared (where FFTW now seems to configure and build properly on MSYS2), the installation step drags the Python scripts residing in bin/ into the installation directory, but it does nothing about the contents of lib/mrtrix3/. If committing to cmake, we are presumably free to choose where they go at installation.

@daljit46
Copy link
Member Author

daljit46 commented Jul 20, 2023

I think it's probably a good idea to give a short summary of the progress made on trying to build MRtrix3 using CMake for future reference.
I have created a new branch called cmake_experimental_shared to see whether approach number 2 list above is suitable. A separate branch was created as it wasn't clear to me whether this approach would work well according to our requirements. However, I think out of all the options we have, this is probably the best one (the only major downside is that building a single command requires you to build all source files in src).
Currently, most things seem to work from a build perspective, but there are two issues that still need some work:

  1. Testing. I slightly changed the layout of the testing directory and separated unit tests into their own folder. The test are incorporated into CMake by running a small function that iterates over all the test scripts and adds a CMake test that internally runs the test in a bash shell. This allows you to essentially keep our current testing strategy intact, as the addition/maintenance of new/existing tests is essentially unchanged. There are a couple of things that need work:

    • Some binary tests fail (e.g. see here) and I still haven't figured out exactly why.

    • There is no support for testing Python scripts (which is also related to the issue below).

  2. Python scripts. Currently, our Python scripts are separated out in two separate folders, bin and lib. This is in light of the fact that previously, we had no clear delineation between source directory and build directory. However, I think it no longer makes sense to maintain this folder structure if the build directory is separate from the source directory (which is how we should use CMake). Thus, my idea was to move all Python scripts into a separate folder called Python. I guess it would still make sense to keep a separation between bin and lib as if the user wants to install MRtrix3 on their system (e.g. via cmake --install), it's probably not a good idea to place "library" scripts in the same place as "executable" scripts (at least on Unix systems). @Lestropie let me know if you have any thoughts on this.

@Lestropie
Copy link
Member

Don't be afraid to separate out into a new PR for the cmake_experimental_shared branch if you're confident that that's the right thread to be pulling on, and then any derivative discussions are specific to that branch (and any relevant commits will appear in order on the PR page).

  1. Testing:

    1. Currently, the "testing/" sub-directory obeys the requisite filesystem structure to operate in the same way as an MRtrix3 external project, and therefore executing the build script from that location (which is done internally by the run_tests script) builds the unit tests. I'm inferring from your last post that your intention is to have drastically different solutions for external projects vs. tests?
    2. In case it is relevant, I should note that in Npy file format support #2437 I have the first introduction of Python scripts that are used to evaluate the operation of C++ unit tests. This is quite different to the current "Python tests", which are testing the execution of MRtrix3 Python command-line scripts.
  2. Python scripts:

    1. Firstly, I will reiterate here that when I first started creating the MRtrix3 Python API, that was my first experience with Python. So there's likely quite a lot in there that's not considered kosher.
    2. The primary distinction will indeed be between CLI-executable and not. However there's also the weird distinction between algorithm code, which is specific to one executable script and currently resides in lib/mrtrix3/<script_name>/, and "the API" that is common across all executable scripts. Having those share a root directory feels incorrect. I'm happy to consider any proposed alternatives especially in the context of a CMake restructuring.
    3. We've always wanted to have a certain degree of assurance that when we execute an MRtrix3 Python script, the versions of the API and any algorithms that get invoked are from the same installed version of MRtrix3 as the script we are invoking. We've tried a few different techniques for this over the years. Current solution involves bin/mrtrix3.py and the import mrtrix3; mrtrix3.execute() line at the end of any executable script.

@daljit46
Copy link
Member Author

Yes, I think it may be a good idea to open a new PR that supersedes this one.

For the Python scripts, I have now moved bin lib and share in a new folder called python (see 4440623), but the structure of the code is still the same. Scripts are now copied into the build and installation directories inside the bin folder (where C++ commands will also be found), while the lib folder contain all the library scripts.
We can probably discuss if more drastic changes are warranted later.

@Lestropie
Copy link
Member

  • Depending on the installation targets, it might no longer be necessary to have a "mrtrix3/" sub-directory within lib/ and share/. That was all intended to preserve FHS conformance in the case where one does a basic copy of content to eg. /usr/. But if instead expecting an explicit installation step, those might be considered redundant.
  • share/ content is not specific to Python; some of it is specific to C++ binaries.

@daljit46 daljit46 mentioned this pull request Jul 27, 2023
6 tasks
@daljit46 daljit46 closed this Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants