-
Notifications
You must be signed in to change notification settings - Fork 195
Fix and optimize string conversions in the S3 VFS. #5545
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
Conversation
7b2b831
to
9159e42
Compare
@hjaekel my latest commits should fix the problems reported in #5352 (comment). I tried building from source using an Alpine Docker container but gave up after some vcpkg-related errors. Could you validate? |
Unfortunately, I still get an error:
|
To explain what happens, I built the AWS SDK with |
Just to motivate you: we are though the main library and are now compiling the unit tests. 😅 Alpine also uses unit-s3.cc:451:76
I will try to put together a Dockerfile, but it is difficult because some Alpine packages need patches as well tiledb needs to be patched. Let's see what I can do. |
Great news. There were only two errors in the tests that are fixed. Can you validate again? You don't have to make the Dockerfile if it's hard; |
Great, everything compiles now against aws-sdk-cpp-1.11.584 on Alpine Linux Edge. However, I have not tested the library with an S3. Thank you for your support. |
PR ready for review and validated. I believe that we should backport it to 2.28.x. Regarding regression testing for these compile errors, I don't think it would be worthwhile to add. Building with the AWS SDK in |
Let's both add a comment and backport. Thanks for the fix! |
Implementation remained in `s3.cc` and explicitly parameterized. `add_front_slash` was updated accordingly.
e729070
to
c7e9e18
Compare
/backport to release-2.28 |
Started backporting to release-2.28: https://github.com/TileDB-Inc/TileDB/actions/runs/15855545883 |
@teo-tsirpanis backporting to "release-2.28" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix and optimize string conversions in the S3 VFS.
Using index info to reconstruct a base tree...
M tiledb/sm/filesystem/s3.cc
Falling back to patching base and 3-way merge...
Auto-merging tiledb/sm/filesystem/s3.cc
CONFLICT (content): Merge conflict in tiledb/sm/filesystem/s3.cc
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix and optimize string conversions in the S3 VFS.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
Fixes #5352. --- TYPE: BUG DESC: Fixed compile errors in the S3 VFS.
Fixes #5352.
TYPE: BUG
DESC: Fixed compile errors in the S3 VFS.