cmake Python changes

[Lestropie]

Closes #2730.

My third attempt at Python refactoring following cmake transition (#2689), after #2737 and #2741. Done a decent amount of thinking about it after it was last spoken about during a dev team meeting. I'm happy with this one and want to proceed. Commands all seem to execute as expected, from both build and install directories. For existing Python commands, requisite change is minimal; just deletion of the "import mrtrix3; mrtrix3.execute()" at the tail of the file.

Lots of details in commit message of 93f3741c68b1fe8a350b05a155aa7272ff2775e2.

@daljit46: Handballing to you. You have carte blanche to change as you see fit; whether to resolve my cmake naivety, or anything in this proposal that would be completely prohibitive if trying to expand the concept to external project support. But in the absence of any major criticisms from yourself or any other @MRtrix3/mrtrix3-devs, hoping this gets me out of your way to proceed with #2901.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[Lestropie]

• [ ] For Python-API-based testing tools, need to have cmake generate executables just as is done for Python commands.

[daljit46]

Looks mostly fine to me at a first glance.

API files reside in python/mrtrix3/ (still in a sub-directory called "mrtrix3/" so that IDEs can reasonably identify them, but no need to be in a "lib/" sub-directory in source form).

I don't quite the reasoning behind this. How does this help IDEs?

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[Lestropie]

I don't quite the reasoning behind this. How does this help IDEs?

Not actually tested, but I assume that if an IDE sees from mrtrix3 import app, and you try to hotkey from function invocation to declaration, that is far more likely to succeed if file "app.py" resides in a directory called "mrtrix3/"; hence putting those files in python/mrtrix3/ rather than python/.

[daljit46]

I see, that makes sense. Just tested this on VS Code and it behaves as you predicted.

[daljit46]

I think it would make sense for cmake/MakePythonExecutable.cmake to accept a DESTINATION_PATH (or an OUTPUT_DIR) argument instead of BUILDDIR. This way we can use the logic in the script for testing tools or elsewhere.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[daljit46]

I'm a little unsure on how to handle testing_python_cli. Currently, it resides in testing/tools, but I think this is not the appropriate location for it. The command is not even a "tool", in the sense that it's not really used by multiple scripts/executables to verify the correctness of input/outputs for a given command. Instead, it's just a "fake" python command used to test that the CLI works as intended.

Furthermore, currently we create the testing tools in ${PROJECT_BINARY_DIR}/testing/tools (which is added to PATH for any bash test invocation), but perhaps it makes sense to set their build output directory to ${PROJECT_BINARY_DIR}/bin. This has the benefit that whatever mechanism we use for standard Python commands should, in theory, also work for executing a testing tool.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[Lestropie]

RE testing_python_cli:

• If it were the case that our CMakeLists didn't involve globbing, then that file could happily reside in python/mrtrix3/commands/, and just be omitted from the list of generated executables if not building tests. I'm not sure if this case is sufficient to make that transition, just making the observation.
• While it's true that the number of tests that invoke this command is only 1, I'm not sure if that distinction is adequate to motivate conceptualising it differently to the other testing tools. It is nevertheless still "a tool that is invoked exclusively by tests".

RE the build path for testing tools, personally I find it a little unusual to look inside of the bin/ directory and see commands in there that are exclusively for testing. Eg. There's a command called to. So if anything I'd prefer to be moving those "unit test" test commands out of bin/ and into testing/tools/, rather than moving anything in the opposite direction.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[daljit46]

I have made some changes to fix the running of unit tests. cmake/MakePythonExecutable.cmake now accepts an OUTPUT_DIR directory where the output executable will be generated, and also takes an optional list EXTRA_PATH_DIRS that can be used to specify additional paths to be prepended to sys.path. In testing/tools/CMakeLists.txt, now there is a new function add_python_tool which can generate a Python test executable that behaves like a standard MRtrix3 python command (using MakePythonExecutable.cmake). This is a slightly cumbersome approach, but it does seem to work. @Lestropie any feedback welcome.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[Lestropie]

Yep, that solution for testing_python_cli looks exactly like what I had in mind.

Python command documentation generation is failing on Linux CI: find: '/home/runner/work/mrtrix3/mrtrix3/python/bin/': No such file or directory Is this maybe a cache problem? The workflow makes no reference to that deprecated location.

Once that's resolved I'm happy for this to be merged.

Edit: Was docs/generate_user_docs.sh, not build cache.

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[Lestropie]

Spoke too soon; one of the changes you made to the cmake-generated Python executables to facilitate testing_python_cli broke the execution of some of the Python commands.

Full details in 3fce6484a071acd6ea703b05bdf9e21064153629, but what I chose to do is put testing_python_cli.py into ${PROJECT_BINARY_DIR}/lib/mrtrix3/commands/ alongside the other command source code, and generate the executable in ${PROJECT_BINARY_DIR}/bin; that way the same logic for the executable file generation applies.

See if you're happy with that. Assuming there isn't anything outstanding still broken, I'm happy to proceed with merging.