Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-17692: [R] Add support for building with system AWS SDK C++ #14235

Merged
merged 45 commits into from
Dec 17, 2022

Conversation

thisisnic
Copy link
Member

@thisisnic thisisnic commented Sep 26, 2022

This PR uses "pkg-config --static ... arrow" to collect build flags. "pkg-config --static ... arrow" reports suitable build flags that depend on build options and used libraries for Apache Arrow C++. This works with the system AWS SDK C++.

@github-actions
Copy link

r/configure Outdated
Comment on lines 258 to 259
# We're depending on openssl/curl/AWS SDK from the system, so they're not in the bundled deps
BUNDLED_LIBS="$BUNDLED_LIBS -lssl -lcrypto -lcurl -laws-cpp-sdk-identity-management -laws-cpp-sdk-sts -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common"
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine, but I honestly am very confused what BUNDLED_LIBS is supposed to represent (or, why aren't we just adding them to PKG_LIBS?). Maybe it's that these are the dependencies of what is in the bundled libraries, so they need to after when linking?

Copy link
Member

Choose a reason for hiding this comment

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

  1. We need to add AWS SDK C++ related -lXXXs only when Apache Arrow C++ is built with system AWS SDK C++ (If we can require arrow.pc, we can implement it simply...)
  2. We need to add AWS SDK C++ related -lXXXs before -lssl -lcrypto -lcurl because AWS SDK C++ depend on -lssl -lcrypto -lcurl

Copy link
Member

Choose a reason for hiding this comment

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

I think we can use pkg-config to detect what libs are required from the system. In fact, we already do on L187. So is the right fix in the arrow.pc file? (Or probably in the pkg-config file that aws-sdk-cpp produces?)

Copy link
Member

Choose a reason for hiding this comment

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

OK. Then I can work on the arrow.pc fix. We need to improve Apache Arrow C++'s CMake codes for it.

(Or probably in the pkg-config file that aws-sdk-cpp produces?)

Unfortunately, AWS SDK C++ doesn't provide .pc files...

@kou kou force-pushed the ARROW-17692_s3_flags branch from 21ce97e to 37f7829 Compare September 28, 2022 03:15
@kou
Copy link
Member

kou commented Sep 28, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Sep 28, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou kou changed the title ARROW-17692 [R]: Arrow Package Installation: undefined symbol error ARROW-17692 [R]: Add support for building with system AWS SDK C++ Sep 28, 2022
@kou kou force-pushed the ARROW-17692_s3_flags branch from ef5da47 to 58810ee Compare October 1, 2022 20:29
@kou
Copy link
Member

kou commented Oct 1, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 1, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 3, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou kou force-pushed the ARROW-17692_s3_flags branch from ce7b7f8 to 89fd5e0 Compare October 4, 2022 00:43
@kou
Copy link
Member

kou commented Oct 4, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 4, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 4, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 4, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou
Copy link
Member

kou commented Oct 4, 2022

@github-actions crossbow submit -g r

@github-actions

This comment was marked as outdated.

@kou kou force-pushed the ARROW-17692_s3_flags branch from 0ebbdda to 6be7d82 Compare October 5, 2022 02:49
@kou
Copy link
Member

kou commented Oct 5, 2022

@github-actions crossbow submit -g r

@kou kou force-pushed the ARROW-17692_s3_flags branch from 1003ccb to 2ee02b6 Compare December 16, 2022 04:36
@kou
Copy link
Member

kou commented Dec 16, 2022

@github-actions crossbow submit -g r

@github-actions
Copy link

Revision: 2ee02b6

Submitted crossbow builds: ursacomputing/crossbow @ actions-52774bb977

Task Status
conda-linux-aarch64-cpu-r41 Azure
conda-linux-aarch64-cpu-r42 Azure
conda-linux-x64-cpu-r41 Azure
conda-linux-x64-cpu-r42 Azure
conda-osx-arm64-cpu-r41 Azure
conda-osx-arm64-cpu-r42 Azure
conda-osx-x64-cpu-r41 Azure
conda-osx-x64-cpu-r42 Azure
conda-win-x64-cpu-r41 Azure
homebrew-r-autobrew Github Actions
homebrew-r-brew Github Actions
r-binary-packages Github Actions
test-fedora-r-clang-sanitizer Azure
test-r-arrow-backwards-compatibility Github Actions
test-r-depsource-bundled Azure
test-r-depsource-system Github Actions
test-r-dev-duckdb Github Actions
test-r-devdocs Github Actions
test-r-gcc-11 Github Actions
test-r-gcc-12 Github Actions
test-r-install-local Github Actions
test-r-install-local-minsizerel Github Actions
test-r-library-r-base-latest Azure
test-r-linux-as-cran Github Actions
test-r-linux-rchk Github Actions
test-r-linux-valgrind Azure
test-r-minimal-build Azure
test-r-offline-maximal Github Actions
test-r-offline-minimal Azure
test-r-rhub-debian-gcc-devel-lto-latest Azure
test-r-rhub-debian-gcc-release-custom-ccache Azure
test-r-rhub-ubuntu-gcc-release-latest Azure
test-r-rstudio-r-base-4.1-opensuse153 Azure
test-r-rstudio-r-base-4.2-centos7-devtoolset-8 Azure
test-r-rstudio-r-base-4.2-focal Azure
test-r-ubuntu-22.04 Github Actions
test-r-versions Github Actions
test-ubuntu-18.04-r-sanitizer Azure

@kou
Copy link
Member

kou commented Dec 17, 2022

Failures are unrelated because they are happen in nightly CI too.

I merge this.

@kou kou merged commit dca8c07 into apache:master Dec 17, 2022
@ursabot
Copy link

ursabot commented Dec 18, 2022

Benchmark runs are scheduled for baseline = 89d9fa3 and contender = dca8c07. dca8c07 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.46% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.21% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] dca8c07d ec2-t3-xlarge-us-east-2
[Finished] dca8c07d test-mac-arm
[Finished] dca8c07d ursa-i9-9960x
[Finished] dca8c07d ursa-thinkcentre-m75q
[Finished] 89d9fa3d ec2-t3-xlarge-us-east-2
[Finished] 89d9fa3d test-mac-arm
[Finished] 89d9fa3d ursa-i9-9960x
[Finished] 89d9fa3d ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@raulcd
Copy link
Member

