-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Use CXX11_ABI as default #4387
Use CXX11_ABI as default #4387
Conversation
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
Update CI to explicitly configure CXX11_ABI
0b09fe7
to
319eeb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ssheorey)
CMakeLists.txt, line 54 at r1 (raw file):
option(STATIC_WINDOWS_RUNTIME "Use static (MT/MTd) Windows runtime" ON ) endif() option(GLIBCXX_USE_CXX11_ABI "Set -D_GLIBCXX_USE_CXX11_ABI=1" ON)
We can add a check under # Catch a few incompatible build options
: if TF=ON or PYTORCH=ON, we can assert that GLIBCXX_USE_CXX11_ABI=OFF.
.github/workflows/Dockerfile.openblas, line 105 at r1 (raw file):
-DCMAKE_CXX_COMPILER=g++ \ -DBUILD_FILAMENT_FROM_SOURCE=ON \ -DGLIBCXX_USE_CXX11_ABI=ON \
Unless it's different than the x86 default, we can remove this, and just use the default ON
,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yxlao)
CMakeLists.txt, line 54 at r1 (raw file):
Previously, yxlao (Yixing Lao) wrote…
We can add a check under
# Catch a few incompatible build options
: if TF=ON or PYTORCH=ON, we can assert that GLIBCXX_USE_CXX11_ABI=OFF.
There's a check for that already inside FindPyTorch.cmake and FindTensorFlow.cmake. That check is more accurate, since it actually detects and matches the ABI of the TF / PyTorch binaries instead of assuming it is OFF. TF / PyTorch will switch to the new ABI as soon as CentOS 7 (manylinux2014) is EOL in 2024, but users with specific needs may build TF / PyTorch with the new ABI as well.
.github/workflows/Dockerfile.openblas, line 105 at r1 (raw file):
Previously, yxlao (Yixing Lao) wrote…
Unless it's different than the x86 default, we can remove this, and just use the default
ON
,
It's the same, but I wanted it to be explicit since we explicitly disable Torch / TF Ops (also the default options). If we decide to enable them in the future this will be a reminder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ssheorey)
Update CI to explicitly configure CXX11_ABI
Remove unused HAS_GLIBCXX_USE_CXX11_ABI
Resolves #2286
Thanks to debugging from @SoulProtagonist
#2286 (comment)
Note:
-D GLIBCXX_USE_CXX11_ABI=OFF
, as before. It is possible to build with TF Ops or PyTorch Ops with the CXX11_ABI if TF and PyTorch are built from source with the new CXX11_ABI.-D GLIBCXX_USE_CXX11_ABI=OFF
as before, since PyTorch and TensorFlow Ops are included.This change is