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

Move _pygit2 to pygit2._pygit2 #978

Merged
merged 2 commits into from
Feb 21, 2020

Conversation

rcoup
Copy link
Contributor

@rcoup rcoup commented Feb 20, 2020

This resolve issues with binary top-level modules affecting delocate on macOS (which helps with macOS wheel creation). Specifically, matthew-brett/delocate#12 (comment)

Change is that it moves (eg) site-packages/_pygit2.cpython-37m-darwin.so to site-packages/pygit2/_pygit2.cpython-37m-darwin.so, meaning the _pygit2 internal api is available via import pygit2._pygit2 cf import _pygit2

The impact this has building macOS wheels:
(fyi delocate-listdeps shows the libraries and what depends on them indented)

Before:

$ delocate-listdeps -d dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
/Users/rcoup/code/pygit2/venv/lib/libgit2.0.99.0.dylib:
    _pygit2.cpython-37m-darwin.so
    pygit2/_libgit2.abi3.so
$ delocate-wheel dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
$ delocate-listdeps -d dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
/Users/rcoup/code/pygit2/venv/lib/libgit2.0.99.0.dylib:
    _pygit2.cpython-37m-darwin.so
@loader_path/.dylibs/libgit2.0.99.0.dylib:
    pygit2/_libgit2.abi3.so
$ zipinfo -1 dist/*.whl | grep -P "\.dylib|\.so"
_pygit2.cpython-37m-darwin.so
pygit2/_libgit2.abi3.so
pygit2/.dylibs/libgit2.0.99.0.dylib
  • libgit2 gets bundled into the wheel but _pygit2.*.so is still referencing the $LIBGIT2 path (everything should be prefixed @loader_path/.dylibs/)
  • if you actually install this wheel with $LIBGIT2/lib/libgit2.dylib in position too (eg via: import pygit2; pygit2.config.Config()), then you get a segfault
  • cause of that seems to be that we end up loading libgit2 twice, once the copy baked into the wheel, and once from the original $LIBGIT2/lib location.
$ python
>>> import pygit2
>>> import os; os.getpid()

$ lsof -p 21632 | grep -P "libgit2\..*\.dylib"
Python  21632 rcoup  txt    REG    1,4   1004300          8737655297 /Users/rcoup/code/pygit2/venv/lib/libgit2.0.99.0.dylib
Python  21632 rcoup  txt    REG    1,4   1004300          8737685087 /Users/rcoup/code/pygit2/venv/lib/python3.7/site-packages/pygit2/.dylibs/libgit2.0.99.0.dylib

By moving the _pygit2 module into the top-level python package, delocate correctly links it to the baked libgit2.

After:

$ delocate-listdeps -d dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
/Users/rcoup/code/pygit2/venv/lib/libgit2.0.99.0.dylib:
    pygit2/_pygit2.cpython-37m-darwin.so
    pygit2/_libgit2.abi3.so
$ delocate-wheel dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
$ delocate-listdeps -d dist/pygit2-1.0.3-cp37-cp37m-macosx_10_15_x86_64.whl
@loader_path/.dylibs/libgit2.0.99.0.dylib:
    pygit2/_pygit2.cpython-37m-darwin.so
    pygit2/_libgit2.abi3.so
$ zipinfo -1 dist/*.whl | grep -P "\.dylib|\.so"
pygit2/_libgit2.abi3.so
pygit2/_pygit2.cpython-37m-darwin.so
pygit2/.dylibs/libgit2.0.99.0.dylib

Delocate could fix the issue, but the comments in that ticket imply there are additional concerns there, and I can't see any particular need for the internal _pygit2 to be a top-level package.

This resolve issues with binary top-level modules affecting delocate on macOS
@rcoup
Copy link
Contributor Author

rcoup commented Feb 20, 2020

Looks like windows builds need some setup.py tweaking to match

@jdavid
Copy link
Member

jdavid commented Feb 21, 2020

LGTM, but AppVeyor must be green to merge.

@jdavid jdavid merged commit eb4984e into libgit2:master Feb 21, 2020
@rcoup rcoup deleted the relocate-internal-api branch February 21, 2020 09:52
netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Mar 1, 2020
1.1.0 (2020-03-01)
-------------------------

- Upgrade to libgit2 0.99
  `#959 <https://github.com/libgit2/pygit2/pull/959>`_

- Continued work on custom odb backends
  `#948 <https://github.com/libgit2/pygit2/pull/948>`_

- New ``Diff.patchid`` getter
  `#960 <https://github.com/libgit2/pygit2/pull/960>`_
  `#877 <https://github.com/libgit2/pygit2/issues/877>`_

- New ``settings.disable_pack_keep_file_checks(...)``
  `#908 <https://github.com/libgit2/pygit2/pull/908>`_

- New ``GIT_DIFF_`` and ``GIT_DELTA_`` constants
  `#738 <https://github.com/libgit2/pygit2/issues/738>`_

- Fix crash in iteration of config entries
  `#970 <https://github.com/libgit2/pygit2/issues/970>`_

- Travis: fix printing features when building Linux wheels
  `#977 <https://github.com/libgit2/pygit2/pull/977>`_

- Move ``_pygit2`` to ``pygit2._pygit2``
  `#978 <https://github.com/libgit2/pygit2/pull/978>`_

Requirements changes:

- Now libgit2 0.99 is required
- New requirement: cached-property

Breaking changes:

- In the rare case you're directly importing the low level ``_pygit2``, the
  import has changed::

    # Before
    import _pygit2

    # Now
    from pygit2 import _pygit2
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.

2 participants