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

New folder structure for C++ code #3012

Merged
merged 15 commits into from
Nov 13, 2024
Merged

New folder structure for C++ code #3012

merged 15 commits into from
Nov 13, 2024

Conversation

daljit46
Copy link
Member

@daljit46 daljit46 commented Sep 24, 2024

This PR aims to partially address #2824 and provide a new folder structure for building C++ code. In particular:

  • All C++ code has been moved to cpp with all source files for executables reside in cpp/cmd
  • Everything that was previously in core and src is now built as a single library inside cpp/lib (except for contents in cpp/lib/gui).
  • Version matching logic for library and executables has been updated.

This is still WIP so many things don't work yet, but I'm just posting the changes now to get early feedback on CI.

@daljit46 daljit46 self-assigned this Sep 24, 2024
@daljit46 daljit46 marked this pull request as draft September 24, 2024 14:11
@daljit46 daljit46 force-pushed the new_cpp_structure branch 10 times, most recently from fe28ee2 to 8672261 Compare September 26, 2024 13:17
@daljit46
Copy link
Member Author

daljit46 commented Sep 26, 2024

@Lestropie tests for dirrotate still fail at times https://github.com/MRtrix3/mrtrix3/actions/runs/11053081048/job/30706613851.

@daljit46 daljit46 force-pushed the new_cpp_structure branch 7 times, most recently from 92b0656 to ebdaeca Compare October 1, 2024 12:37
@daljit46 daljit46 changed the title WIP: New folder structure for C++ code New folder structure for C++ code Oct 1, 2024
@daljit46
Copy link
Member Author

daljit46 commented Oct 1, 2024

