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
291 stars 179 forks source link

Fix python CLI CI test #2846

Closed Lestropie closed 6 months ago

Lestropie commented 7 months ago

Testing whether the proposed change in #2845 facilitates executing the Python CLI unit tests in CI for #2678.

I do realise in retrospect that this solution might not actually live that long (even if it's a useful capability to leave in place): with #2741, the Python CLI test would need to have the same mechanism of executable binary generation.

github-actions[bot] commented 7 months ago

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

github-actions[bot] commented 7 months ago

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

Lestropie commented 7 months ago

d9587e650a19c5fc4a54bfac8c654a356d387b13 got the new Python CLI test working on both Unix and MacOS, but the Windows CI test fails, with command testing_python_cli unable to find the mrtrix3 Python module.

Lestropie commented 6 months ago

On Linux:

sys.stderr.write(repr(os.environ))
[..., 'PYTHONPATH': '/home/unimelb.edu.au/robertes/src/mrtrix3/build_cmake/lib/', ...]
sys.stder.write(repr(sys.path))
[..., '/home/unimelb.edu.au/robertes/src/mrtrix3/build_cmake/lib', ...]

On MSYS2:

sys.stderr.write(repr(os.environ))
[..., 'PYTHONPATH': 'C:/msys64/home/rob/src/mrtrix3_cmake/build_cmake/lib', ...]
sys.stder.write(repr(sys.path))
[..., '/home/rob/src/mrtrix3_cmake/testing/data/C', '/msys64/home/unimelb.edu.au/robertes/src/mrtrix3/build_cmake/lib', ...]

Setting PYTHONPATH via ENVIRONMENT is being done with ${PROJECT_BINARY_DIR}; this seems to be yielding an absolute path from the drive root with Windows-style drive separator; this gets corrupted when python reads it and appends sys.path.

github-actions[bot] commented 6 months ago

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

Lestropie commented 6 months ago

Looks like my attempted hack fix didn't work; @daljit46 this might need your expertise.

daljit46 commented 6 months ago

Differences between path handling between Windows and Unix systems is always a pain. Fortunately, MSYS2 ships with a tool called cygpath which allows you to convert between the different formats (if you look at the code in cmake/BashTests.cmake you can see that we're using this tool already for exec_dirs).

For this specific case, I think it's best to set the environment conditionally based on the OS. We could add some logic to do this automatically in BashTests.cmake but since the ENVIRONMENT property doesn't need to restrict itself to file paths only it's probably best not to.

So I propose the following:

set(PYTHON_PATH "${PROJECT_BINARY_DIR}/lib")
# On MSYS2 we need to convert Windows paths to Unix paths
if(MINGW AND WIN32)
      EXECUTE_PROCESS(
          COMMAND cygpath -u ${PYTHON_PATH }
          OUTPUT_VARIABLE PYTHON_PATH
          OUTPUT_STRIP_TRAILING_WHITESPACE
      )
endif()

# Then set environment
add_bash_tests(
    ...
    ENVIRONMENT "PYTHONPATH=${PYTHON_PATH}"
)

This should do the job (seems to work on my local MSYS2 installation)

github-actions[bot] commented 6 months ago

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