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

Build modernization (GHA, wheels, setuptools) #407

Merged
merged 48 commits into from Dec 12, 2020

Conversation

@bsolomon1124
Copy link
Contributor

@bsolomon1124 bsolomon1124 commented May 23, 2020

Closes:

This is meant to be run inside a manylinux Docker container.

It produces sets of wheels in wheelhouse/ dir.

Ref:
- https://github.com/pypa/manylinux
- https://github.com/pypa/python-manylinux-demo
@bsolomon1124 bsolomon1124 mentioned this pull request May 23, 2020
bsolomon1124 added 12 commits May 23, 2020
Allows for better control over version
and build output.
- Single-source the libyaml upstream version used in tests and
  build
- Redo the .travis.yml matrix for conditional logic
- Put the libyaml-build into its own script since it's used in
  multiple places
sudo is actually not available on some containers,
but we can do an initial UID check.
@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 24, 2020

For macOS wheels, I'm currently running into a documented issue with delocate that prevents bundling of libyaml into the wheel because PyYAML uses a 'top-level' _yaml extension module rather than a package structure.

@nsoranzo
Copy link

@nsoranzo nsoranzo commented May 24, 2020

Thanks for starting this work! I'd absolutely recommend cibuildwheel, saves you a lot of reinventing the wheel!

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 24, 2020

@nsoranzo thanks, though I've already reinvented most of the wheel at this point with manylinux and pure-Python wheels succeeding. I'm not sure if cibuildwheel would alleviate the delocate issue unless it patches it in some way.

To clarify on the delocate issue, delocate-wheel doesn't like that the _yaml extension isn't part of a Python package, and so simply ignores it and fails to bundle in libyaml.

It looks like there is an open PR to fix this (matthew-brett/delocate#39).

@nitzmahone
Copy link
Member

@nitzmahone nitzmahone commented May 27, 2020

Looking good so far at a glance, but a couple things:

  • I think you're still going against a dynamically-linked libyaml, which is probably bad. For Linux wheels, check with readelf -d (path to _yamlXXX.so)- if you see any (NEEDED) entries to anything but libc, it's still trying to link dynamically. For Mac ... I have no idea.
  • If it's easier to move the extension to a yaml subpackage, go for it- nothing but pyyaml itself should be messing with that

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 27, 2020

If it's easier to move the extension to a yaml subpackage, go for it- nothing but pyyaml itself should be messing with that

Are you sure there's not a concern for messing with any code out there that's doing import _yaml? I know that's absolutely not part of the public API, but since this package is such a widely used dependency, I would guess that re-namespacing to yaml._yaml would break more than a few things. The other devious option would be to make _yaml the package name itself and import C-extension stuff into its __init__.

@nitzmahone
Copy link
Member

@nitzmahone nitzmahone commented May 27, 2020

I don't personally have any issue with re-namespacing it- I've always hated the fact that the extension was in a different namespace instead of contained beneath the main package. It's pretty clear from the package name that it shouldn't be consumed externally, and if, in the worst case, it causes a lot of problems, we can always backpedal and create a stub package for _yaml (which I think is what you're saying as well). But I wouldn't do that until the pitchfork-wielding masses arrive. ;) Depending on how the efforts to package the wheels works out, we might want to do it as a 6.0 anyway, just to give folks an easy upper-bound to pin to if that causes problems.

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 27, 2020

Done, via

grep --null -lr " _yaml" tests lib lib3 | \
    xargs -0 -L1  gsed -i 's/ _yaml/ yaml._yaml/g'

Looks like there is now an issue Cythonizing (can't find yaml_parser_t from yaml.h, among other things), so I probably neglected to change something. Will check back later.

@nitzmahone
Copy link
Member

@nitzmahone nitzmahone commented May 27, 2020

Did some Googling for import _yaml- there are definitely folks doing it to do things like check for the presence of the C ext, check its version, and play with its parser internals (for reasons I don't understand at a glance). I'm still not sure we shouldn't just break it anyway, but if not, I'd still personally prefer a stub compatibility package to having the extension itself live in the site-packages root.

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 27, 2020

Not exactly sure what the issue is here because I'm able to cythonize -i ext/_yaml.pyx just fine. Maybe some of the various Extension stuff happening in setup.py.

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 27, 2020

Still need to re-work --with-libyaml back into setup.py, but slimmed that file down for now to make things simpler for the moment.

@bsolomon1124
Copy link
Contributor Author

@bsolomon1124 bsolomon1124 commented May 28, 2020

A few considerations for compatibility (they're generally positive things):

The front page of the PyYAML docs give the example python setup.py --with-libyaml install. That's a strong enough reason to retain --with-libyaml and --without-libyaml when installing this way.

pip also advertises its --install-option to pass arguments to setup.py. For example, you might expect this to work:

$ pip install --install-option='--with-libyaml' PyYAML
$ pip install --install-option='--without-libyaml' PyYAML

The blessing in disguise is that the above two commands don't work using the PyPI 5.3.1 PyYAML. I'm not exactly sure why, but probably has to do with the various hacks currently used in setup.py and distutils.

Instead, the straightforward behavior when installing from pip is to use the libyaml bindings if libyaml is found, or otherwise skip them. (setup.py goes out of its way to check for this.) You can see this easily this by running:

pip install --quiet --disable-pip-version-check PyYAML
python -c 'import pyyaml; print(pyyaml.__with_libyaml__)

...first in a python:3-slim Docker container and then in a python:3-stretch, where the results will be False and True, respectively.

So, besides the fact that this PR puts _yaml into yaml._yaml (and that can possibly be monkey-patched still), the only "incompatibility" I see (and it's a huge stretch to call it that), is:

  • Previously: user on Ubuntu box calls pip install PyYAML without libyaml present. This will install pyyaml without libyaml bindings
  • Now: if manylinux wheels are distributed and the same user goes to install, they will implicitly use the libyaml bindings because libyaml is shipped with the wheel. If they really want to disable this, they should use pip's --no-binary flag.

@nitzmahone nitzmahone mentioned this pull request Jan 14, 2021
asherf added a commit to asherf/pants that referenced this issue Jan 21, 2021
5.4.1 (2021-01-20)

* yaml/pyyaml#480 -- Fix stub compat with older pyyaml versions that may unwittingly load it

5.4 (2021-01-19)

* yaml/pyyaml#407 -- Build modernization, remove distutils, fix metadata, build wheels, CI to GHA
* yaml/pyyaml#472 -- Fix for CVE-2020-14343, moves arbitrary python tags to UnsafeLoader
* yaml/pyyaml#441 -- Fix memory leak in implicit resolver setup
* yaml/pyyaml#392 -- Fix py2 copy support for timezone objects
* yaml/pyyaml#378 -- Fix compatibility with Jython

https://github.com/yaml/pyyaml/blob/master/CHANGES
stuhood pushed a commit to pantsbuild/pants that referenced this issue Jan 21, 2021
5.4.1 (2021-01-20)

* yaml/pyyaml#480 -- Fix stub compat with older pyyaml versions that may unwittingly load it

5.4 (2021-01-19)

* yaml/pyyaml#407 -- Build modernization, remove distutils, fix metadata, build wheels, CI to GHA
* yaml/pyyaml#472 -- Fix for CVE-2020-14343, moves arbitrary python tags to UnsafeLoader
* yaml/pyyaml#441 -- Fix memory leak in implicit resolver setup
* yaml/pyyaml#392 -- Fix py2 copy support for timezone objects
* yaml/pyyaml#378 -- Fix compatibility with Jython

https://github.com/yaml/pyyaml/blob/master/CHANGES
amykyta3 added a commit to amykyta3/pyyaml that referenced this issue Mar 20, 2021
PR yaml#407 also drops support for Python 3.5
otc-zuul bot pushed a commit to opentelekomcloud-infra/customer-service-monitoring that referenced this issue Mar 29, 2021
Bump pyyaml from 5.3.1 to 5.4 in /roles/prepare_build/files

Bumps pyyaml from 5.3.1 to 5.4.

Changelog
Sourced from pyyaml's changelog.

5.4 (2021-01-19)

yaml/pyyaml#407 -- Build modernization, remove distutils, fix metadata, build wheels, CI to GHA
yaml/pyyaml#472 -- Fix for CVE-2020-14343, moves arbitrary python tags to UnsafeLoader
yaml/pyyaml#441 -- Fix memory leak in implicit resolver setup
yaml/pyyaml#392 -- Fix py2 copy support for timezone objects
yaml/pyyaml#378 -- Fix compatibility with Jython




Commits

58d0cb7 5.4 release
a60f7a1 Fix compatibility with Jython
ee98abd Run CI on PR base branch changes
ddf2033 constructor.timezone: _copy & deepcopy
fc914d5 Avoid repeatedly appending to yaml_implicit_resolvers
a001f27 Fix for CVE-2020-14343
fe15062 Add 3.9 to appveyor file for completeness sake
1e1c7fb Add a newline character to end of pyproject.toml
0b6b7d6 Start sentences and phrases for capital letters
c976915 Shell code improvements
Additional commits viewable in compare view




Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

@dependabot rebase will rebase this PR
@dependabot recreate will recreate this PR, overwriting any edits that have been made to it
@dependabot merge will merge this PR after your CI passes on it
@dependabot squash and merge will squash and merge this PR after your CI passes on it
@dependabot cancel merge will cancel a previously requested merge and block automerging
@dependabot reopen will reopen this PR if it is closed
@dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
@dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
@dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
@dependabot use these labels will set the current labels as the default for future PRs for this repo and language
@dependabot use these reviewers will set the current reviewers as the default for future PRs for this repo and language
@dependabot use these assignees will set the current assignees as the default for future PRs for this repo and language
@dependabot use this milestone will set the current milestone as the default for future PRs for this repo and language

You can disable automated security fix PRs for this repo from the Security Alerts page.

Reviewed-by: None <None>
Reviewed-by: Anton Kachurin <katchuring@gmail.com>
Reviewed-by: Anton Sidelnikov <None>
amykyta3 added a commit to amykyta3/pyyaml that referenced this issue Apr 29, 2021
PR yaml#407 also drops support for Python 3.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants