-
Notifications
You must be signed in to change notification settings - Fork 256
Add Apache Avro wrap #2270
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
base: master
Are you sure you want to change the base?
Add Apache Avro wrap #2270
Conversation
f1fa5ae
to
3146511
Compare
Can we add |
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.
Cool - thanks for doing this!
@@ -0,0 +1,6 @@ | |||
option( | |||
'build_executable', | |||
type: 'boolean', |
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.
FYI on other Apache projects we have been using feature
options instead of boolean. I don't think its a huge deal, but probably nice to stick with that pattern for consistency in the ecosystem
3146511
to
b431290
Compare
@bgilbert any idea why the CI fails with this cryptic message:
|
b431290
to
12a56ae
Compare
releases.json
Outdated
"avrogencpp" | ||
], | ||
"versions": [ | ||
"1.12.99-1" |
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 if you just include the hash here instead of the version you can work around the sanity check issue. So 82a2bc8b034de34626e2ab8bf091234122474d50-1
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'm not quite sure what exactly will do the trick, since differences between the json and the meson build file will lead to some error at some point. Do you happen to know when there will be a new avro release? That might be the easiest...
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.
Not sure. That's also probably a question for @wgtmac
Although as far as the wrapdb is concerned, I think we will wait for the release before merging this. Any commit-hash inclusion here is just a workaround for now to trick CI
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.
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.
BTW, iceberg-cpp is also depending on a hash tag of avro-cpp: https://github.com/apache/iceberg-cpp/blob/main/cmake_modules/IcebergThirdpartyToolchain.cmake#L186
a5213e5
to
9d80573
Compare
@WillAyd how should the dependency be named? For avro-c the pkg-config file is named |
89395e7
to
586d6b9
Compare
Hmm following the c pattern it sounds like avro-cpp is best. @eli-schwartz may have a suggestion here as well |
We definitely want to align with upstream. I agree that given the name "avro-c" it makes a decent amount of sense to use "avro-cpp" as well. Avro upstream has only added CMake package config files for avro-cpp, not for avro-c, and it did that in January which hasn't been incorporated in a release yet. So it can be changed. https://issues.apache.org/jira/browse/AVRO-3088 asked for "this way one could just use |
FYI @wgtmac - you may be interested in the discussion around the package name for Avro |
Its CMake project name is Proposed change: apache/avro#3477 |
586d6b9
to
57661ff
Compare
In principle it wouldn't be a problem to have a CMake target and pkg-config target with different names - this is also the case for zstd (pkg-config: libzstd). However then we should ensure that upstream actually generates a pkg-config file for the C++ version to be consistent. I opened a PR which will include one named |
Let's stop right there. Official meson policy is that it's a big problem, breaks assumptions, and requires what is officially termed as "a workaround" in projects that do I will add to this by pointing out that zstandard canonically, officially, doesn't provide CMake package config files at all. The sole supported build system is GNU Makefiles, all other build systems are unofficial. The unofficial meson support carefully matches the Makefiles, while the unofficial CMake and visual studio solution files do not. One of the ways the CMake unofficial files differ from the Makefiles is that they (alone) do provide unvetted CMake package config files that the zstandard developers have made no real effort to consider the correctness of. You can't assume that those zstd CMake package config files exist at all -- e.g. there are some Linux distros that do provide them, while others do not provide them. |
Right, I have not considered the case where pkg-config is not installed, so point taken. I guess then if the CMake dependency is called |
57661ff
to
0c00687
Compare
Thanks for all of the upstream work on this @wgtmac and @stephanlachnit ! |
Adds a wrap for the C++ bindings of Apache Avro. Since it's only possible to build it without Boost using main, we need to wait until a new release.
TODO:
/cc @WillAyd