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

Add support for multiple manylinux versions #193

Conversation

jcfr
Copy link
Contributor

@jcfr jcfr commented Jun 22, 2022

Motivation for this change is to build wheels with C++ ABI compatible with Slicer built using newer g++ having _GLIBCXX_USE_CXX11_ABI defaulting to 0.

See https://gcc.gnu.org/onlinedocs/libstdc++/manual/using_dual_abi.html

Note that the latest Slicer release (5.0) is built using centos7 based 1 environment having _GLIBCXX_USE_CXX11_ABI set to 0.

We should likely build wheels using both manylinux2014 and manylinux_2_28.

As of 2022.06.22, this pull-request uses 2_24, as commented 2 by @henryiii, I will instead update dockcross 3 to add 2_28 based image (instead of 2_24).

References:

Footnotes

  1. https://github.com/Slicer/SlicerBuildEnvironment#supported-build-environments

  2. https://github.com/pypa/manylinux/issues/1332#issuecomment-1157666846

  3. https://github.com/dockcross/dockcross/pull/705

@jcfr
Copy link
Contributor Author

jcfr commented Jun 22, 2022

Attempting to build the wheels using the 2_24 image report the following error:

[633/5991] Building CXX object Modules/Core/Common/src/CMakeFiles/ITKCommon.dir/itkTotalProgressReporter.cxx.o
FAILED: Modules/Core/Common/src/CMakeFiles/ITKCommon.dir/itkTotalProgressReporter.cxx.o
/usr/bin/g++ -DTBB_DEPRECATED=0 -DTBB_USE_CAPTURED_EXCEPTION=0 -I/work/ITK-source/ITK/Modules/ThirdParty/DoubleConversion/src -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/DoubleConversion/src/double-conversion -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/Eigen3/src -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/KWSys/src -I/work/ITK-source/ITK/Modules/ThirdParty/VNL/src/vxl/v3p/netlib -I/work/ITK-source/ITK/Modules/ThirdParty/VNL/src/vxl/vcl -I/work/ITK-source/ITK/Modules/ThirdParty/VNL/src/vxl/core -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/VNL/src/vxl/v3p/netlib -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/VNL/src/vxl/vcl -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/VNL/src/vxl/core -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/Core/Common -I/work/ITK-source/ITK/Modules/Core/Common/include -I/work/ITK-source/ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl/algo -I/work/ITK-source/ITK/Modules/ThirdParty/VNL/src/vxl/core/vnl -I/work/ITK-cp39-cp39-manylinux_2_24_x64/Modules/ThirdParty/VNL/src/vxl/core/vnl -isystem /work/ITK-source/ITK/Modules/ThirdParty/Eigen3/src/itkeigen/.. -isystem /work/oneTBB-prefix/include -O3 -DNDEBUG  -mtune=generic -march=corei7 -Wall -Wcast-align -Wdisabled-optimization -Wextra -Wformat=2 -Winvalid-pch -Wno-format-nonliteral -Wpointer-arith -Wshadow -Wunused -Wwrite-strings -funit-at-a-time -Wno-strict-overflow -Wno-deprecated -Wno-invalid-offsetof -Woverloaded-virtual -Wstrict-null-sentinel  -O3 -DNDEBUG -fPIC -std=c++14 -MD -MT Modules/Core/Common/src/CMakeFiles/ITKCommon.dir/itkTotalProgressReporter.cxx.o -MF Modules/Core/Common/src/CMakeFiles/ITKCommon.dir/itkTotalProgressReporter.cxx.o.d -o Modules/Core/Common/src/CMakeFiles/ITKCommon.dir/itkTotalProgressReporter.cxx.o -c /work/ITK-source/ITK/Modules/Core/Common/src/itkTotalProgressReporter.cxx
In file included from /work/ITK-source/ITK/Modules/Core/Common/include/itkImageRegion.h:33:0,
                 from /work/ITK-source/ITK/Modules/Core/Common/include/itkMultiThreaderBase.h:35,
                 from /work/ITK-source/ITK/Modules/Core/Common/src/itkTotalProgressReporter.cxx:19:
/work/ITK-source/ITK/Modules/Core/Common/include/itkSize.h:242:27: error: ‘ptrdiff_t’ does not name a type
   using difference_type = ptrdiff_t;
                           ^~~~~~~~~
[654/5991] Building CXX object Modules/ThirdParty/VNL/src/vxl/core/vnl/CMakeFiles/itkvnl.dir/Templates/vnl_matrix_fixed+float.allsizes-.cxx.o
ninja: build stopped: subcommand failed.

@jcfr
Copy link
Contributor Author

jcfr commented Jun 23, 2022

Likely a regression introduced by InsightSoftwareConsortium/ITK@c173dfd, I will follow up with a pull request adding #include <cstddef> // For ptrdiff_t where relevant.

@jcfr
Copy link
Contributor Author

jcfr commented Jun 23, 2022

@jcfr jcfr force-pushed the update-from-manylinux2014-to-manylinux_2_24 branch from 3c528ce to bbd9c9b Compare June 23, 2022 03:41
@thewtex
Copy link
Member

thewtex commented Jun 23, 2022

Thanks @jcfr !

We should go directly to 2_28. I believe I tried 2_24 or a similar version in the past and ran into the issues noted by others in other threads, namely regression in GCC version relative to manylinux2014. We do have remote modules that need C++17, so that is also a blocker for 2_24. And, recently, it has been agreed that 2_24 will be deprecated, as discussed in the PyPA issues.

@jcfr
Copy link
Contributor Author

jcfr commented Jun 23, 2022

We should go directly to 2_28 [...] and recently, it has been agreed that 2_24 will be deprecated

Agreed.

I will then add new images for 2_28 in dockcross (and mark 2_24 as unsupported adding links to relevant discussions)

@jcfr
Copy link
Contributor Author

jcfr commented Jun 28, 2022

Updates:

  • With the 2_24 based itk wheels, I confirmed the gcc ABI issue is addressed ✅️

  • After updating the hdf5 mangling specific to the ITK build used in Slicer, I was also able to avoid the segfault related to global symbol.

  • Independentpy of HDF5 (with default loading of factory disabled), I am still having issues ... after I have a better idea I will provide a summary.

@h-vetinari
Copy link

We should go directly to 2_28 [...] and recently, it has been agreed that 2_24 will be deprecated

Agreed.

I will then add new images for 2_28 in dockcross (and mark 2_24 as unsupported adding links to relevant discussions)

Just a lurker, but it seems current work in dockcross/dockcross#705 is still focussing on 2_24?

@jcfr
Copy link
Contributor Author

jcfr commented Jun 29, 2022

@h-vetinari Once the PR you referenced is integrated, I will follow up with a new dockcross one marking the 2_24 image as not longer maintained and adding 2_28 ...

Since there is already an 2_24 image pushed on dockerhub, it will allow to have a reference in the README and focus on the maintenance of the more recent one.

@thewtex
Copy link
Member

thewtex commented Jul 6, 2022

Following up with 2_28 dockcross images / scripts.

@thewtex thewtex merged commit 74561b6 into InsightSoftwareConsortium:master Jul 6, 2022
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.

None yet

3 participants