-
Notifications
You must be signed in to change notification settings - Fork 189
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 find_libpython.py in deps/build.jl #556
Conversation
deps/build.jl
Outdated
else | ||
pipe = process = open(cmd) | ||
end | ||
libpy_name = read(pipe.out, String) |
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.
ATM find_libpython.py
is run in the default "print the best one" mode. I can also run it with --list-all
option, loop over all the candidate paths and use the first one that Libdl.dlopen
succeeds. What do you think?
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.
Best to be defensive here and try multiple paths, I suppose
I find it very frustrating that the code to find libpython from |
Yeah, I can't agree enough... The upside is that we wrote (or about to finish writing) a portable script so that other people can just grab it if they need it. |
278130f
to
8950f6c
Compare
Locate libpython associated with this Python executable. | ||
""" | ||
|
||
# License |
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.
I inlined the license so that it's easier to grab this file and put it in other projects.
|
||
# LIBPL seems to be the right config_var to use. It is the one | ||
# used in python-config when shared library is not enabled: | ||
# https://github.com/python/cpython/blob/v3.7.0/Misc/python-config.in#L55-L57 |
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.
So it seems adding LIBPL
was the key (it was not in the PyJulia version; I'll back-port it later). This script finds libpython in case the old julia function couldn't: #565 (comment)
I think it's good to go. CI on appveyor is failing even on master at: Lines 327 to 330 in 0f685cf
which is irrelevant to this PR (the build is already succeeded at this stage). |
It would be great to get this one merged since it fixes #565 which currently causes many failures on CIBot. |
Backported to JuliaPy/pyjulia#203 and the tests pass there too. |
lib_basenames = list(candidate_names(suffix=suffix)) | ||
|
||
for directory in lib_dirs: | ||
for basename in lib_basenames: |
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.
Might want to guard this with if directory:
in case get_config_var
returned None
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.
That's done with append_truthy
utility function defined above. If my usage of utility function is confusing I can remove it and add explicit if
s.
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.
No, I see.
I still wish we had a simpler solution, but I will merge this since it seems like it handles cases that our current code doesn't. |
as I suggested in JuliaPy/pyjulia#190 (comment)
I think we can also remove this part (untouched ATM):PyCall.jl/deps/build.jl
Lines 95 to 134 in 49029e0
ctypes.util.find_library
used infind_libpython.py
is probably good enough. What do you think?(Edit: the fallback should stay: https://travis-ci.org/JuliaPy/PyCall.jl/builds/425108395)