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

replace the python version finding mess with cmake builtins #2931

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

nilsnolde
Copy link
Member

fixes #2929

it uses the "new" CMake builtin approach to get the right python lib path from the actual python installation used to build the bindings. Meaning it'll install now to /usr/lib/python3/dist-packages/valhalla instead of /usr/local..., but that's actually better IMO

@@ -30,7 +30,7 @@ RUN rm -f valhalla_*.debug
RUN strip --strip-debug --strip-unneeded valhalla_* || true
RUN strip /usr/local/lib/libvalhalla.a
# TODO: fix https://github.com/valhalla/valhalla/issues/2929 and update this line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# TODO: fix https://github.com/valhalla/valhalla/issues/2929 and update this line

@kevinkreiser
Copy link
Member

i kind of prefer the /usr/local prefix as that is the standard location to install when you build something from source. ie i think its the canonical location for from source builds. but at the same time i like the extreme simplification in the cmakefile. maybe we can merge this can get the /usr/local back later

@nilsnolde
Copy link
Member Author

nilsnolde commented Mar 11, 2021

Well, yeah I agree for valhalla (and other self-contained/-built apps), but if you think about it more, it actually makes sense to let the python framework you're using to build the bindings tell you where to install them. It installs to /usr/local if your python installation is also at /usr/local. And if you force the install to /usr/local, a regular python won't be able to find it by default, you'd have to alter the sys.path. An analogy would be if someone wanted to build a valhalla add-on, but strictly installs it to /usr instead of letting the already (wherever) installed valhalla decide where to put the plugin so it can even find it. Plus the really nice simplification:)

@kevinkreiser
Copy link
Member

i still think /usr/local is quite standard:

Python 3.6.9 (default, Jan 26 2021, 15:33:00) 
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
Could not open PYTHONSTARTUP
FileNotFoundError: [Errno 2] No such file or directory: '/home/kkreiser/.pythonrc'
>>> import sys
>>> print(sys.path)
['', '/usr/lib/python36.zip', '/usr/lib/python3.6', '/usr/lib/python3.6/lib-dynload', '/usr/local/lib/python3.6/dist-packages', '/usr/lib/python3/dist-packages']

but as i mentioned im totally cool with tackling that later

@nilsnolde
Copy link
Member Author

Huh, you're right, thought I checked it on ubuntu. Maybe I did it on my arch, where /usr/local is not in the sys.path (or in the default linker paths, guess they go together).. I have less strong feelings about it in that case:)

@nilsnolde nilsnolde merged commit 91a8b02 into master Mar 11, 2021
@nilsnolde nilsnolde deleted the nn_fix_more_python branch March 11, 2021 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken Python Installation
2 participants