ktbarrett / find_libpython

Finds the libpython associated with the current Python environment, wherever it may be hiding.
MIT License
14 stars 7 forks source link

Look up libpython from /proc/<pid>/maps file of running process #44

Closed awmoore-intel closed 8 months ago

awmoore-intel commented 8 months ago

The original find_libpython did not find our libpython. This looks up the current process and looks for a library continaing libpython

ktbarrett commented 8 months ago

I need to get CI working again, but I'm fine with the approach. However, this is not going to work on Windows, so you are going to need to add OS support guards.

EDIT: Maybe a try-except around opening the /proc file would suffice.

awmoore-intel commented 8 months ago

I need to get CI working again, but I'm fine with the approach. However, this is not going to work on Windows, so you are going to need to add OS support guards.

EDIT: Maybe a try-except around opening the /proc file would suffice.

sounds good. updated. thanks

ktbarrett commented 8 months ago

Reopening to re-run CI.

ktbarrett commented 8 months ago

Is it possible to get a test for this case? Or is this really just an issue with your in-house Python build?

awmoore-intel commented 8 months ago

For some reason, the _linked_libpython_unix() is returning the process binary, not the library.

        path_linked = _linked_libpython_unix(libpython)
        # Get library from /proc method
        path_from_proc = list(_get_proc_library())[0]

>       assert path_linked == path_from_proc
E       AssertionError: assert '/usr/bin/python3.10' == '/usr/lib/x86...on3.10.so.1.0'
ktbarrett commented 8 months ago

For some reason, the _linked_libpython_unix() is returning the process binary, not the library.

This is typical. Most distros use static linking and distribute libpython as a separate package. I would not assume this test to pass.

ktbarrett commented 8 months ago

What is CDLL("") and where is it documented?

awmoore-intel commented 8 months ago

What is CDLL("") and where is it documented?

I could not find it, but I've used it before. It uses dlopen and in those man pages it says:

If filename is NULL, then the returned handle is for the main
       program.  If filename contains a slash ("/"), then it is
       interpreted as a (relative or absolute) pathname.  Otherwise, the
       dynamic linker searches for the object as follows (see [ld.so(8)](https://man7.org/linux/man-pages/man8/ld.so.8.html)
       for further details):
ktbarrett commented 8 months ago

As soon as we get this in I will release 0.4 with some of the other improvements I did today.

codecov-commenter commented 8 months ago

Codecov Report

Attention: Patch coverage is 67.74194% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 63.34%. Comparing base (4dafdb1) to head (184cc2e).

Files Patch % Lines
src/find_libpython/__init__.py 68.75% 2 Missing and 3 partials :warning:
tests/test_find_libpython.py 66.66% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #44 +/- ## ========================================== + Coverage 55.24% 63.34% +8.09% ========================================== Files 3 4 +1 Lines 181 221 +40 Branches 48 53 +5 ========================================== + Hits 100 140 +40 + Misses 61 60 -1 - Partials 20 21 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ktbarrett commented 8 months ago

There are rebase conflicts apparently?

awmoore-intel commented 8 months ago

There are rebase conflicts apparently?

I don't know. I don't see any message about rebasing, but I think I did rebase when I merged in some of your changes.

awmoore-intel commented 8 months ago

There are rebase conflicts apparently?

I don't know. I don't see any message about rebasing, but I think I did rebase when I merged in some of your changes.

I'm unable to merge in case you're waiting for that. It says I don't have write access.

ktbarrett commented 8 months ago

A couple recommendations.

  1. Don't use master, even in a fork, as a development branch.
  2. This repo uses rebase merges, so avoid making merge commits and instead rebase.

Rebase flows are in general far easier to reason about. Especially for this repo where I only do rebase/squash merges. Rebasing with merge commits is a nightmare, which is why it's not currently letting me merge due to rebase conflicts. I'll fix it this time.

ktbarrett commented 8 months ago

Well, we don't have a single environment in CI that touches this code, so I hope it works for you, because we won't be able to verify it going forward. I'll merge this, but I recommend getting whomever is in charge of building your Python installations to make changes so it behaves more like a typical installation and hits some other case.

awmoore-intel commented 8 months ago

A couple recommendations.

  1. Don't use master, even in a fork, as a development branch.
  2. This repo uses rebase merges, so avoid making merge commits and instead rebase.

Rebase flows are in general far easier to reason about. Especially for this repo where I only do rebase/squash merges. Rebasing with merge commits is a nightmare, which is why it's not currently letting me merge due to rebase conflicts. I'll fix it this time.

Noted. thanks

awmoore-intel commented 8 months ago

Well, we don't have a single environment in CI that touches this code, so I hope it works for you, because we won't be able to verify it going forward. I'll merge this, but I recommend getting whomever is in charge of building your Python installations to make changes so it behaves more like a typical installation and hits some other case.

Yes, our project is using an embedded python, but will be relying on an external python in the future - which I assume will work better here.

ktbarrett commented 8 months ago

And since you are probably watching this thread. v0.4.0 has been released!