-
Notifications
You must be signed in to change notification settings - Fork 3.7k
GH-46092: [C++] Add filesystem related options to Meson #46101
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
2bb0e73
to
a2bdd12
Compare
@github-actions crossbow submit *meson |
Revision: a2bdd12 Submitted crossbow builds: ursacomputing/crossbow @ actions-df2349b5fe
|
cpp/meson.build
Outdated
needs_hdfs = get_option('hdfs') | ||
needs_filesystem = false or needs_hdfs | ||
needs_s3 = get_option('s3') | ||
needs_filesystem = get_option('filesystem') or needs_azure or needs_gcs or needs_s3 or needs_hdfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs_filesystem = get_option('filesystem') or needs_azure or needs_gcs or needs_s3 or needs_hdfs | |
needs_filesystem = get_option('filesystem') or needs_azure or needs_gcs or needs_hdfs or needs_s3 |
cpp/meson.options
Outdated
option( | ||
'gcs', | ||
type: 'boolean', | ||
description: 'Build Arrow with GCS support (requires the GCloud SDK for C++)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on its README: https://github.com/googleapis/google-cloud-cpp?tab=readme-ov-file#google-cloud-platform-c-client-libraries
description: 'Build Arrow with GCS support (requires the GCloud SDK for C++)', | |
description: 'Build Arrow with GCS support (requires the Google Cloud Platform C++ Client Libraries)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem - do you want me to update DefineOptions.cmake
with the new description as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
cpp/src/arrow/filesystem/meson.build
Outdated
if needs_s3 | ||
endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if needs_s3 | |
endif |
if needs_s3 | ||
endif | ||
|
||
if needs_hdfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WillAyd I think that we can merge this once this is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am misunderstanding the request - I removed all the empty branches but this one has a test in it. I think we want to keep this around right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. You're right. I misunderstood.
I thought that we have disabler()
dependency for needs_hdfs
here but it doesn't exist. We need this if needs_hdfs
.
cpp/src/arrow/meson.build
Outdated
# TODO: couldn't find any usage of this in Arrow - do we need? | ||
# set(ENV{AZURE_SDK_DISABLE_AUTO_VCPKG} TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure SDK for C++ prepared vcpkg automatically without this: https://github.com/Azure/azure-sdk-for-cpp/blob/27c2e5d335c9306d08aa49bd5fe92d0cc1628fe0/cmake-modules/AzureVcpkg.cmake#L17-L41
We don't want to use vcpkg here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok great - I will set to FALSE then. Should I update the CMake configuration as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind - ignore comment above. I was thinking this ENABLED something but I see it DISABLEs
cpp/src/arrow/meson.build
Outdated
arrow_filesystem_srcs += [ | ||
'filesystem/gcsfs.cc', | ||
'filesystem/gcsfs_internal.cc', | ||
] | ||
gcs_dep = dependency('google-cloud-cpp') | ||
arrow_filesystem_deps += [gcs_dep] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem at all (for gcs and s3)
cpp/src/arrow/meson.build
Outdated
|
||
if needs_s3 | ||
error('s3 filesystem support is not yet implemented in Meson') | ||
arrow_filesystem_srcs += ['filesystem/s3fs.cc'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove this for now?
a2bdd12
to
3355228
Compare
@github-actions crossbow submit *meson |
Revision: 3355228 Submitted crossbow builds: ursacomputing/crossbow @ actions-f11410af11
|
@@ -346,7 +346,7 @@ takes precedence over ccache if a storage backend is configured" ON) | |||
ARROW_WITH_UTF8PROC) | |||
|
|||
define_option(ARROW_GCS | |||
"Build Arrow with GCS support (requires the GCloud SDK for C++)" | |||
"Build Arrow with GCS support (requires the Google Cloud Platform C++ Client Libraries)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix this failure?
https://github.com/apache/arrow/actions/runs/14430422114/job/40464526899?pr=46101#step:7:423
CMake Error at cmake_modules/DefineOptions.cmake:27 (message):
description for ARROW_GCS contained a
line 86 characters long!
(max is 80). Split it into more lines with semicolons
Call Stack (most recent call first):
cmake_modules/DefineOptions.cmake:47 (check_description_length)
cmake_modules/DefineOptions.cmake:348 (define_option)
CMakeLists.txt:224 (include)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - should be fixed now
999cf12
to
320167c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6b64396. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…#46101) ### Rationale for this change This continues adding support for the Meson build system by adding filesystem support ### What changes are included in this PR? This implements the filesystem directory and adds support for Azure. GCS and S3 are stubbed out but will raise an error when a user attempts to enable them, strictly because they look like larger integrations to get the dependency chain properly configured. ### Are these changes tested? Locally yes ### Are there any user-facing changes? No * GitHub Issue: apache#46092 Authored-by: Will Ayd <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…#46101) ### Rationale for this change This continues adding support for the Meson build system by adding filesystem support ### What changes are included in this PR? This implements the filesystem directory and adds support for Azure. GCS and S3 are stubbed out but will raise an error when a user attempts to enable them, strictly because they look like larger integrations to get the dependency chain properly configured. ### Are these changes tested? Locally yes ### Are there any user-facing changes? No * GitHub Issue: apache#46092 Authored-by: Will Ayd <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
This continues adding support for the Meson build system by adding filesystem support
What changes are included in this PR?
This implements the filesystem directory and adds support for Azure. GCS and S3 are stubbed out but will raise an error when a user attempts to enable them, strictly because they look like larger integrations to get the dependency chain properly configured.
Are these changes tested?
Locally yes
Are there any user-facing changes?
No