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

Change build system to CMake #2689

Merged
merged 37 commits into from
Oct 19, 2023
Merged

Change build system to CMake #2689

merged 37 commits into from
Oct 19, 2023

Conversation

daljit46
Copy link
Member

@daljit46 daljit46 commented Jul 27, 2023

This PR supersedes #2620 (which you should check out for the motivations for this proposal). It adds support for building MRtrix3 using CMake.
A major change is that we now build all the code (except for the commands) as a shared library that links to each executable.
As far as I can tell, this is branch should be feature complete and shouldn't miss any major functionalities compared to its base branch. Instructions to build and test can be found here.

What's missing?

  • Tweaking of CI checks to match the settings of our current workflows.
  • Addition of secondary checks in the CI to run Pylint and update copyright headers.
  • Handling of the files inside the share directory.
  • Removal of possible redundant files that are closely tied to the old build system.
  • Other minor details in the CMake scripts.
  • Update documentation.

All of the above would need to be discussed before this is ready for merging.
@MRtrix3/mrtrix3-devs if there is anything needs to be added or changed or something I missed, please let me know.

@daljit46 daljit46 force-pushed the cmake branch 2 times, most recently from 4940d70 to 198c086 Compare July 28, 2023 10:15
@daljit46 daljit46 force-pushed the cmake branch 9 times, most recently from 69012d9 to 4fa388c Compare August 7, 2023 11:19
@daljit46
Copy link
Member Author

daljit46 commented Aug 8, 2023

@MRtrix3/mrtrix3-devs For the full documentation of the new build system, shall I just create a new document that details how to build the project? Or is it better if I wait until we actually start using the branch in practice to obtain more feedback?
The cmake-instructions file provides the basics but a more thorough explanation is probably warranted.

Furthermore, we currently have two documents (one in the user documentation directory and the other in the developer documentation), both of which explain the build process with significant overlap. I propose that the we should get rid of the any documentation regarding building MRtrix3 from the user documentation and instead provide a single page where the CMake build process is documented for developers.

daljit46 and others added 11 commits October 19, 2023 12:27
Previously, mrview required special handling of QFileOpenEvent and thus bool QApplication::event(QEvent*) was defined mrview.cpp. This strategy is no longer possible since now the code in src is built as a shared library, which requires the function to be defined in the shared library. To get around this we instead rely on a custom event handler using std::function.
Since commands are no longer built in source_dir/bin, we now need to specify the build directory in generate_user_docs.sh
@daljit46 daljit46 merged commit 05878e5 into dev Oct 19, 2023
5 checks passed
@daljit46 daljit46 deleted the cmake branch October 19, 2023 12:17
@Lestropie Lestropie mentioned this pull request Feb 20, 2024
Lestropie added a commit that referenced this pull request Feb 21, 2024
- Remove code within cmd/mrview.cpp that originates from 6417a52 as part of #2689, which was erroneously added in 39240ad.
- Fix compilation of src/gui/mrview/file_open.cpp to coincide with changes in #2764; these were omitted from cherry-picks 39240ad and 0afe37b due to that file having already been removed during the course of #2689.
Lestropie added a commit that referenced this pull request Feb 21, 2024
Resolves changes made to cmd/mrmath.cpp in #2784 to prevent memory leak against removal of MR::vector in #2689.
Lestropie added a commit that referenced this pull request Feb 21, 2024
Construction of a 3.1.0 release involves taking features on the development branch and separating them from the adoption of cmake in #2689. Some of these changes involve the use of C++17 features. However the transition to defaulting to C++17 was performed exclusively within the cmake ecosystem. This commit modifies the legacy "configure" script to reflect the increment to use of C++17.
Lestropie added a commit that referenced this pull request Feb 28, 2024
- Some limited modification of code structure to be more faithful to C++ changes in #2818.
- Resolution of documentation changes occurring in #2678, particularly in its merge to dev following cmake adoption (#2689).
@Lestropie Lestropie mentioned this pull request May 7, 2024
9 tasks
@Lestropie
Copy link
Member

@daljit46 Do you want to have a go at updating this wiki page to also reflect this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants