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

Filesystem structure post cmake (inc. testing) #2824

Open
Lestropie opened this issue Feb 26, 2024 · 6 comments
Open

Filesystem structure post cmake (inc. testing) #2824

Lestropie opened this issue Feb 26, 2024 · 6 comments
Labels

Comments

@Lestropie
Copy link
Member

In recent discussions it sounds like the filesystem structure of the source code is likely to change on dev following the transition to cmake. This includes potentially removing the distinction between core/ and src/, though that isn't the focus here.

@daljit46 mentioned a preference for having cpp/ at the root directory, in distinction to the current python/ root directory.

In mucking around with the content relating to testing, it occurred to me that there we have "binaries" and "scripts" appearing, both in the filesystem structure and in the invocation of specific tests. It may well be the case that adopting the "cpp" vs "python" distinction here would make things more consistent.

@Lestropie
Copy link
Member Author

  • In the source repository, share/mrtrix3/ should be moved to just share/; cmake should create the additional parent directory in the build process.

@daljit46
Copy link
Member

daljit46 commented Aug 2, 2024

Working towards #2901, I think it'd be good separate Python and C++ code into separate directories. @MRtrix3/mrtrix3-devs unless you have any objections, I will make a PR to implement and merge this on dev

@Lestropie
Copy link
Member Author

I'm happy to proceed with some restructuring, I'm just not sure how much consensus there is on what the target structure is. My own list would be:

  • Merge core/, and src/, into cpp/
    Possible exception to this might be if we were to want to encourage construction of a cpp/ sub-directory per C++ command, so that for more complex commands code can be split across multiple files rather than enormous cmd/*.cpp files. But myself I think I'd prefer to encourage persisting with how we've been doing it, which is placing features depending on the feature itself rather than the first command in which it is used, as sometimes such functionalities later become useful in some other command.
    • Would the plan be to have cpp/cmd/*.cpp per command?
  • Move share/mrtrix3/ into share/
    (but to be placed in share/mrtrix3/ in build / install directories)
    (not 100% sold on this)
  • Rename "binaries" / "scripts" to "cpp" / "python" wherever used

The main one for which I'm not sure how much agreement there might be:

  • Merge MRtrix3/script_test_data repo into MRtrix3/test_data, and merge testing/binaries/ and testing/scripts into testing/.
    run_tests is no longer there to only pull test data for Python commands if testing of a Python command is attempted. If tests are to be constructed at configure time, data from both testing repositories needs to be pulled. Hypothetically we could re-instate this split, if only to reduce the size of the download for any CI run that only does testing of C++ binaries. But with greater priority being placed on readability and sensible structure over adding shortcuts to minimise CI loads, I'm increasingly in preference of merging them together. It would also simplify any logic around copying test data into the build directory and setting working directory for executing tests.

@daljit46
Copy link
Member

daljit46 commented Aug 8, 2024

Merge core/, and src/, into cpp/

I think it makes little sense to keep src. I don't know what the original motivation for choosing "src" name, but I don't think it is appropriate. My suggestion would be to rename to something like "misc".

Would the plan be to have cpp/cmd/*.cpp per command?

Yes, I think we can always change this later if needed.

@Lestropie
Copy link
Member Author

I don't know what the original motivation for choosing "src" name

I think it was the distinction of what got compiled into the singular shared library vs. what didn't.

My suggestion would be to rename to something like "misc".

My original point about "merging core/ and src/ into cpp/ may not have landed given this question.
I'd envisaged that that distinction would disappear, and that all current contents of core/ and src/ would be blended together within cpp/. So eg. core/dwi/gradient.h and src/dwi/tensor.h would become cpp/dwi/gradient.h and cpp/dwi/tensor.h. Not what I'm pushing for, just where I thought we were headed, and in that case there'd be no need for a cpp/misc/.
It would raise the question of how the build itself might change, given that "core" and "headless" distinctions in shared library construction makes less sense.

@daljit46
Copy link
Member

It would raise the question of how the build itself might change, given that "core" and "headless" distinctions in shared library construction makes less sense.

Yes, I think then it would be appropriate to build everything as a shared library, at least preliminarily. For optimising build times and improving the codebase's design (with CMake in mind), it may be worthwhile to have each folder in src (and core) compiled as a static library. These static libraries would then be compiled into a final libmrtrix library.

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

No branches or pull requests

2 participants