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

Upload only linux_x86_64 wheels instead of manylinux2014 #777

Merged

Conversation

diegoferigo
Copy link
Member

Closes #776

@diegoferigo diegoferigo self-assigned this Nov 21, 2020
@diegoferigo diegoferigo linked an issue Nov 21, 2020 that may be closed by this pull request
@diegoferigo
Copy link
Member Author

Let me try the wheel before merging, I let you know if it is ok.

@traversaro
Copy link
Member

Ok!

@diegoferigo
Copy link
Member Author

diegoferigo commented Nov 21, 2020

In #776 (comment) I wrote that is auditwheel that fixes the symbols to match the ABI. I was wrong. I think that _GLIBCXX_USE_CXX11_ABI is automatically set to iDynTree transitively from its dependencies found in the CentOS system.

In any case, using auditwheel to vendor the external dependencies would in any case result with a wheel with libraries with the old ABI, taken from CentOS. I think that there is no escape here.

Let's revert to what we used to do until now in other projects: building on an ubuntu system and upload the wheel as it is, just performing the renaming workaround. We loose the vendoring of the dependencies, but this is the price we have to pay. I think we learnt a lot in these days and faced many of the challenges related to a PEP compliant PyPI packaging. The next standard has been finalized in PEP600 (other infos here), I guess we have to wait its adoption by the build tools and the update of all the tooling (pypa/manylinux#542). Alternatively, conda is already available.

Here below I leave the reference code I used to build the old wheels with cibuildwheel. It can be useful when we will provide wheels for Windows and macOS.

cibuildwheel snippet
  # The following can be adapted to build wheels on macOS and Windows
  build_wheels:
    name: Build wheels on ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      fail-fast: false
      matrix:
        python-version:
          - 3.8
        os:
          # - macos-latest
          # - windows-latest

    steps:

      - uses: actions/checkout@master
      - run: git fetch --prune --unshallow

      - name: Set up Python
        uses: actions/setup-python@v2
        with:
          python-version: ${{ matrix.python-version }}

      - name: Install cibuildwheel
        run: pip install cibuildwheel

      - name: Build wheels
        run: python -m cibuildwheel --output-dir wheelhouse
        env:
          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"
          CIBW_TEST_COMMAND: "python -c 'import idyntree.bindings'"
          # Note: here we repair the wheel maintaining the linux_x86_64 platform
          CIBW_REPAIR_WHEEL_COMMAND_LINUX: "auditwheel repair --plat linux_x86_64 -w {dest_dir} {wheel}"

      - uses: actions/upload-artifact@v2
        if: github.repository == 'robotology/idyntree'
        with:
          path: ./wheelhouse/*.whl

@traversaro
Copy link
Member

Ok for me. Can you make a PR to refer to the old case? I think the automatic publishing on PyPI is still great, even if just in source form.

@diegoferigo
Copy link
Member Author

You anticipated me, when you commented I was editing the comment above with the old code. Is it fine?

@diegoferigo diegoferigo force-pushed the fix/upload_only_linux_x86_64_wheels branch from b4c816d to 4084859 Compare November 21, 2020 14:39
@diegoferigo
Copy link
Member Author

Good to go as soon as CI turns green. I tested the wheels bu downloading the artifact and they work well 🎉

@traversaro
Copy link
Member

Just to clarify for iDynTree on PyPI this is still a change, right?

Before we were posting on PyPI source archives, so anyone doing pip install --pre idyntree would download a source archive, and would compile it on his system, and the assumption was that it had the right dependencies to compile them (so on recent debian systems libassimp-dev libxml2-dev libeigen3-dev coinor-libipopt-dev . Now instead we compile and make available the Linux binaries compiled on Ubuntu 20.04, and so doing pip install --pre idyntree will download this linux binaries, and the assumption is that the user already has installed libxml2 libassimp5 coinor-libipopt1v5 with the exact ABI that have on Ubuntu 20.04, so we assume that the user is on Ubuntu 20.04, right?

@traversaro
Copy link
Member

To clarify, I am totally ok with this change, I just want to make sure that I got correctly change.

@diegoferigo
Copy link
Member Author

Just to clarify for iDynTree on PyPI this is still a change, right?

There is no real change, what you wrote here (with small modifications) is correct:

Before we were posting manually on PyPI source archives, so anyone doing pip install --pre idyntree would download a source archive, and would compile it on his system, and the assumption was that it had the right dependencies to compile them (so on recent debian systems libassimp-dev libxml2-dev libeigen3-dev coinor-libipopt-dev . Now instead we also compile and make available the Linux binaries compiled on Ubuntu 20.04, and so doing pip install --pre idyntree will download these linux binaries, and the assumption is that the user already has installed libxml2 libassimp5 coinor-libipopt1v5 with the exact ABI that have on Ubuntu 20.04, so we assume that the user is on Ubuntu 20.04, right?

My point is that we upload both the sdist and the linux_x86_64 wheel. By default, if both are available for a given platform, pip will prefer downloading the wheel. However, using pip install idyntree --no-binary :all:, the wheel can be ignored.

For the records, this is the expected ABI, that could be available also on other distributions, not specifically only for Ubuntu 20.04:

❯ ldd /rl/venv/lib/python3.8/site-packages/idyntree/_iDynTree.so 
        linux-vdso.so.1 (0x00007ffd8650b000)
        libassimp.so.5 => /lib/x86_64-linux-gnu/libassimp.so.5 (0x00007fb9307a3000)
        libxml2.so.2 => /lib/x86_64-linux-gnu/libxml2.so.2 (0x00007fb9305e9000)
        libipopt.so.1 => /lib/libipopt.so.1 (0x00007fb93037c000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fb93022d000)
        libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fb93004c000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fb93002f000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fb92fe3d000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fb92fe21000)
        libminizip.so.1 => /lib/x86_64-linux-gnu/libminizip.so.1 (0x00007fb92fc16000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fb92fc10000)
        libicuuc.so.66 => /lib/x86_64-linux-gnu/libicuuc.so.66 (0x00007fb92fa2a000)
        liblzma.so.5 => /lib/x86_64-linux-gnu/liblzma.so.5 (0x00007fb92f9ff000)
        liblapack.so.3 => /lib/x86_64-linux-gnu/liblapack.so.3 (0x00007fb92f35b000)
        libblas.so.3 => /lib/x86_64-linux-gnu/libblas.so.3 (0x00007fb92f2ee000)
        libdmumps_seq-5.2.1.so => /lib/x86_64-linux-gnu/libdmumps_seq-5.2.1.so (0x00007fb92f0df000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fb93196d000)
        libicudata.so.66 => /lib/x86_64-linux-gnu/libicudata.so.66 (0x00007fb92d61e000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fb92d5f9000)
        libgfortran.so.5 => /lib/x86_64-linux-gnu/libgfortran.so.5 (0x00007fb92d331000)
        libmumps_common_seq-5.2.1.so => /lib/x86_64-linux-gnu/libmumps_common_seq-5.2.1.so (0x00007fb92d2d9000)
        libquadmath.so.0 => /lib/x86_64-linux-gnu/libquadmath.so.0 (0x00007fb92d28f000)
        libpord_seq-5.2.1.so => /lib/x86_64-linux-gnu/libpord_seq-5.2.1.so (0x00007fb92d276000)
        libesmumps-6.so => /lib/x86_64-linux-gnu/libesmumps-6.so (0x00007fb92d26e000)
        libscotch-6.so => /lib/x86_64-linux-gnu/libscotch-6.so (0x00007fb92d1d9000)
        libscotcherr-6.so => /lib/x86_64-linux-gnu/libscotcherr-6.so (0x00007fb92d1d4000)
        libbz2.so.1.0 => /lib/x86_64-linux-gnu/libbz2.so.1.0 (0x00007fb92d1c1000)

@diegoferigo
Copy link
Member Author

I updated my previous #777 (comment) with new links and resources if anyone is interested in the upcoming manylinux_${GLIBCMAJOR}_${GLIBCMINOR}_${ARCH} platform.

@traversaro
Copy link
Member

Ok thanks!

@traversaro traversaro merged commit 32ee6f6 into robotology:devel Nov 22, 2020
@diegoferigo diegoferigo deleted the fix/upload_only_linux_x86_64_wheels branch December 6, 2020 13:17
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.

Wheels are not ABI compatible with C++11
2 participants