mctools / ncrystal

NCrystal : a library for thermal neutron transport in crystals and other materials
https://mctools.github.io/ncrystal/
Other
38 stars 17 forks source link

manylinux_2_28 pypi pkgs missing #166

Closed tkittel closed 4 months ago

tkittel commented 4 months ago

@MilanKlausz, here is a hopefully easy one for you :-)

I think we need to add manylinux_2_28 to: https://pypi.org/project/ncrystal/#files

Background info: I noticed the hard way that we do not provide any manylinux_2_28 pypi packages. This matters when using the pypi package to compile downstream C++ code - at least when any of the interface functions using std::string somewhere in the function signature. This problem is related to the dual-symbols introduced around the time of gcc5 (cf. _GLIBCXX_USE_CXX11_ABI) for std::string and std::list, and gcc5 is from 2015. So indeed, we are still haunted by the c++11 migration, here 13 years later! :-)

We should probably still provide the existing manylinux packages still, since ubuntu 20 apparently does not work with manylinux_2_28.

tkittel commented 4 months ago

Btw., I am happy to provide a 3.8.1 release of NCrystal once this is in place, in case it is not easy to simply trigger the addition of these new wheels.

tkittel commented 4 months ago

Btw., this command is useful to see which wheels a particular pip installation will prefer:

 python -m pip debug --verbose

It failed for me until I updated pip.

tkittel commented 4 months ago

I guess the relevant docs are https://cibuildwheel.readthedocs.io/en/stable/options/#linux-image ?

If I understand correctly, this is as simple as changing CIBW_MANYLINUX_X86_64_IMAGE and CIBW_MANYLINUX_I686_IMAGE to be manylinux_2_28. However, reading https://github.com/pypa/cibuildwheel/issues/1153 it seems that if one wants to also still have the manylinux2014 images (which probably makes sense?), we would have to run the CI job twice? I.e. have two actual workflows in the ncrystal repo, one which builds everything like now (with perhaps manylinux_2_28 as the manylinux choice), and one which only builds manylinux2014 (and no macos stuff etc.).

Do you agree?

MilanKlausz commented 4 months ago

Indeed, based on the linked documentation, we could set CIBW_MANYLINUX_X86_64_IMAGE to manylinux_2_28, but it is also stated there that "_manylinux_228 is not supported for i686 architecture.", so it probably wouldn't work for CIBW_MANYLINUX_I686_IMAGE.

I think we only need to duplicate the Build wheel step inside the _buildwheels job, extending the duplicate's env field with: CIBW_MANYLINUX_X86_64_IMAGE: manylinux_2_28, and only running this job in case the matrix.identifier specifies x86_64 architecture ( if: contains(matrix.identifier, 'x86_64')),

      - name: Build wheel
        uses: pypa/cibuildwheel@v2.16.5
        with:
          only: ${{ matrix.identifier }}
        env:
          CIBW_BUILD_VERBOSITY: 1

      - name: Build wheel for manylinux_x86_64 with manylinux_2_28
        uses: pypa/cibuildwheel@v2.16.5
        with:
          only: ${{ matrix.identifier }}
        env:
          CIBW_BUILD_VERBOSITY: 1
          CIBW_MANYLINUX_X86_64_IMAGE: manylinux_2_28
       if: contains(matrix.identifier, 'manylinux_x86_64')

Although the first step (Build wheel) would continue to use the default values for CIBW_MANYLINUX_X86_64_IMAGE and CIBW_MANYLINUX_I686_IMAGE -- creating wheels for manylinux_2_17 with the manylinux2014 image --, it might be cleaner to explicitly add:

    CIBW_MANYLINUX_X86_64_IMAGE: manylinux2014
    CIBW_MANYLINUX_I686_IMAGE: manylinux2014

However, this might be a bit confusing given that this step also handles identifiers like cp36-musllinux_x86_64 -- in which case these env vars are irrelevant, and they are simply ignored..hopefully.

tkittel commented 4 months ago

Sounds good to me @MilanKlausz ! I'll leave the decision of whether or not to set manylinux2014 explicitly up to your judgement!

tkittel commented 4 months ago

Also, since you are working on it now, I won't disturb by accepting the following PR: https://github.com/mctools/mctools_actions/pull/10/files

Feel free to do it (or just perform the update manually when you are anyway editing the files).

tkittel commented 4 months ago

@MilanKlausz made it work now for NCrystal 3.8.0 wheels: :partying_face:

Keeping this issue open Milan until you think that the infrastructure is completely finalised.

tkittel commented 4 months ago

@MilanKlausz I have a few tiny fixes, so I can make a 3.8.1 release later this week so we can test that everything works automatically as it should on a new future release. Just let me know when you believe it is ready.

MilanKlausz commented 4 months ago

For the record, fixing this issue revealed that the recent actions/upload-artifact v3->v4 upgrade broke our way of uploading 1 distribution(wheel or sdist) to the same artifact in the job matrix (same issue reported by others), and then downloading/unpacking a single artifact before during the publishing job. My solution (instead of rollback to v3 as many others do) is to

The other problem encountered was that in our previous setup, we verified the completeness of the upload by checking that the number of distributions on PyPI (for the relevant version) matches the number of matrix jobs+1, as each matrix job used to create 1 wheel file +1 separate job the sdist. Now, with the 'minimal change' of adding a separate step to the matrix job to create an additional wheel with the manylinux_2_28 image for manylinux_x86_64, the 1 wheel / 1 job relation isn't true anymore. It is now fixed by expecting an additional manylinux_x86_64_setup_number more files to be present on PyPI.