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-15451: [C++] Fix build with C++17 and ARROW_GCS=ON #12257

Closed
wants to merge 2 commits into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jan 25, 2022

Bump the vendored google-cloud-cpp version so that CMAKE_CXX_STANDARD doesn't get ignored.

Also cleanup CMAKE_CXX_STANDARD handling in a few other places.

@pitrou pitrou requested a review from kszucs January 25, 2022 15:33
@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

@coryan Does this look ok to you?

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Yes, this LGTM. At some point you may want to update the google-cloud-cpp dependency in vcpkg, just for consistency I think.

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

Ok, I'll let @kszucs choose how to handle the vcpkg configuration.

@github-actions
Copy link

@kszucs
Copy link
Member

kszucs commented Jan 25, 2022

Ok, I'll let @kszucs choose how to handle the vcpkg configuration.

Can't because we're not using the vcpkg versioning feature yet (because of various AWS SDK issues), so we have a specific vcpkg revision with a set of patches for those particular ports. If we'd switch to vcpkg versioning then the custom patching wouldn't work because vcpkg checks out each port's pinned git tree (struggled with it during the weekend).

We need to fix the vcpkg ports upstream and/or provide overlay ports (basically overriding certain ports) then we can switch to use vcpkg versioning, then we can bump ORC's version there.

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

@kszucs We're already versioning google-cloud-cpp in vcpkg, aren't we?

    {
      "name":"google-cloud-cpp",
      "version>=": "1.32.1",
      "default-features": false,
      "features": [
        "storage"
      ]
    },

@kszucs
Copy link
Member

kszucs commented Jan 25, 2022

@pitrou requires a cmake format

@kszucs
Copy link
Member

kszucs commented Jan 25, 2022

@github-actions crossbow submit test-ubuntu-20.04-cpp-bundled

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

@github-actions
Copy link

Revision: 54d6ee5

Submitted crossbow builds: ursacomputing/crossbow @ actions-1488

Task Status
test-ubuntu-20.04-cpp-bundled Github Actions

@kszucs
Copy link
Member

kszucs commented Jan 25, 2022

@kszucs We're already versioning google-cloud-cpp in vcpkg, aren't we?

    {
      "name":"google-cloud-cpp",
      "version>=": "1.32.1",
      "default-features": false,
      "features": [
        "storage"
      ]
    },

We have a new ci/vcpkg/vcpkg.json for the wheels.

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

We have a new ci/vcpkg/vcpkg.json for the wheels.

Shouldn't it be folded back into the main one? The "features" thing looks useful.

@kszucs
Copy link
Member

kszucs commented Jan 25, 2022

We have a new ci/vcpkg/vcpkg.json for the wheels.

Shouldn't it be folded back into the main one? The "features" thing looks useful.

Eventually yes, possibly for 8.0.

@pitrou pitrou force-pushed the ARROW-15451-gcs-cxx-standard branch from 102e33e to 98b3c32 Compare January 25, 2022 16:38
@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

Rebased.

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

@pitrou
Copy link
Member Author

pitrou commented Jan 25, 2022

Also triggered Linux packaging Crossbow builds: https://github.com/ursacomputing/crossbow/branches/all?query=pitrou-3

@pitrou pitrou closed this in f70acb2 Jan 25, 2022
@pitrou pitrou deleted the ARROW-15451-gcs-cxx-standard branch January 25, 2022 19:46
@ursabot
Copy link

ursabot commented Jan 25, 2022

Benchmark runs are scheduled for baseline = 38d4d77 and contender = f70acb2. f70acb2 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
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.26% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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.

None yet

4 participants