-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[C++] Move to C++17 #32415
Comments
Where is the lower bound of R support defined (and how/why)? I tried looking but couldn't find anything. I think it'd be a good idea to define some sort of support policy (note, we did this in scipy over the last year or so, allowing us to move from 4.8 to 6.x and now to 8.x).
Barring some progress on the lower bounds for compilers, you'll then be limited from upgrading abseil beyond 20220623.0 (and that's already working more or less by accident since 20211102 on unix is compiled with C+\17 in c-f. Unless we introduce multiple builds per CXX version in conda-forge, this problem will only get worse, because once vc142 becomes the minimum toolchain in the not too distant future, c-f can move to C\+17 also on windows globally). |
Neal Richardson / @nealrichardson: R 3.6 on Windows used an odd gcc 4.9 mingw compiler, and that's the main source of "R requires an old compiler". But we already disable many features on R < 4.0 on Windows, and conditionally disabling more is not a problem. (GCS filesystem support, which uses abseil, is one of those already.) We could drop support for R 3.6 now, but since we can just disable features on the build, we haven't been forced to do so yet. CRAN checks are now all running gcc 8 or newer: https://cran.r-project.org/web/checks/check_flavors.html We have CI that builds arrow on C+\17 (and maybe also 14?). I think Homebrew also bumped up building arrow with C\17 to match abseil (or maybe that's still in the copy of the formula we test in apache/arrow). We also have an open PR to add Azure Blob Storage, which will require C\14: https://github.com/apache/arrow/pull/12914/files#r899724290. So maybe the solution for the abseil issue is to require the newer C\+ standard if using abseil built with it? |
Antoine Pitrou / @pitrou: I agree that for now conda-forge can simply build using C+\17. Just before the minimum version for Arrow is C\+11 doesn't mean you are forbidden to use a newer one :-D |
Centos 7 has the devtoolset backports until GCC 11 (except aarch where it's GCC 10) though... These are obviously available & in use for the manylinux images, and I think they're a very much acceptable requirement for users on such old platforms.
Well, I would like to avoid breaking your CI if possible. :) PS. I hate the JIRA text |
Weston Pace / @westonpace: |
Antoine Pitrou / @pitrou: |
What shape does this requirement take? The defining feature of the devtoolset backports is that they're fully ABI-compatible with the default compiler (i.e. 4.8), and I doubt R hard-depends on the presence of specific bugs in GCC 4.x that were fixed in later versions. |
David Li / @lidavidm: |
Kouhei Sutou / @kou: |
AFAIU, the problem is that the GCS C++ headers use Abseil, so we depend on the ABI no matter what. @coryan Is my understanding right? |
Sorry, I read too quickly; should have been obvious that CentOS has nothing to do with windows. However, the good news is that as soon as you drop support for R<4, the lower bound should be able to move up to GCC 8 (in rtools40; for R 4.0 & 4.1) resp. GCC 10 (in rtools42; for R>=4.2). |
That is correct, FWIW, gRPC now requires C++ >= 14: https://github.com/grpc/proposal/blob/master/L98-requiring-cpp14.md so does google-cloud-cpp: googleapis/google-cloud-cpp#8740 I expect that Protobuf will follow suit sooner rather than later. The following policy, while yet adopted by any of these projects, may be informational: https://opensource.google/documentation/policies/cplusplus-support |
Kouhei Sutou / @kou: |
Weston Pace / @westonpace: "We do not release new versions for R < 4 but we will consider backporting critical security issues" I'm not sure if that would be more or less work than sprinkling more ifdef/checks throughout our code base. |
Neal Richardson / @nealrichardson: That said, IMO the real issue holding us to C+\11 isn't R or Windows but rather CentOS 7 and its default compilers. And I also don't think that abseil or any other optional dependency should determine whether the core Arrow library requires a newer C\+ standard, it should come from the Arrow developer community. |
This sounds like the opposite of what Antoine was saying above (which I tend to agree with, if the R requirements aren't lifted as you describe). Isn't it harder to change stuff on windows (especially when there's an ABI-compatible GCC backport for CentOS)? |
Neal Richardson / @nealrichardson: |
Apologies if I worded this badly (because relatively) - what I meant was: are the default compilers on CentOS really such a hard constraint when an os-native devtoolset install is just a CLI invocation away? Arguably that's the very reason these exist in the first place - because ultra-LTS OSes and up-to-date software don't mix well otherwise. |
Pavel Solodovnikov / @ManManson: If yes, then I think getting a green light after running the complete test suite will be a sign that we can safely advice Arrow users to build with devtoolset on centos and drop the dependency on gcc-4.8. |
H. Vetinari: |
Kouhei Sutou / @kou: |
H. Vetinari:
|
When we decide to switch to C+\14 or C\17, our header files will probably not be valid C+11 anymore, so it's not just a linker issue. Either we want Arrow C\ to be compatible with the default g\ (why?) and then we need to still limit ourselves to C+\11, or we are happy telling users they need to use the devtoolset, and then we can switch to C+17 (assuming the R team is ok with dropping R 3.6 on Windows :-) ). |
H. Vetinari: |
Antoine Pitrou / @pitrou: The important issue is about moving away from C+\11 for the whole codebase, i.e. adopt C\17 features in Arrow C\+ itself, not just have an optional dependency which requires it. |
H. Vetinari: |
Kouhei Sutou / @kou: I think that the following 2 cases:
It seems that 2. works too. So I think that we can switch to C+\14 or C\+17. |
Antoine Pitrou / @pitrou: |
Dewey Dunnington / @paleolimbot: I'm not qualified to comment on moving Arrow's C++ code to anything for maintainability purposes...I just wanted to add some context to "we haven't done this yet because R": there really are users out there who want to use Arrow and may have to look elsewhere. |
Neal Richardson / @nealrichardson: Also, in practice, I'm not concerned about arrow R package users on Windows 3.6. The parts of the package we're actively developing- |
Antoine Pitrou / @pitrou: |
Closes #33804 ### Rationale for this change At some point, it would be useful to support the new C++ ABI `_GLIBCXX_USE_CXX11_ABI=1` in pyarrow wheels, especially when moving to C++17: - #32415 I wanted to create a pyarrow wheel that supported the above ABI and adapted the existing CENTOS 7 manylinux2014 Dockerfile/wheel to produce a AlmaLinux 8 manylinux_2_28 Dockerfile/wheel. Publishing wheels with a new ABI needs [careful consideration](https://pypackaging-native.github.io/key-issues/native-dependencies/cpp_deps/) so I think this is low priority, but I thought I'd provide this manylinux_2_28 implementation in case it was useful for current/future adoption. ### What changes are included in this PR? A manylinux_2_28 Dockerfile, adopted from the existing manylinux2014 Dockerfile ### Are these changes tested? Manually tested at present ### Are there any user-facing changes? Yes, there's a major ABI change, as pyarrow will be compiled with `_GLIBCXX_USE_CXX11_ABI=1` * Closes: #33804 Supercedes: * #33805 * Closes: #33804 Lead-authored-by: Simon Perkins <simon.perkins@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…#34818) Closes apache#33804 ### Rationale for this change At some point, it would be useful to support the new C++ ABI `_GLIBCXX_USE_CXX11_ABI=1` in pyarrow wheels, especially when moving to C++17: - apache#32415 I wanted to create a pyarrow wheel that supported the above ABI and adapted the existing CENTOS 7 manylinux2014 Dockerfile/wheel to produce a AlmaLinux 8 manylinux_2_28 Dockerfile/wheel. Publishing wheels with a new ABI needs [careful consideration](https://pypackaging-native.github.io/key-issues/native-dependencies/cpp_deps/) so I think this is low priority, but I thought I'd provide this manylinux_2_28 implementation in case it was useful for current/future adoption. ### What changes are included in this PR? A manylinux_2_28 Dockerfile, adopted from the existing manylinux2014 Dockerfile ### Are these changes tested? Manually tested at present ### Are there any user-facing changes? Yes, there's a major ABI change, as pyarrow will be compiled with `_GLIBCXX_USE_CXX11_ABI=1` * Closes: apache#33804 Supercedes: * apache#33805 * Closes: apache#33804 Lead-authored-by: Simon Perkins <simon.perkins@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…#34818) Closes apache#33804 ### Rationale for this change At some point, it would be useful to support the new C++ ABI `_GLIBCXX_USE_CXX11_ABI=1` in pyarrow wheels, especially when moving to C++17: - apache#32415 I wanted to create a pyarrow wheel that supported the above ABI and adapted the existing CENTOS 7 manylinux2014 Dockerfile/wheel to produce a AlmaLinux 8 manylinux_2_28 Dockerfile/wheel. Publishing wheels with a new ABI needs [careful consideration](https://pypackaging-native.github.io/key-issues/native-dependencies/cpp_deps/) so I think this is low priority, but I thought I'd provide this manylinux_2_28 implementation in case it was useful for current/future adoption. ### What changes are included in this PR? A manylinux_2_28 Dockerfile, adopted from the existing manylinux2014 Dockerfile ### Are these changes tested? Manually tested at present ### Are there any user-facing changes? Yes, there's a major ABI change, as pyarrow will be compiled with `_GLIBCXX_USE_CXX11_ABI=1` * Closes: apache#33804 Supercedes: * apache#33805 * Closes: apache#33804 Lead-authored-by: Simon Perkins <simon.perkins@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
…#34818) Closes apache#33804 ### Rationale for this change At some point, it would be useful to support the new C++ ABI `_GLIBCXX_USE_CXX11_ABI=1` in pyarrow wheels, especially when moving to C++17: - apache#32415 I wanted to create a pyarrow wheel that supported the above ABI and adapted the existing CENTOS 7 manylinux2014 Dockerfile/wheel to produce a AlmaLinux 8 manylinux_2_28 Dockerfile/wheel. Publishing wheels with a new ABI needs [careful consideration](https://pypackaging-native.github.io/key-issues/native-dependencies/cpp_deps/) so I think this is low priority, but I thought I'd provide this manylinux_2_28 implementation in case it was useful for current/future adoption. ### What changes are included in this PR? A manylinux_2_28 Dockerfile, adopted from the existing manylinux2014 Dockerfile ### Are these changes tested? Manually tested at present ### Are there any user-facing changes? Yes, there's a major ABI change, as pyarrow will be compiled with `_GLIBCXX_USE_CXX11_ABI=1` * Closes: apache#33804 Supercedes: * apache#33805 * Closes: apache#33804 Lead-authored-by: Simon Perkins <simon.perkins@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
This was done some time ago, closing. |
The upcoming abseil release has dropped support for C++11, so {}eventually{}, arrow will have to follow. More details here.
Relatedly, when I tried to switch abseil to a newer C++ version on windows, things apparently broke in arrow CI. This is because the ABI of abseil is sensitive to the C++ standard that's used to compile, and google only supports a homogeneous version to compile all artefacts in a stack. This creates some friction with conda-forge (where the compilers are generally much newer than what arrow might be willing to impose). For now, things seems to have worked out with arrow specifying C+\11 while conda-forge moved to C\+17 - at least on unix, but windows was not so lucky.
Perhaps people would therefore also be interested in collaborating (or at least commenting on) this issue, which should permit more flexibility by being able to opt into given standard versions also from conda-forge.
Update:
It was voted on the dev ML to move to C++17:
https://lists.apache.org/thread/h9v83rwdl015z2j6s8zwdr1qp4svb5j8
https://lists.apache.org/thread/dod96gbqtfz7pf096vhlczq6f5hv81z8
Reporter: H. Vetinari
Subtasks:
Related issues:
Note: This issue was originally created as ARROW-17110. Please see the migration documentation for further details.
The text was updated successfully, but these errors were encountered: