GH-46410: [C++] Add parquet options to Meson configuration #46647
Conversation
|
|
| @@ -66,6 +66,22 @@ option( | 10000|||
| type: 'string', | |||
| description: 'Arbitrary string that identifies the kind of package (for informational purposes)', | |||
| ) | |||
| option('parquet', type: 'feature', description: 'Build the Parquet libraries') | |||
There was a problem hiding this comment.
There are a few differences with the CMake configuration worth calling out.
First, CMake has an extra option for PARQUET_MINIMAL_DEPENDENCY. This seems to maybe not be used all of that often, so I did not bother implementing here. Let me know if that is a mistake.
CMake also requires that when either the executables or examples are built that you use static libraries. My assumption is that was done to make the CMake configuration easier, so I did not place that same restriction in the Meson configuration.
Finally, CMake will fail if a pre-installed version of OpenSSL is not found and encryption is required. This configuration is not as strict, and will use Meson's wrap system to resolve the OpenSSL dependency if it is not provided by the system
840cc24 to
ee277eb
Compare
2a8b860 to
764f5a2
Compare
|
@kou this one might have gotten missed - mind taking a look when you have time? |
cpp/examples/parquet/meson.build
Outdated
| }, | ||
| } | ||
|
|
||
| if get_option('parquet_require_encryption').enabled() |
There was a problem hiding this comment.
Can we use needs_parquet_encryption here?
cpp/meson.build
Outdated
| ' Meson >= 1.8.0 is required for fuzzing support, found @0@'.format( | ||
| meson.version(), | ||
| ), |
There was a problem hiding this comment.
Can we use f'... @meson_version@ here?
| option('parquet', type: 'feature', description: 'Build the Parquet libraries') | ||
| option( | ||
| 'parquet_build_executables', | ||
| type: 'feature', |
There was a problem hiding this comment.
Ah, should we use boolean not feature for option that doesn't depend on any dependency?
There was a problem hiding this comment.
Hmm not sure. I think feature is nice because it lets you easily toggle everything on/off (particularly useful for our current CI setup) but don't have a strong point of view here
| ) | ||
| option( | ||
| 'parquet_build_examples', | ||
| type: 'feature', |
| if needs_parquet_encryption | ||
| openssl_dep = dependency('openssl') |
There was a problem hiding this comment.
Ah, we may need to use the feature object not feature.enabled() to support auto:
| if needs_parquet_encryption | |
| openssl_dep = dependency('openssl') | |
| openssl_dep = dependency('openssl', required: get_option('parquet_require_encryption')) | |
| if openssl_dep |
There was a problem hiding this comment.
I've updated this logic slightly, although I'm not sure if I accomplished what you are after. Let me know!
ff43c0f to
19aa192
Compare
|
Hey @kou just wanted to toss a friendly ping on this. If you have time, please let me know what you think of the latest updates. Thanks! |
cpp/src/parquet/meson.build
Outdated
| openssl_dep = dependency('openssl', required: needs_parquet_encryption) | ||
| if openssl_dep.found() |
There was a problem hiding this comment.
It seems that Parquet encryption is enabled even when parquet_require_encryption=disabled and system OpenSSL exists.
I think that the following is expected:
parquet_require_encryption=enabled:dependency('openssl')must be found- Parquet encryption is built
parquet_require_encryption=auto:- Parquet encryption is built only when
dependency('opnessl')is found
- Parquet encryption is built only when
parquet_require_encryption=disabled:- Don't execute
dependency('openssl') - Parquet encryption is not built
- Don't execute
There was a problem hiding this comment.
OK I think the latest iteration simplifies this by branching only when the parquet_require_encryption option is enabled.
Note that the auto feature in Meson is still either in an enabled or disabled state; it just depends if auto_features=enabled or auto_features=disabled when the project is configured
There was a problem hiding this comment.
Oh, really?
Do you know the auto feature documentation?
I saw https://mesonbuild.com/Reference-manual_returned_feature.html . It includes feature.allowed() and feature.auto(). It seems that get_option('parquet_require_encryption') may be auto.
19aa192 to
2484174
Compare
Rationale for this change
This continues adding functionality to the Meson configuration
What changes are included in this PR?
This adds the parquet directory to the Meson configuration
Are these changes tested?
Yes
Are there any user-facing changes?
No