-
Notifications
You must be signed in to change notification settings - Fork 187
Use find_libpython.py in deps/build.jl #556
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
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 ifs.
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_libraryused infind_libpython.pyis probably good enough. What do you think?(Edit: the fallback should stay: https://travis-ci.org/JuliaPy/PyCall.jl/builds/425108395)