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

Fix python CLI CI test #2846

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Fix python CLI CI test #2846

merged 7 commits into from
Mar 11, 2024

Conversation

Lestropie
Copy link
Member

@Lestropie Lestropie commented Mar 5, 2024

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.

daljit46 and others added 2 commits March 6, 2024 10:32
This adds a new function argument to add_bash_tests() to specify
environment variables that should be defined for running a test.
@Lestropie Lestropie self-assigned this Mar 5, 2024
@Lestropie Lestropie changed the base branch from python_cmd_changes to dev March 5, 2024 23:43
@Lestropie Lestropie changed the base branch from dev to python_cmd_changes March 5, 2024 23:45

This comment was marked as outdated.

Interface of sed differs between MacOSX and Unix, such that the CI test of only the former would fail.
@Lestropie Lestropie force-pushed the fix_python_cli_ci_test branch from 1974fee to d9587e6 Compare March 6, 2024 01:30
Copy link

github-actions bot commented Mar 6, 2024

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member Author

d9587e6 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 Lestropie changed the title Fix python cli ci test Fix python CLI CI test Mar 8, 2024
@Lestropie
Copy link
Member Author

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.

Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie
Copy link
Member Author

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

@daljit46
Copy link
Member

daljit46 commented Mar 11, 2024

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)

Lestropie and others added 2 commits March 12, 2024 08:43
@Lestropie Lestropie force-pushed the fix_python_cli_ci_test branch from a3048fd to 08bc526 Compare March 11, 2024 21:52
…hon_cli_ci_test

Conflicts:
	testing/unit_tests/python_cli
Echoes changes in 08bc526 and d9587e6 for the Python CLI tests.
@Lestropie Lestropie merged commit f57bbd8 into python_cmd_changes Mar 11, 2024
1 check passed
@Lestropie Lestropie deleted the fix_python_cli_ci_test branch March 11, 2024 22:28
Copy link

clang-tidy review says "All clean, LGTM! 👍"

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

Successfully merging this pull request may close these issues.

2 participants