ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain #8757
Conversation
|
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that? |
Maybe you have a different version of that installed and that formats differently? You can also summon the autoformatter with |
|
I'll take a look! Didn't know I could do that. |
|
@github-actions autotune |
|
@xhochy I can't seem to summon the bot. But I read what the action did, and saw that it required a very specific version of the cmake autoformatter, as you suggested. It works now! Thanks for your help! |
The bot did run but didn't make any changes to your CMake files: https://github.com/apache/arrow/runs/1450988053?check_suite_focus=true#step:10:11 |
|
Oh, I know why! The CI queue was full (my test runs were taking hours to go through). By the time the runner got to my PR (Wed, 25 Nov 2020 02:15:47 GMT), my final commit with the manual cmake autoformat had gone in about an hour before. |
7d1014a to
4fd6a36
Compare
|
@github-actions rebase |
979c43a to
953de85
Compare
3c26730 to
b9313da
Compare
|
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after. |
|
Whoops no problem; I’ll rebase it tonight or over the weekend. Thanks for
picking this up again!
…On Fri, Jan 29, 2021 at 12:30 PM Neal Richardson ***@***.***> wrote:
Hi @josiahyan <https://github.com/josiahyan>, apologies for the delay
here. Could you please rebase this? Hopefully we can get this reviewed and
merged soon after.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8757 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACPH2DGRRPB7VG6QSWOYL4DS4LWCVANCNFSM4UA73NKQ>
.
|
d4608a9 to
356c300
Compare
b9313da to
850c239
Compare
|
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master. |
ci/conda_env_unix.yml
Outdated
There was a problem hiding this comment.
What is the reason for the exact pin here?
There was a problem hiding this comment.
The currently available version in conda (1.24) is incompatible with the way I initially packaged it. I believe I saw in the CI run that storage_client was missing or something, as the google-cloud-cpp package has started namespacing its exports (finally!).
Given the hard link against abseil libraries (as gRPC does too), to reduce packaging size, I decided that the version was safer pinned. The abseil libraries actually loaded probably depend on the matrix of abseil versions and users of it (gRPC, and the proposed google-cloud-cpp inclusion), making it more brittle. Personally, I extract it by a bazel query (provided in comments) and a Python script to lookup the symbols in the ar files. I believe the gRPC builds do something similar, or just pull them out from the compiler invocations (does @pitrou or @kou know how they were determined in commits like these?). Ideally the linker would do the dead section elimination, but I didn't dig into why the build system wouldn't allow so (I'm guessing the libs have to be bundled in and linked separately somehow?).
I'm hoping to upgrade the dependency when there is code using it (which would be the GCS implementation of the Arrow file interfaces).
There was a problem hiding this comment.
I determined required abseil libraries manually. CI error helped me.
|
What's the status of this PR? |
Sorry, but I lost track of following up on this PR! I believed it was ready to merge in Feb, but assumed people had lost interest in pursuing (native) GCS support, or that this change was not quite worth making for now. Is there still a need for this? If so, I can attempt to rebase it again. |
I think there's interest (I'm interested, at least). I'd like to see someone more skilled in cmake than I to review and approve: @kou @pitrou @xhochy ? |
|
Could you rebase on the master? Then I'll review this. |
|
Will rebase!
…On Sun, Aug 1, 2021 at 1:46 PM Sutou Kouhei ***@***.***> wrote:
Could you rebase on the master? Then I'll review this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8757 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACPH2DGIQYWZJBQFOK4RMNLT2WXCDANCNFSM4UA73NKQ>
.
|
|
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be. |
7076b4c to
7531ca5
Compare
Whoops sorry, I lost track of this. I’ll be trying to rebase this - I don’t see any major changes (so far) that should result in conflicts. |
| autoconf | ||
| ccache | ||
| google-cloud-cpp=1.20.0 | ||
| nlohmann_json>=3.4.0 |
There was a problem hiding this comment.
what is the new JSON dependency for?
There was a problem hiding this comment.
I guess google's SDK must rely on it?
There was a problem hiding this comment.
I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
There was a problem hiding this comment.
Conda has dependency handling, so there is no reason to mention it explicitly, though.
| GCPSDK_PREFIX_PATH) | ||
|
|
||
| set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}") | ||
| # workaround for crc32c not declaring its header interface in its Find*.cmake file |
There was a problem hiding this comment.
You can avoid this by using crc32c 1.1.1:
|
I’ve discussed this PR with @coryan , and we think its better to start over, given the need to upgrade dependencies, and handle the Abseil libraries (which I believe, when used with gRPC are causing the build to fail on some platforms) with more scripting. He will be taking over this work. |
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.