@MRtrix3/mrtrix3-devs This is now ready for feedback (I'd especially appreciate any input on the new version matching logic).
I wouldn't be against merging this soon even if it will most likely break the status of other PRs. Other than the relocation of C++ files, the changes are fairly minimal so it shouldn't be difficult to merge them into existing PRs.

@daljit46 daljit46 marked this pull request as ready for review October 1, 2024 18:01
@daljit46 daljit46 requested a review from a team October 1, 2024 18:01
Lestropie added a commit that referenced this pull request Oct 2, 2024
Follows on from ec79132 in #3008.
Should resolve issue reported in #3012.
cpp/cmd/CMakeLists.txt Outdated Show resolved Hide resolved
@Lestropie
Copy link
Member

So it looks like as far as filesystem structure is concerned, you've essentially done the following moves:

cmd/  -> cpp/cmd/
core/ -> cpp/lib/core/
src/  -> cpp/lib/

?

Whereas my thinking had been that it would be:

cmd/  -> cpp/cmd/
core/ -> cpp/
src/  -> cpp/

The disadvantage here is that it would no longer be trivial to take a single directory and say "compile everything in this directory into a single shared object". I don't recall the outcome of the most recent discussion here, whether we wanted to remove such a large shared object in favour of many smaller ones or whether to store within a CMakeLists.txt an explicit list of files to be included in such an object.

I'd especially appreciate any input on the new version matching logic

Are you able to give a concise description of how it differs? I can see some renaming and some moving of code, but change to the logic isn't jumping out at me.

Lestropie added a commit that referenced this pull request Oct 2, 2024
Follows on from ec79132 in #3008.
Should resolve issue reported in #3012.
@daljit46
Copy link
Member Author

daljit46 commented Oct 16, 2024

In the meeting this morning with @jdtournier and @bjeurissen, it was discussed that since the separation between core and src is no longer a benefit, all files inside core should be moved to cpp/lib. However, as suggested by @jdtournier, I do believe that it's a good idea to keep a separation between library code and executables and that .cpp command files should not reside in the same directory as library files.
Additionally, we discussed the idea of moving all GUI code up one level so that there would be three folders inside cpp: cmd, lib and gui (or libgui).
@Lestropie any thoughts are welcome.

@jdtournier
Copy link
Member

Thanks @daljit46 - just one minor correction: this is the structure that I think is most logical (though not necessarily the most convenient):

└── cpp/
    ├── cmd/
    └── lib/
        ├── core/
        └── gui/

Both folders in cpp/lib/ would produce libraries (whether dynamic or static, that's another debate), while everything in cpp/cmd/ produces executables. I think that filesystem layout most closely matches how I would envisage the code hanging together.

We also had a debate about what to name the folder for the main library, as there was a suggestion to name it headless/ or cli/, but @bjeurissen made the point that this might actually give the impression that this contains code that is exclusively for terminal-based apps, which clearly isn't the case: the functionality in the main library will clearly be part of any GUI apps as well. So we thought sticking with core/ was a closer reflection of its role than these other alternatives.

@daljit46 daljit46 force-pushed the new_cpp_structure branch 2 times, most recently from c51c9c4 to 9d8be18 Compare October 16, 2024 17:55
@daljit46
Copy link
Member Author

Thanks @daljit46 - just one minor correction: this is the structure that I think is most logical (though not necessarily the most convenient):

└── cpp/
    ├── cmd/
    └── lib/
        ├── core/
        └── gui/

Just rebased to implement this structure.

@Lestropie
Copy link
Member

I find that clunky, having two sequential filesystem hierarchy levels that only discriminate between two sub-directories. I personally wouldn't choose that over the intermediate:

└── cpp/
    ├── cmd/
    ├── core/
    └── gui/

This requirement:

Both folders in cpp/lib/ would produce libraries (whether dynamic or static, that's another debate), while everything in cpp/cmd/ produces executables

takes very minimal CMake code; I don't see why that distinction has to be made at the filesystem level.

@jdtournier
Copy link
Member

Like I said, most logical, though not necessarily most convenient...

The structure you're suggesting is actually what I had in mind initially, and what I'd prefer in practical terms. The lib/ folder is only there to explicitly signal the intention, but I agree it's otherwise entirely redundant. I'm quite happy to ditch the lib/ bit, personally.

@daljit46
Copy link
Member Author

I think the distinction between cmd and lib is mostly on philosophical grounds. The idea is to mark a clear delineation between library code and executables. Ideally, our CMake build files would be "in harmony" with how the files are laid out on disk. Having that said, I'm also not opposed to giving this up. Perhaps, an alternative naming convention could be:

└── cpp/
    ├── cmd/
    ├── libcore/
    └── libgui/

- All C++ code has been moved to a new `cpp` folder.
- C++ commands now reside in `cpp/cmd`.
- We now build a single (shared by default) library that packages all 
non-gui code previously in `core` and `src` (now in `cpp/lib`).
- All gui code in `cpp/lib/gui` is still built separately as before.
- Version matching logic for library and executables has been updated.
By setting, CMAKE_FIND_FRAMEWORK=LAST we instruct CMake to not look
for header files inside /Library/Frameworks as a first search path.
This can cause issue when frameworks like Mono are present on the system,
resulting in CMake unable to find the homebrew library headers.
The strategy now matches what we do to get the library version.
This ensures that the version string of library and commands live in
separate binary objects. The version matching logic has been moved from
app.cpp to command.h to accomodate for this.
- `cpp/lib` is now split between `core` and `gui`, the former contains all code previously under `core` and `src`
while the latter contains only GUI code for `mrview` and `shview`

- `mrtrix-headless` has been renamed to `mrtrix-core`
@daljit46
Copy link
Member Author

As discussed in the meeting earlier, the structure is now:

└── cpp/
    ├── cmd/
    ├── core/
    └──gui/

Additionally, gui/opengl/gl.h has been renamed to gui/opengl/glutils.h to avoid conflicts with system headers.

We ensure that glutils.h is included prior to gl_core_3_3.h since it
indirectly includes Qt's OpenGL headers (which breaks the build).
@daljit46
Copy link
Member Author

@Lestropie @jdtournier ready for review and merge.

@daljit46 daljit46 merged commit 39d3488 into dev Nov 13, 2024
5 of 6 checks passed
@daljit46 daljit46 deleted the new_cpp_structure branch November 13, 2024 11:35
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.

4 participants