-
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
ARROW-14506: [C++] Conda support for google-cloud-cpp #11916
ARROW-14506: [C++] Conda support for google-cloud-cpp #11916
Conversation
The build failures seem unrelated, but do let me know if I missed something. |
@coryan They are unrelated. I wonder why it's necessary to enable C++17 for the conda builds and not the bundled builds, though? |
Ah, sorry, I've just read the description more attentively :-) |
Ping. Should I take some action or is this ready to be merged? |
@@ -54,6 +56,7 @@ ENV ARROW_BUILD_TESTS=ON \ | |||
ARROW_WITH_SNAPPY=ON \ | |||
ARROW_WITH_ZLIB=ON \ | |||
ARROW_WITH_ZSTD=ON \ | |||
CMAKE_CXX_STANDARD=17 \ |
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.
Note this will break OSX for now, so it should only be set for Linux (as it is done here). See the tracking issue conda-forge/clang-compiler-activation-feedstock#17 for that. Can we remove the explicit setting of CMAKE_CXX_STANDARD
in CMake instead?
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.
Note this will break OSX for now, so it should only be set for Linux (as it is done here).
So this change is doing the "Right Thing"[tm] but maybe accidentally?
See the tracking issue conda-forge/clang-compiler-activation-feedstock#17 for that.
Ack.
Can we remove the explicit setting of
CMAKE_CXX_STANDARD
in CMake instead?
That is way above my pay grade (for this project). It depends on whether arrow supports any compiler where the default C++ version is < C++11. For example, GCC defaults to C++98 until GCC 6.x, and there is a similar story with Clang.
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.
@pitrou do you have any thoughts on whether we can change the default for CMAKE_CXX_STANDARD
?
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.
Hmm. I'm not sure I understand the situation entirely, but for now Arrow C++ needs to compile on gcc 4.9 (and perhaps even gcc 4.8, for a subset of Arrow). This is because of the compiler requirements for R packages...
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.
Also, unless I misunderstand @xhochy 's first message ("it should only be set for Linux (as it is done here)"), it seems there's no real problem here?
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.
My message was mainly a heads-up that on OSX, you need to set CMAKE_CXX_STANDARD
to 14
. This is Linux here, so everything is fine here.
I'm not sure about the general removal of CMAKE_CXX_STANDARD
as google-cloud-cpp does also set it: https://github.com/googleapis/google-cloud-cpp/blob/13ec1e946ae1baad6bcae952daf5910649dcfd0a/CMakeLists.txt#L31-L41 Possibly that combination could also be used here?
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.
Well, we already have the following:
if(NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
endif()
The only difference AFAICT is that we don't error out if the user explicitly asked for something earlier than C++11, but that must be a really rare case.
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.
@xhochy Can you remind me the reason of CMAKE_CXX_STANDARD=17
here? Also, perhaps explain it in a comment?
@github-actions crossbow submit -g conda |
Revision: 4e33f9f Submitted crossbow builds: ursacomputing/crossbow @ actions-1294 |
Revision: 4e33f9f Submitted crossbow builds: ursacomputing/crossbow @ actions-1295 |
Hmm. There are a lot of build failures on conda CI jobs. @xhochy I don't know if you could provide guidance on these? |
@xhochy ping, is there something I can do to make this move forward? |
/cc: @emkornfield |
Conda is not something I'm very familiar with. I'll try once again to ping @xhochy to see if they might be have some free cycles in the new year. |
This PR adds support for `google-cloud-cpp` to the Conda files. Probably the most difficult change to grok is the change to compile with C++17 when using Conda: - Conda defaults all its builds to C++17, [this bug](conda/conda-build#3375) goes into some detail as to why. - Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is provided. - Abseil's ABI changes when used from C++11 vs. C++17, see abseil/abseil-cpp#696 - Therefore, one must compile with C++17 to use Abseil in Conda. - And because `google-cloud-cpp` has a direct dependency on Abseil, exposed through the headers, one must use C++17 to use `google-cloud-cpp` too.
4e33f9f
to
584f3f4
Compare
@github-actions crossbow submit -g conda |
Revision: 584f3f4 Submitted crossbow builds: ursacomputing/crossbow @ actions-1434 |
@github-actions crossbow submit conda-linux-gcc-py38-cpu |
Revision: c0ef3c5 Submitted crossbow builds: ursacomputing/crossbow @ actions-1438
|
@github-actions crossbow submit conda-linux-gcc-py38-cpu |
Revision: 7f3f567 Submitted crossbow builds: ursacomputing/crossbow @ actions-1442
|
@github-actions crossbow submit conda-linux-gcc-py38-cpu |
Revision: 44d84f5 Submitted crossbow builds: ursacomputing/crossbow @ actions-1445
|
Revision: d87af2a Submitted crossbow builds: ursacomputing/crossbow @ actions-1592
|
@github-actions crossbow submit conda-linux-gcc-py37-ppc64le |
Revision: cd79992 Submitted crossbow builds: ursacomputing/crossbow @ actions-1593
|
@coryan Can you have a look at the failure in the last crossbow job? |
Explicitly convert from `std::unique_ptr<Buffer>` to a `std::shared_ptr` on the way to convert to `Result<std::shared_ptr<>>`. I am not sure why it only failed with one compiler.
Fixed, thanks for the heads up. I am not sure why only this compiler complained. Two user-defined conversions are not allowed 🤷 |
@github-actions crossbow submit conda-linux-gcc-py37-ppc64le |
Revision: 8c4f77a Submitted crossbow builds: ursacomputing/crossbow @ actions-1595
|
@github-actions crossbow submit conda-linux-gcc-py37-ppc64le |
Revision: 366b494 Submitted crossbow builds: ursacomputing/crossbow @ actions-1596
|
@github-actions crossbow submit -g conda |
Revision: 366b494 Submitted crossbow builds: ursacomputing/crossbow @ actions-1597 |
@pitrou Ready for review, all 💚 again. |
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.
Just one question.
@@ -54,6 +56,7 @@ ENV ARROW_BUILD_TESTS=ON \ | |||
ARROW_WITH_SNAPPY=ON \ | |||
ARROW_WITH_ZLIB=ON \ | |||
ARROW_WITH_ZSTD=ON \ | |||
CMAKE_CXX_STANDARD=17 \ |
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.
@xhochy Can you remind me the reason of CMAKE_CXX_STANDARD=17
here? Also, perhaps explain it in a comment?
(really great to see all conda builds green again!) |
Is the PR description good enough? |
Woops, sorry. Yes, definitely! |
Benchmark runs are scheduled for baseline = fa4d517 and contender = d78967e. d78967e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
The actual builds were already fixed before (#11916), but now enabling them again to run them nightly. Closes #12492 from jorisvandenbossche/ARROW-14256 Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com> Signed-off-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
This PR adds support for `google-cloud-cpp` to the Conda files. Probably the most difficult change to grok is the change to compile with C++17 when using Conda: - Conda defaults all its builds to C++17, [this bug](conda/conda-build#3375) goes into some detail as to why. - Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is provided. - Abseil's ABI changes when used from C++11 vs. C++17, see abseil/abseil-cpp#696 - Therefore, one must compile with C++17 to use Abseil in Conda. - And because `google-cloud-cpp` has a direct dependency on Abseil, exposed through the headers, one must use C++17 to use `google-cloud-cpp` too. Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com> Co-authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
This PR adds support for `google-cloud-cpp` to the Conda files. Probably the most difficult change to grok is the change to compile with C++17 when using Conda: - Conda defaults all its builds to C++17, [this bug](conda/conda-build#3375) goes into some detail as to why. - Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is provided. - Abseil's ABI changes when used from C++11 vs. C++17, see abseil/abseil-cpp#696 - Therefore, one must compile with C++17 to use Abseil in Conda. - And because `google-cloud-cpp` has a direct dependency on Abseil, exposed through the headers, one must use C++17 to use `google-cloud-cpp` too. Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com> Co-authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
This PR adds support for `google-cloud-cpp` to the Conda files. Probably the most difficult change to grok is the change to compile with C++17 when using Conda: - Conda defaults all its builds to C++17, [this bug](conda/conda-build#3375) goes into some detail as to why. - Arrow defaults to C++11 if no `CMAKE_CXX_STANDARD` argument is provided. - Abseil's ABI changes when used from C++11 vs. C++17, see abseil/abseil-cpp#696 - Therefore, one must compile with C++17 to use Abseil in Conda. - And because `google-cloud-cpp` has a direct dependency on Abseil, exposed through the headers, one must use C++17 to use `google-cloud-cpp` too. Closes apache#11916 from coryan/ARROW-14506-add-google-cloud-cpp-to-conda-files Lead-authored-by: Uwe L. Korn <uwe.korn@quantco.com> Co-authored-by: Carlos O'Ryan <coryan@google.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
This PR adds support for
google-cloud-cpp
to the Conda files.Probably the most difficult change to grok is the change to compile with
C++17 when using Conda:
this bug goes into
some detail as to why.
CMAKE_CXX_STANDARD
argument isprovided.
Abseil should install w/ the correct base/options.h abseil/abseil-cpp#696
google-cloud-cpp
has a direct dependency on Abseil,exposed through the headers, one must use C++17 to use
google-cloud-cpp
too.