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

Automatically publish wheels to PyPI #774

Merged
merged 7 commits into from
Nov 20, 2020

Conversation

diegoferigo
Copy link
Member

In the series of #726 #733 #735, the SWIG Python bindings have been improved, an initial packaging support was introduced, and the CI started testing the source distribution (sdist) packaging and installation.

What's still missing is the creation and automatic upload (continuous delivery) of the binary version of the package, called wheel in Python jargon.

This PR fills the gap introducing the following changes:

  • Replace the code that builds the CMake project from setup.py with the recently open-sourced diegoferigo/cmake-build-extension.
  • Update the Python workflow:
    • Standalone jobs to 1) create sdist, 2) create wheel, 3) publish packages to PyPI (Python 3.8 for the moment)
    • Use joerick/cibuildwheel to build a real PEP599 manylinux2014 wheel on a CentOS docker container
  • Update CMake to prevent linking against libpython (the target Python3::Python is not strictly required, my fault)

It's long long time I want to finalize a similar setup, but every time I try on other projects, I always get stuck on the CentOS situation. In fact, all the dependencies must be installed in this distribution, and often this is not trivial. In the iDynTree case, instead, all the dependencies are available in the repositories and can be installed with yum. The real turning point to get this done is the discovery of cibuildwheel, amazing. I wouldn't have tried manually without, it's too much a pain.

What's really nice of the process, is that it calls automatically pypa/auditwheel on the resulting wheel, "repairing" the shared libraries. It means that it takes all the shared libraries in the CentOS system that link against the packaged libraries, and include them in the wheel, fixing the RPATH. The result is similar to static linking, but keeping the shared type.

All of this means that this manylinux2014 wheel does not need any runtime system dependency!. The usual dependencies require a manual installation only when building the package from either the sdist or the repository's setup.py.

The package auditwheel also support macOS and Windows. However, they are more tricky. I think that macOS has something similar to auditwheel, but if I understood not Windows. I'll leave these two platform out for the moment.

Final remark: it does not include, as the previous, the bindings based on pybind #747

@diegoferigo diegoferigo self-assigned this Nov 20, 2020
@traversaro
Copy link
Member

Use joerick/cibuildwheel to build a real PEP599 manylinux2014 wheel on a CentOS docker container

Wow!


build_wheels:
name: Build wheels on ${{ matrix.os }}
runs-on: ubuntu-latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it make sense to be explicit if we are using ubuntu-18.04 or ubuntu-20.04 . ubuntu-latest could make sense in a CI job, but for a deploy job it probably make sense to try to have explicit control on the version used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not matter much since cibuildwheel performs all the operations in a CentOS container. It could be also Ubuntu Hardy if you want (maybe I got a too old one xD).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I meant that cibuildwheel itself could have a different behavior on Ubuntu 18.04 or 20.04, but if you think that is not a problem let's keep it like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's installed through pip and non the package manager, I confirm it should be ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm giving a last pass to the changes, and I realized that in any case with different distros there no difference also because we configure a particular python version (3.8 here on bionic). cibuildwheel is then installed with that interpreter regardless of the distribution.

- uses: pypa/gh-action-pypi-publish@master
with:
user: __token__
password: ${{ secrets.PYPI_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to to something for adding this Secret?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ow yes, I forgot to write it above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can I generate it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to log in in PyPI with your account, go to the settings, create a token. I recommend filtering the access to only the idyntree package.

CIBW_BUILD_VERBOSITY: 1
CIBW_BUILD: cp38-manylinux_x86_64
CIBW_MANYLINUX_X86_64_IMAGE: manylinux2014
CIBW_BEFORE_BUILD: "yum install -y eigen3-devel assimp-devel libxml2-devel coin-or-Ipopt-devel swig3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the version of eigen3 installed in this CentOS image? Old Eigen version have some quite subtle bugs, and for the future we may want to use some functionalities of newer Eigen versions. I don't think it is a problem to achieve this (we can easily add an option to use an internal Eigen via FetchContent, and enable it when building the wheels) but it is better to be aware of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it seems to be 3.3.7.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you get this info?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the logs :)

@diegoferigo diegoferigo force-pushed the feature/wheels branch 3 times, most recently from 7776920 to f47b2af Compare November 20, 2020 10:27
@diegoferigo
Copy link
Member Author

I think I managed to fix CI with b72e735 🎉


Something I want to point out: release schedule. As done for gym-ignition, pre-releases are uploaded after any commit land into devel. Instead, stable releases are uploaded only when GitHub releases are published. This means that hot-fix commits in master without an associated release are not included in any published package.

I think this is a reasonable default, and it is the only pattern I found that allows us to maintain only a single idyntree package and not a second idyntree_nightly.

@traversaro
Copy link
Member

Something I want to point out: release schedule. As done for gym-ignition, pre-releases are uploaded after any commit land into devel. Instead, stable releases are uploaded only when GitHub releases are published. This means that hot-fix commits in master without an associated release are not included in any published package.

I think this is perfectly fine.

@diegoferigo
Copy link
Member Author

diegoferigo commented Nov 20, 2020

Ok! Ready to merge as soon as CI turns green. Did you by chance add the token to the repo?

@traversaro
Copy link
Member

Ok! Ready to merge as soon as CI turns green. Did you by chance add the token to the repo?

Done!

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