-
Notifications
You must be signed in to change notification settings - Fork 132
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
Use posix_prefix scheme to avoid 'local' in path #386
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@@ -46,7 +46,7 @@ macro(_ament_cmake_python_get_python_install_dir) | |||
set(_python_code | |||
"import os" | |||
"import sysconfig" | |||
"print(os.path.relpath(sysconfig.get_path('purelib', vars={'base': '${CMAKE_INSTALL_PREFIX}'}), start='${CMAKE_INSTALL_PREFIX}').replace(os.sep, '/'))" | |||
"print(os.path.relpath(sysconfig.get_path('purelib', vars={'base': '${CMAKE_INSTALL_PREFIX}'}, scheme='posix_prefix'), start='${CMAKE_INSTALL_PREFIX}').replace(os.sep, '/'))" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question; is "posix_prefix" the correct thing to do on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not. It does change the paths away from what distutils used to report.
>>> import sysconfig
>>> sysconfig.get_path('purelib', vars={'base': '.'})
'Lib\\site-packages'
>>> sysconfig.get_path('purelib', vars={'base': '.'}, scheme='posix_prefix')
'lib\\python3.10\\site-packages'
>>> from distutils.sysconfig import get_python_lib
>>> get_python_lib(prefix='.')
'.\\Lib\\site-packages'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could mirror the same logic as cpython to conditionally enforce the scheme using os.name
:
https://github.com/python/cpython/blob/7cdaf87ec50f76c934ba651256484c4624b84ef2/Lib/sysconfig.py#L278-L295
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh - even better. scheme='posix_prefix' if sysconfig.get_default_scheme() == 'posix_local' else None
.
That should only enforce the scheme if the debian hack is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drat - None
doesn't work for scheme
.
Maybe something like:
get_path_kwargs = {'scheme': 'posix_prefix'} if sysconfig.get_default_scheme() == 'posix_local' else {}
and then pass **get_path_kwargs
to get_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe:
scheme={'posix_local': 'posix_prefix'}.get(sysconfig.get_default_scheme(), sysconfig.get_default_scheme())
or
scheme='posix_prefix' if sysconfig.get_default_scheme() == 'posix_local' else sysconfig.get_default_scheme()
For what it is worth, this (combined with colcon/colcon-core#504) does not seem to fix the problem for me reported in colcon/colcon-core#503 . |
Follow up from #378
Matches colcon/colcon-core#504
Related to colcon/colcon-core#503
This uses the
posix_prefix
scheme to avoid alocal/
in the python path.This should be backported to Humble