Skip to content

ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757

Closed
josiahyan wants to merge 10 commits into
apache:masterfrom
josiahyan:ARROW-8147-add-gcp-sdk
Closed

ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom
josiahyan:ARROW-8147-add-gcp-sdk

Conversation

@josiahyan

@josiahyanjosiahyan commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.

@josiahyan

josiahyan commented Nov 24, 2020

Copy link
Copy Markdown
ContributorAuthor

Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?

@josiahyan
josiahyan marked this pull request as draft November 24, 2020 15:27
@github-actions

github-actionsBot commented Nov 24, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147

@xhochy

xhochy commented Nov 24, 2020

Copy link
Copy Markdown
Member

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 @github-actions<space>autotune on the PR.

@josiahyan

josiahyan commented Nov 25, 2020

Copy link
Copy Markdown
ContributorAuthor

I'll take a look! Didn't know I could do that.

@josiahyan

josiahyan commented Nov 25, 2020

Copy link
Copy Markdown
ContributorAuthor

@github-actions autotune

@josiahyan

josiahyan commented Nov 25, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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!

@josiahyan
josiahyan marked this pull request as ready for review November 25, 2020 04:31
@xhochy

xhochy commented Nov 25, 2020

Copy link
Copy Markdown
Member

@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

@josiahyan

josiahyan commented Nov 25, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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.

pitrou
pitrou reviewed Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile Outdated
@josiahyan
josiahyanforce-pushed the ARROW-8147-add-gcp-sdk branch from 7d1014a to 4fd6a36CompareDecember 1, 2020 14:43
@josiahyan

josiahyan commented Dec 1, 2020

Copy link
Copy Markdown
ContributorAuthor

@github-actions rebase

@josiahyan
josiahyanforce-pushed the ARROW-8147-add-gcp-sdk branch 2 times, most recently from 979c43a to 953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson reviewed Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt Outdated
@josiahyan
josiahyanforce-pushed the ARROW-8147-add-gcp-sdk branch from 3c26730 to b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot added the needs-rebase A PR that needs to be rebased by the author label Dec 24, 2020
@nealrichardson

nealrichardson commented Jan 29, 2021

Copy link
Copy Markdown
Member

Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.

@josiahyan

josiahyan commented Jan 29, 2021 via email

Copy link
Copy Markdown
ContributorAuthor

@josiahyan
josiahyanforce-pushed the ARROW-8147-add-gcp-sdk branch from b9313da to 850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan marked this pull request as draft February 28, 2021 01:03
@josiahyan
josiahyan marked this pull request as ready for review February 28, 2021 04:29
@josiahyan

josiahyan commented Feb 28, 2021

Copy link
Copy Markdown
ContributorAuthor

Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.

xhochy
xhochy reviewed Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml Outdated

@xhochyxhochyFeb 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?

@josiahyanjosiahyanFeb 28, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@koukouFeb 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.

@salqadri

salqadri commented Jul 30, 2021

Copy link
Copy Markdown

What's the status of this PR?

@josiahyan

josiahyan commented Aug 1, 2021

Copy link
Copy Markdown
ContributorAuthor

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.

@nealrichardson

nealrichardson commented Aug 1, 2021

Copy link
Copy Markdown
Member

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 ?

@kou

kou commented Aug 1, 2021

Copy link
Copy Markdown
Member

Could you rebase on the master? Then I'll review this.

@josiahyan

josiahyan commented Aug 1, 2021 via email

Copy link
Copy Markdown
ContributorAuthor

@emkornfield

emkornfield commented Aug 20, 2021

Copy link
Copy Markdown
Contributor

@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.

@josiahyan
josiahyanforce-pushed the ARROW-8147-add-gcp-sdk branch from 7076b4c to 7531ca5CompareAugust 23, 2021 05:44
@josiahyan

josiahyan commented Aug 23, 2021

Copy link
Copy Markdown
ContributorAuthor

@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.

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.

@josiahyan
josiahyan marked this pull request as draft August 23, 2021 07:25
emkornfield
emkornfield reviewed Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0

@emkornfieldemkornfieldSep 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?

@emkornfieldemkornfieldSep 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?

@coryancoryanSep 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.

@pitroupitrouSep 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.

coryan
coryan reviewed Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
GCPSDK_PREFIX_PATH)

set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
# workaround for crc32c not declaring its header interface in its Find*.cmake file

@coryancoryanSep 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:

google/crc32c@b377ce4

@josiahyan

josiahyan commented Sep 28, 2021

Copy link
Copy Markdown
ContributorAuthor

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.

@asfimportasfimport mentioned this pull request Oct 5, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou
+1 more reviewer
@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain by josiahyan · Pull Request #8757 · apache/arrow · GitHub
Skip to content
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain - #8757
Closed
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Closed
ARROW-8147: [C++] Add google-cloud-cpp to ThirdpartyToolchain#8757
josiahyan wants to merge 10 commits into
apache:masterfrom 
josiahyan:ARROW-8147-add-gcp-sdk
Conversation
@josiahyan
@josiahyanjosiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

Contributor
I'm hoping to work on parts of the GCS filesystem implementation, and this is a prerequisite.
@josiahyan
josiahyan
commented
Nov 24, 2020

Copy link


Copy Markdown

ContributorAuthor
Running the cmake autoformatter heavily modifies the touched cmake files; should I be doing that?
@josiahyan
josiahyan
marked this pull request as draft
November 24, 2020 15:27
@github-actions
github-actionsBot
commented
Nov 24, 2020

Copy link


Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8147
@xhochy
xhochy
commented
Nov 24, 2020

Copy link


Copy Markdown

Member
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 @github-actions<space>autotune on the PR.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
I'll take a look! Didn't know I could do that.
@josiahyan
josiahyan
commented
Nov 25, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions autotune
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
@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!
@josiahyan
josiahyan
marked this pull request as ready for review
November 25, 2020 04:31
@xhochy
xhochy
commented
Nov 25, 2020

Copy link


Copy Markdown

Member
@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
@josiahyan
josiahyan
commented
Nov 25, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
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.
pitrou
pitrou
reviewed
Dec 1, 2020
View reviewed changes
Comment threadci/docker/conda-cpp.dockerfile
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7d1014a to
4fd6a36CompareDecember 1, 2020 14:43
@josiahyan
josiahyan
commented
Dec 1, 2020

Copy link


Copy Markdown

ContributorAuthor
@github-actions rebase
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
2 times, most recently
from
979c43a to
953de85CompareDecember 1, 2020 16:43
nealrichardson
nealrichardson
reviewed
Dec 1, 2020
View reviewed changes
Comment threadcpp/thirdparty/versions.txt
Outdated

@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
3c26730 to
b9313daCompareDecember 1, 2020 21:21
@github-actionsgithub-actionsBot
added
the 
needs-rebase
A PR that needs to be rebased by the author
label
Dec 24, 2020
@nealrichardson
nealrichardson
commented
Jan 29, 2021

Copy link


Copy Markdown

Member
Hi @josiahyan, apologies for the delay here. Could you please rebase this? Hopefully we can get this reviewed and merged soon after.
@josiahyan
josiahyan
commented
Jan 29, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
b9313da to
850c239CompareFebruary 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as draft
February 28, 2021 01:03
@josiahyan
josiahyan
marked this pull request as ready for review
February 28, 2021 04:29
@josiahyan
josiahyan
commented
Feb 28, 2021

Copy link


Copy Markdown

ContributorAuthor
Sorry about the delay! I've rebased the commits and manually merged in the changes made in master.
xhochy
xhochy
reviewed
Feb 28, 2021
View reviewed changes
Comment threadci/conda_env_unix.yml
Outdated

@xhochyxhochyFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the exact pin here?
@josiahyanjosiahyanFeb 28, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
@koukouFeb 28, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I determined required abseil libraries manually. CI error helped me.
@salqadri
salqadri
commented
Jul 30, 2021

Copy link


Copy Markdown

What's the status of this PR?
@josiahyan
josiahyan
commented
Aug 1, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@nealrichardson
nealrichardson
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
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 ?
@kou
kou
commented
Aug 1, 2021

Copy link


Copy Markdown

Member
Could you rebase on the master? Then I'll review this.
@josiahyan
josiahyan
commented
Aug 1, 2021
via email 

Copy link


Copy Markdown

ContributorAuthor
@emkornfield
emkornfield
commented
Aug 20, 2021

Copy link


Copy Markdown

Contributor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
@josiahyan
josiahyanforce-pushed
the
ARROW-8147-add-gcp-sdk
branch
from
7076b4c to
7531ca5CompareAugust 23, 2021 05:44
@josiahyan
josiahyan
commented
Aug 23, 2021

Copy link


Copy Markdown

ContributorAuthor
@josiahyan do you think you will have time to rebase? Otherwise I can see how difficult this might be.
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.
@josiahyan
josiahyan
marked this pull request as draft
August 23, 2021 07:25
emkornfield
emkornfield
reviewed
Sep 12, 2021
View reviewed changes
Comment threadci/conda_env_unix.txt
autoconf
ccache
google-cloud-cpp=1.20.0
nlohmann_json>=3.4.0
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the new JSON dependency for?
@emkornfieldemkornfieldSep 12, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess google's SDK must rely on it?
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am one of the authors of google's GCS SDK. Yes, the SDK definitely depends on nlohmann_json.
@pitroupitrouSep 27, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conda has dependency handling, so there is no reason to mention it explicitly, though.
coryan
coryan
reviewed
Sep 27, 2021
View reviewed changes
Comment threadcpp/cmake_modules/ThirdpartyToolchain.cmake
 GCPSDK_PREFIX_PATH)

 set(GCPSDK_CXX_FLAGS "${EP_CXX_FLAGS}")
 # workaround for crc32c not declaring its header interface in its Find*.cmake file
@coryancoryanSep 27, 2021

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can avoid this by using crc32c 1.1.1:
google/crc32c@b377ce4
@josiahyan
josiahyan
commented
Sep 28, 2021

Copy link


Copy Markdown

ContributorAuthor
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.
@asfimportasfimport
mentioned this pull request
Oct 5, 2021

Closed

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@koukoukou left review comments
@xhochyxhochyxhochy left review comments
@emkornfieldemkornfieldemkornfield left review comments
@nealrichardsonnealrichardsonAwaiting requested review from nealrichardson
@pitroupitrouAwaiting requested review from pitrou

+1 more reviewer

@coryancoryancoryan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

Component: C++needs-rebaseA PR that needs to be rebased by the author

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@josiahyan@xhochy@nealrichardson@salqadri@kou@emkornfield@pitrou@coryan