MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
294 stars 181 forks source link

Pylint - cmake interaction #2853

Closed Lestropie closed 5 months ago

Lestropie commented 8 months ago

Something that came to mind looking at #2741, but warrants handling separately.

Currently, file _version.py is generated in the source tree, so that CI can run pylint without running the whole build script. This however violates the source code - build directory separation that's been under discussion in #2822 in the context of C++.

(Note that in #2850 this extends to a second cmake-generated file, _commands.py, which stores a list of MRtrix3 commands such that the run module can handle them differently)

Initially, I was going to suggest that script run_pylint could generate that file in the source directory and then erase it upon completion. However that is only a mitigation; it still doesn't do a full purist separation.

I'm wondering if instead the better solution for that particular CI job would be:

Would this be too purist?

daljit46 commented 8 months ago

Currently, file _version.py is generated in the source tree, so that CI can run pylint without running the whole build script. This however violates the source code - build directory separation that's been under discussion in https://github.com/MRtrix3/mrtrix3/issues/2822 in the context of C++.

Yes, I think it'd be good to avoid this. For the CI (in .github/workflows/checks.yml), I think this isn't even a problem because we are manually generating _version.py anyway.

Have an environment variable that configures cmake to completely ignore building of the C++ binaries

Personally, I think it should be fine to just to require building C++ files before running pylint. It's a minor inconvenience but since Python modules depend on the C++ code, it makes quite sense to me.

Also, alternatively we could add a global python-target (which I think would be more idiomatic) so that we can do something like:

cmake --build build --target python-target

That will only build Python files. Then we could implement your suggestion of running pylint in the build directory.

Lestropie commented 8 months ago

For the CI (in .github/workflows/checks.yml), I think this isn't even a problem because we are manually generating _version.py anyway.

From memory it came about because I was running run_pylint locally and file _version.py didn't exist because build used to be responsible for making it. The CI is free to do its own thing, but still want to be able to run this sensibly locally.

Personally, I think it should be fine to just to require building C++ files before running pylint.

Given the adoption of ccache, the wasted CPU time is no longer so much of a factor for this particular check.

Also, alternatively we could add a global python-target

That feels clean; it does however still require running cmake --build just to run a set of pylint tests.


Another possible option would be: rather than requiring a full build, run_pylint could instead generate a temporary directory, create mrtrix3/_version.py within it, and include that directory in PYTHONPATH.

  1. Not sure if this would work having multiple mrtrix3/ module locations in sys.path.
  2. It wouldn't be restricted to just _version.py: with #2850 (specifically 02b0a7e34e7e1bdeae353da55e0aa2f4d8825137) there will also be a _commands.py that would need to be generated.

Unlike cmake --build build --target python-target, it would neither require building nor specifying a build directory when running run_pylint. It would however involve a small amount of fragility in terms of run_pylint and cmake generating files with identical names and similar content but for different purposes. (_commands.py wouldn't actually need to be populated with useful information for the purpose of pylint; it would just need to create the expected variables)

daljit46 commented 5 months ago

Can this be closed in light of #2920 having been merged?