-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-46147: [C++] Implement GCS support in Meson #47568
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
|
|
fbf7739 to
5eec224
Compare
| required: false, | ||
| ) | ||
|
|
||
| if not (gcs_common_dep.found() |
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.
This is some unfortunate indentation - opened an upstream issue at mesonbuild/meson#15032
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.
This will be fixed in the next release of Meson
5eec224 to
8e54ffe
Compare
3151a22 to
69e81f2
Compare
|
@kou could use some review on this if you get the chance. Thanks! |
raulcd
left a comment
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.
Thanks for working on this @WillAyd ! LGTM.
Is the idea to have a follow up to provide some fallback similar to what we do if azure dependency is not found?
|
Yes eventually. I just didn't want that to be a blocker for now |
|
For some more context @raulcd, for projects that don't have Meson configurations anywhere (whether in their source tree or in the wrapdb), Meson can even still auto-generate a configuration based off of CMake (as you see in our Azure wrap). This works reasonably well for a few, smaller dependencies, but the more you add the more the auto-generated CMake wrapper struggles to resolve dependencies to other project dependencies. So I think its easier for larger projects like this just to assume a system installation first (that's all we test in CI now anyway) and leave it to a separate initiative to work out the kinks with the auto-generated config |
kou
left a comment
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 0 benchmarking runs that have been run so far on merge-commit bbb3d48. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
Rationale for this change
This continues adding features to the Meson configuration (azure is already implemented)
What changes are included in this PR?
System-provided GCS can be leveraged to build optional GCS features. Note that this does not support Meson to automatically download GCS - GCS is a rather complicated CMake project and does not work well in the Meson/CMake subproject bridge. Another PR will have to implement that, due to its complexity (or ideally a Meson wrap for GCS is created)
Are these changes tested?
Yes
Are there any user-facing changes?
No