Skip to content

Conversation

@kszucs
Copy link
Member

@kszucs kszucs commented Sep 29, 2020

No description provided.

@github-actions
Copy link

@nealrichardson nealrichardson self-requested a review September 29, 2020 20:49
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure how feasible is this, perhaps we can turn off the curl dependency of AWS. I assume linking against the system curl should be fine rather than compiling curl as well.

@nealrichardson nealrichardson requested a review from kou September 29, 2020 20:52
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that AWS::aws-cpp-sdk-config wasn't required although we look for it find_package components.

@nealrichardson
Copy link
Member

nealrichardson commented Sep 30, 2020

@kou since you're looking at this now, wanted to let you know I'm debugging locally how on the rstudio/r-base:3.6-centos7 image, it can't find openssl even though I've yum installed it (debugging and failing, that is, and my next step is the bundled openssl build, the nuclear option I'm afraid). I'm currently skipping that problem though and working on the patch to aws-sdk-cpp so that git isn't required.

@kou
Copy link
Member

kou commented Oct 1, 2020

Thanks for sharing the current status.
I'll also take a look the OpenSSL problem.

@nealrichardson
Copy link
Member

I got past that particular openssl problem by installing openssl-static as @pitrou suggested, though I suspect there's more to the issue.

@corleyma
Copy link
Contributor

corleyma commented Oct 1, 2020

I know that this is not quite related, but since we're looking upstream now... is there any thought to when/if we'll update the AWS SDK version? I ask because as of 1.8.0, the default credentials resolution, etc, was enhanced to more fully match what you might expect from awscli/boto3.

@corleyma
Copy link
Contributor

corleyma commented Oct 1, 2020

I know that this is not quite related, but since we're looking upstream now... is there any thought to when/if we'll update the AWS SDK version? I ask because as of 1.8.0, the default credentials resolution, etc, was enhanced to more fully match what you might expect from awscli/boto3.

Actually, ignore me, I see the version change in this PR now. That's exciting! Given these changes it might be worth (at some point) taking another look at how credentials specification is handled to see if there's anything that can be streamlined.

@nealrichardson
Copy link
Member

@corleyma for R on macOS and Windows we're still using binaries for aws-sdk-cpp that are 1.7, so we can't assume 1.8 in the Arrow code just yet. cf. autobrew/homebrew-core#25

@nealrichardson
Copy link
Member

The latest R failures from the CentOS 7 builds are compiler warnings being converted to errors in the awssdk_ep. As it turns out, aws-sdk-cpp requires gcc >= 4.9, and these builds are on 4.8. So somewhere we're going to need to add a if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "4.9") check.

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-*

@github-actions
Copy link

github-actions bot commented Oct 1, 2020

Revision: 41e2b98e6136a935137061cb3e0292371f989c57

Submitted crossbow builds: ursa-labs/crossbow @ actions-573

Task Status
test-r-linux-as-cran Github Actions
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-r*

@github-actions
Copy link

github-actions bot commented Oct 1, 2020

Revision: a2f90756b9a0f91e4a7c347720b42b1ade567bcb

Submitted crossbow builds: ursa-labs/crossbow @ actions-574

Task Status
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure

@nealrichardson
Copy link
Member

Latest R issue(s):

  • The R Linux builds do a static Arrow C++ build, which gets pulled into a shared arrow.so library with the R bindings when the R package builds. AFAICT, because the C++ build is static, it insists on the static version of openssl. So IIUC when R makes arrow.so, the openssl symbols will be pulled into the shared library--exactly what folks on https://issues.apache.org/jira/browse/ARROW-10138 were concerned to prevent.
  • aws-sdk-cpp doesn't build on gcc 4.8, so we need to catch that in cmake. I'd like to have a way to build like ARROW_S3=IF_POSSIBLE, like build with S3 support if we can but fall back to ARROW_S3=OFF rather than fail the build if some prerequisite is not met (similar to ARROW-8155). If there's no interest in supporting that in cmake itself, I'll find some way to hack that into the R Linux build script.

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-rstudio-r-base-3.6-bionic

@github-actions
Copy link

github-actions bot commented Oct 1, 2020

Revision: 992cb382c84169babf76d85a502c4ec8a16979d8

Submitted crossbow builds: ursa-labs/crossbow @ actions-575

Task Status
test-r-rstudio-r-base-3.6-bionic Azure

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-rstudio-r-base-3.6-bionic

@github-actions
Copy link

github-actions bot commented Oct 1, 2020

Revision: 66593cdf2aeaf67c1b087c4e9d789f8fd35620c8

Submitted crossbow builds: ursa-labs/crossbow @ actions-576

Task Status
test-r-rstudio-r-base-3.6-bionic Azure

@kou
Copy link
Member

kou commented Oct 2, 2020

Linux:

  • We need MinIO for testing S3 filesystem
    • There is no package for MinIO in system package manager
    • MinIO provides binary only for amd64
    • We need to download it in our Dockerfiles for amd64
    • We need to build MinIO on non-amd64 platforms such as arm64

@kou
Copy link
Member

kou commented Oct 2, 2020

  * We need to build MinIO on non-amd64 platforms such as arm64

It's not true: https://dl.min.io/server/minio/release/linux-arm64/archive/

@kou
Copy link
Member

kou commented Oct 5, 2020

We can complete #7997 after we merge this.

@kszucs
Copy link
Member Author

kszucs commented Oct 5, 2020

We can complete #7997 after we merge this.

I have a working PR #8315 based on this branch. I'm going to rebase that to check the builds are still passing.

@kou
Copy link
Member

kou commented Oct 5, 2020

@github-actions crossbow submit *-fedora-cpp -ubuntu-20.04-cpp *-debian-10-cpp

@kou
Copy link
Member

kou commented Oct 5, 2020

OK. I closed #7997.

@kou
Copy link
Member

kou commented Oct 5, 2020

@github-actions crossbow submit *-fedora-cpp

@kou
Copy link
Member

kou commented Oct 5, 2020

@github-actions crossbow submit test-fedora-32-cpp test-debian-10-cpp test-ubuntu-20.04-cpp

@github-actions
Copy link

github-actions bot commented Oct 5, 2020

Revision: 0657942

Submitted crossbow builds: ursa-labs/crossbow @ actions-599

Task Status
test-debian-10-cpp CircleCI
test-fedora-32-cpp CircleCI
test-ubuntu-20.04-cpp Github Actions

@nealrichardson
Copy link
Member

@github-actions crossbow submit -g r

@github-actions
Copy link

github-actions bot commented Oct 5, 2020

Revision: 0657942

Submitted crossbow builds: ursa-labs/crossbow @ actions-600

Task Status
homebrew-r-autobrew TravisCI
test-conda-r-4.0 Github Actions
test-r-linux-as-cran Github Actions
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure
test-ubuntu-18.04-r-sanitizer Azure

@nealrichardson
Copy link
Member

I am revising the R installation guide to mention S3 support and the required system dependencies. Will push that today.

@kszucs @kou are there other developer guides that need revision here? I still think it's odd that we now have a setup with ARROW_DEPENDENCY_SOURCE=BUNDLED but that still requires system dependencies, so I'm guessing there are docs that need to be clarified.

@nealrichardson
Copy link
Member

@github-actions crossbow submit -g r

@github-actions
Copy link

github-actions bot commented Oct 5, 2020

Revision: 548b6f3

Submitted crossbow builds: ursa-labs/crossbow @ actions-601

Task Status
homebrew-r-autobrew TravisCI
test-conda-r-4.0 Github Actions
test-r-linux-as-cran Github Actions
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-centos8 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure
test-ubuntu-18.04-r-sanitizer Azure

@nealrichardson
Copy link
Member

@github-actions crossbow submit test-r-rhub-ubuntu-gcc-release test-r-rocker-r-base-latest

@github-actions
Copy link

github-actions bot commented Oct 5, 2020

Revision: 406febf

Submitted crossbow builds: ursa-labs/crossbow @ actions-603

Task Status
test-r-rhub-ubuntu-gcc-release Azure
test-r-rocker-r-base-latest Azure

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

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

+1 from me. R Linux builds now have ARROW_S3 on where possible, and they (nearly) all run tests with minio too. Thanks everyone for pushing on this!

@kou
Copy link
Member

kou commented Oct 5, 2020

Thanks!

We have https://arrow.apache.org/docs/developers/cpp/building.html#build-dependency-management for C++.
(I think that updating the documentation can be worked as a follow-up task.)

@nealrichardson
Copy link
Member

(I think that updating the documentation can be worked as a follow-up task.)

Ok. I'll make a followup JIRA and merge this. Thanks again!

@nealrichardson
Copy link
Member

kszucs added a commit that referenced this pull request Oct 7, 2020
Depends on #8304

Closes #8315 from kszucs/macos3

Authored-by: Krisztián Szűcs <[email protected]>
Signed-off-by: Krisztián Szűcs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants