-
Notifications
You must be signed in to change notification settings - Fork 505
ORC-1906: Support Meson build
#2249
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
8372448 to
3c290db
Compare
WillAyd
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.
@wgtmac @dongjoon-hyun hope this is a reasonable starting point for the Meson configuration. More options and features can of course be added over time :-)
|
closes #2244 |
|
Thank you for making a PR. Could you make CI happy, @WillAyd ? It seems that |
Meson build
dongjoon-hyun
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.
Thank you for updating, @WillAyd .
In general, it looks okay although the new directory seems to make the source tree complicated.
However, the library difference could be very misleading to the Apache ORC community users. Ideally, build systems are supposed to be orthogonal to the ORC dependency. I'd recommend to use the matched versions in Mesos build too if there is no inevitable reasons. Especially, Protobuf difference means a breaking difference. So, I believe it should be identical.
|
I'm not sure if Meson intends for each library to house its own version of dependencies. Taking the protobuf dependency as an example, the version that Orc requires is different than the one Arrow requires, neither of which match the version that is installed on a major platform like current Debian. So enforcing exact version requirements I think makes it more difficult to build these packages, especially together. @eli-schwartz is better qualified to talk about the implications of the dependency version management |
|
That's too bad. Unfortunately, this sounds like a blocker for adding 3rd-party
Since this is the official Apache ORC main repository, we don't want to introduce any confusions or risks to the existing Apache ORC users and non-memos down-streams. If this is inevitable, I'd recommend to keep this specific incompatible changes in the AS-IS way only in the |
|
Meson doesn't have strong opinions about what version you use. If you build ORC as a third-party vendored dependency of some other project, and that project ALSO uses protobuf, then that other project, being the top-level project, gets "first dibs" on building a copy of protobuf. There can only be one version per build directory, due to the diamond dependency rule, and multiple subprojects can each have a wrap file but "first come, first served". The diamond dependency rule can't really be "solved" by build systems. If you try to link multiple versions of protobuf into one executable process space, they will stomp on each others' internal data. If ORC requires specific versions of protobuf and fails otherwise, that information should be encoded as: dependency('protobuf', version: ['>=minimum', '<=maximum'])which will of course be respected. If it supports multiple versions but there's a specific recommended one, use whichever version makes both you and the reviewers happy :D probably the one currently in use. Linux distros building ORC already want to ignore vendored code copies of dependencies, which means they need to successfully build with the latest system-packaged /usr/lib64/libprotobuf.so |
|
Thanks @eli-schwartz that context is really helpful. @dongjoon-hyun for now I have downgraded the versions pulled in by Meson's wrapdb for googletest and protobuf to match the existing CMake configuration. The snappy version is the only one left I think that is different, but that specific version doesn't exist in the wrapdb yet. I'll take a look at adding that |
a2b59f9 to
b3d0094
Compare
google-snappy 1.2.2 was added upstream to the Meson WrapDB today and I've updated this PR to use that. Hope I've addressed all your feedback @dongjoon-hyun but let me know if anything is missing |
dongjoon-hyun
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.
Thank you for updating, @WillAyd . I reviewed again and added comments.
de7ed09 to
4ebcabf
Compare
dongjoon-hyun
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.
Thank you for update, @WillAyd . However, this seems to be ignored from my perspective.
https://github.com/apache/orc/pull/2249/files#r2136861282
|
Sorry about that - I think I've addressed it now |
dongjoon-hyun
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, LGTM. Thank you so much, @WillAyd and @eli-schwartz .
|
cc @williamhyun , @wgtmac , @ffacs , too. |
williamhyun
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 LGTM, will be merging this now.
|
Thank you @WillAyd, @eli-schwartz, and @dongjoon-hyun for your contributions! |
|
@williamhyun you can use willayd - thanks! |
FYI, JIRA ID means the ID at https://issues.apache.org , @WillAyd . |
|
Ah sorry - my Jira id is willayd |
|
No problem at all. I added you to the Apache ORC contributor group and assigned ORC-1906 to you.
|
### What changes were proposed in this pull request? This PR aims to upgrade ORC to 2.2.0. 2.2.0 RC1 is currently under voting. ### Why are the changes needed? Apache ORC 2.2.0 is a new feature release. - https://github.com/apache/orc/releases/tag/v2.2.0 - apache/orc#2032 - apache/orc#2338 - apache/orc#2269 - apache/orc#2249 - apache/orc#2144 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #51676 from williamhyun/ORC-2.2.0. Authored-by: William Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

What changes were proposed in this pull request?
This adds a Meson configuration for the default C++ options in ORC. By adding this configuration, users of the Meson build system will have an easier time including Orc into their projects. In the mid to longer term, this could help the Apache Arrow project also become easier to include in other C++ project
Why are the changes needed?
The current motivating factor is for Apache Arrow, where incremental improvements to the Meson build system are underway
How was this patch tested?
A new CI job for Meson has been configured
Was this patch authored or co-authored using generative AI tooling?
No
Closes #2244