ARROW-14506: [C++] Conda support for google-cloud-cpp#11916
ARROW-14506: [C++] Conda support for google-cloud-cpp#11916coryan wants to merge 18 commits intoapache:masterfrom coryan:ARROW-14506-add-google-cloud-cpp-to-conda-files
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? |
ci/docker/conda-cpp.dockerfile
Outdated
There was a problem hiding this comment.
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.
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_STANDARDin 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.
@pitrou do you have any thoughts on whether we can change the default for CMAKE_CXX_STANDARD?
There was a problem hiding this comment.
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.
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.
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.
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.
@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: 4e33f9f7b13119b4bc44dff267e7392c0a752cdc Submitted crossbow builds: ursacomputing/crossbow @ actions-1294
|
|
Revision: 4e33f9f7b13119b4bc44dff267e7392c0a752cdc 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.
|
@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
|
|
@github-actions crossbow submit conda-linux-gcc-py38-cpu |
|
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. |
ci/docker/conda-cpp.dockerfile
Outdated
There was a problem hiding this comment.
@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 <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
6 participants
This PR adds support for
google-cloud-cppto 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_STANDARDargument isprovided.
Abseil should install w/ the correct base/options.h abseil/abseil-cpp#696
google-cloud-cpphas a direct dependency on Abseil,exposed through the headers, one must use C++17 to use
google-cloud-cpptoo.