raulcd commented Dec 19, 2022

Are there new dependencies required to build due to this PR? I can see the job for our cookbooks fail if I use the nightlies:
https://github.com/raulcd/arrow-cookbook/actions/runs/3730980375/jobs/6328817362

 /usr/bin/ld: cannot find -lnghttp2: No such file or directory
/usr/bin/ld: cannot find -lidn2: No such file or directory
/usr/bin/ld: cannot find -lrtmp: No such file or directory
/usr/bin/ld: cannot find -lssh: No such file or directory
/usr/bin/ld: cannot find -lpsl: No such file or directory
/usr/bin/ld: cannot find -lgssapi_krb5: No such file or directory
/usr/bin/ld: cannot find -llber: No such file or directory
/usr/bin/ld: cannot find -lldap: No such file or directory
/usr/bin/ld: cannot find -llber: No such file or directory
collect2: error: ld returned 1 exit status

This PR could be related right? This is the workflow definition: https://github.com/apache/arrow-cookbook/blob/main/.github/workflows/test_r_cookbook.yml#L47

@raulcd
Copy link
Member

raulcd commented Dec 19, 2022

I'll fix it on the cookbooks, I've tested adding the dependencies on a PR I am already working on.

@assignUser
Copy link
Member

@raulcd afaik it should also work to add aws_SOURCE=BUNDLED (or what ever the var is called) to force building it.

@kou
Copy link
Member

kou commented Dec 19, 2022

Oh... I didn't notice that these libraries are required with pre-built Apache Arrow C++ binaries...

It seems that I should implement the following:

#14235 (comment)

If we use shared libraries, we only need libcurl-dev. We don't need to install additional packages such as libssh-dev because libcurl-dev depends on needed packages for shared linking.

Could you install https://github.com/apache/arrow/pull/14235/files#diff-8a7b064de580a41c3d6212eeeef79930236caf01b58c73407c84fec929b39260R62-R74 for now?

assignUser pushed a commit that referenced this pull request Dec 29, 2022
Add new dependencies to windows builds following #14235
* Closes: #15110

Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
EpsilonPrime pushed a commit to EpsilonPrime/arrow that referenced this pull request Jan 5, 2023
…#15111)

Add new dependencies to windows builds following apache#14235
* Closes: apache#15110

Authored-by: Jonathan Keane <jkeane@gmail.com>
Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants