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

Update the GCS SDK to its latest version. #4031

Merged
merged 18 commits into from
Oct 5, 2023

Conversation

teo-tsirpanis
Copy link
Member

A subset of #4008, let's see what CI thinks.


TYPE: IMPROVEMENT
DESC: Update the Google Cloud Storage SDK to its latest version.

@teo-tsirpanis
Copy link
Member Author

SC-27301

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #27301: Update to the latest version of the GCS SDK..

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

I think we'll need to make the use of the new zero-copy InsertObject API conditional on the version available at compile-time for now. Moving to GCS 2.9 for 2.16 is blocked by the conda-forge package update for 2.9: conda-forge/google-cloud-cpp-feedstock#129, which is in turn blocked by https://github.com/conda/conda-build/issues/4842.

@teo-tsirpanis
Copy link
Member Author

teo-tsirpanis commented Apr 11, 2023

I don't know how to do that with vcpkg. Better wait until the conda-forge issue is resolved. 2.6.0 is better than 1.11.0.

@ihnorton
Copy link
Member

I don't know how to do that with vcpkg.

You can use >= version specifier.

https://learn.microsoft.com/en-us/vcpkg/users/examples/versioning.getting-started#version-1

@teo-tsirpanis
Copy link
Member Author

How about we close this in favor of #4008? I managed to make CI green.

@teo-tsirpanis
Copy link
Member Author

Hmm, copying the ports to our own folder causes failures. 🤔

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1 The port update matches what I get locally. Everything else looks trivial.

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

Thanks. Fixes sc-34943.

@teo-tsirpanis
Copy link
Member Author

CI fails because it runs out of space. 😭

@teo-tsirpanis
Copy link
Member Author

Maybe we could run a git clean -xdf in the vcpkg directory after we configure? 🤔

@ihnorton
Copy link
Member

ihnorton commented Oct 4, 2023

We are building abseil in both release and debug mode (along with everything else):

2023-10-04T15:55:29.2972334Z -- Fixing pkgconfig file: /home/runner/work/TileDB/TileDB/external/vcpkg/packages/abseil_x64-linux/lib/pkgconfig/absl_variant.pc
2023-10-04T15:55:30.3111449Z -- Fixing pkgconfig file: /home/runner/work/TileDB/TileDB/external/vcpkg/packages/abseil_x64-linux/debug/lib/pkgconfig/absl_absl_check.pc
2023-10-04T15:55:30.3115761Z -- Fixing pkgconfig file: /home/runner/work/TileDB/TileDB/external/vcpkg/packages/abseil_x64-linux/debug/lib/pkgconfig/absl_absl_log.pc
2023-10-04T15:55:30.3121185Z -- Fixing pkgconfig file: /home/runner/work/TileDB/TileDB/external/vcpkg/packages/abseil_x64-linux/debug/lib/pkgconfig/absl_algorithm.pc
...

I think we need to use the approach here across all platforms:

- name: Prevent vcpkg from building debug variants
shell: python
run: |
import os
workspace = os.environ["GITHUB_WORKSPACE"]
triplets = os.path.join(workspace, "external", "vcpkg", "triplets")
for path, dnames, fnames in os.walk(triplets):
for fname in fnames:
fname = os.path.join(path, fname)
print(fname)
with open(fname, "rb") as handle:
contents = handle.read()
# If a file is already patched we skip patching it again because
# this affects the hashes calculated by vcpkg for caching
if b"VCPKG_BUILD_TYPE release" in contents:
continue
contents += b"\nset(VCPKG_BUILD_TYPE release)\n"
with open(fname, "wb") as handle:
handle.write(contents)

@teo-tsirpanis
Copy link
Member Author

I will work on it tomorrow.

@teo-tsirpanis
Copy link
Member Author

Dependecies will be built in Release mode only during CI with #4162.

@teo-tsirpanis
Copy link
Member Author

CI is green.

@ihnorton ihnorton merged commit ea86f85 into TileDB-Inc:dev Oct 5, 2023
50 checks passed
@ihnorton
Copy link
Member

ihnorton commented Oct 5, 2023

Thanks!

@teo-tsirpanis teo-tsirpanis deleted the gcs-update branch October 5, 2023 15:39
KiterLuc pushed a commit that referenced this pull request Nov 30, 2023
This PR:

* Removes the `abseil` vcpkg feature, the abseil linkage tests and the
relevant bootstrap script options. These are dead code since #4055.
  * Abseil is still acquired from vcpkg if GCS is enabled.
* Deletes unused patches for the ExternalProject of the GCS SDK, which
has been removed in #4031.
* Removes the vcpkg direct dependency to liblzma. The Core does not
directly use liblzma.
* Removes the crc32c linkage tests and the related CMake and bootstrap
script options. They are not used by CI and their value has declined
with the advent of vcpkg. Moreover using exported static libraries with
`TILEDB_CRC32` enabled from CMake will fail because there is no
`find_dependency(Crc32c)` in the config file. Nor there is a `crc32`
vcpkg feature. And this option offers nothing at runtime.
* Removes `unit_link_webp` as this was used to prove WebP was properly
included into the build. Now that we have a WebP filter, this code is
not necessary anymore.

---
TYPE: IMPROVEMENT
DESC: Clean-up unused build system code.
TYPE: BUILD
DESC: Remove the CMake and bootstrap script options related to abseil
and crc32c linkage tests.
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

3 participants