jrl-umi3218 / jrl-cmakemodules

CMake utility toolbox
https://jrl-cmakemodules.readthedocs.io/en/master/
Other
57 stars 45 forks source link

fix test.cmake #569

Closed fabinsch closed 1 year ago

fabinsch commented 1 year ago

append PYTHONPATH using cmake_path instead of list.

This is currently leading to a failure in a PR for eigenpy. I tried the solution locally on a windows machine and ssh into the github CI. After fixing this line, the test passed.

nim65s commented 1 year ago

This is only available for CMake >= 3.20: https://cmake.org/cmake/help/latest/command/cmake_path.html

jcarpent commented 1 year ago

@nim65s Good point. What do you suggest instead?

fabinsch commented 1 year ago

We currently have a problem with a PR https://github.com/stack-of-tasks/eigenpy/pull/325 related to importing modules from a local path in python. We append paths in CMake and when I print them I can see in cmake

PYTHONPATH=C:/Users/ci/workspace/eigenpy/build/python:C:/Users/ci/workspace/eigenpy/build/python/$<CONFIG>:C:/Users/ci/workspace/eigenpy/build/unittest:C:/Users/ci/workspace/eigenpy/build/unittest/$<CONFIG>

but then in python the print of sys.path shows:

['C:\\Users\\ci\\workspace\\eigenpy\\unittest\\python', 'C:\\Users\\ci\\workspace\\eigenpy\\build\\unittest:C:\\Users\\ci\\workspace\\eigenpy\\build\\unittest\\Release', 'C:\\ProgramData\\Miniconda3\\env
s\\proxsuite\\python310.zip', 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite\\DLLs', 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite\\lib', 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite', 'C:\\ProgramData\\
Miniconda3\\envs\\proxsuite\\lib\\site-packages', 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite\\lib\\site-packages\\win32', 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite\\lib\\site-packages\\win32\\lib'
, 'C:\\ProgramData\\Miniconda3\\envs\\proxsuite\\lib\\site-packages\\Pythonwin']

Check the second entry, it shows two paths glued together. This is a problem currently on our windows pipeline here. I did not find a suitable solution yet, the original one proposed in this PR is wrong. If you have any ideas let me know.

gergondet commented 1 year ago

I think this line https://github.com/fabinsch/jrl-cmakemodules/blob/fix-py-test/test.cmake#L162 might be the issue, as far as I know ; is the proper separator for Windows and not : which is why you get two paths concatenated together in your PYTHONPATH

Another issue that I personally faced recently is that since Python 3.8, the search of DLL by the Python interpreter has been modified and new paths need to be added explicitly via os.add_dll_directory. The interpreter will only report that it cannot load the module in that case and that might be confusing as the DLLs are in your PATH (note that you also need to take care of your dependencies e.g. https://github.com/jrl-umi3218/sch-core-python/blob/master/sch/__init__.py). See https://docs.python.org/3/whatsnew/3.8.html#bpo-36085-whatsnew for more details.

fabinsch commented 1 year ago

Ok, I understood why we have this line. The separator for paths on windows is indeed ; - but as we use the test properties ENVIRONMENT which is a semicolon-separated list this is leading to problems -> we have a new ENV variable for each of the paths (without a name). So changing it to : ensured to have all the paths together in the python test but they were glued (also not ideal).

I found that replacing ; by \; could solve this issue. I will adapt this PR accordingly.