Skip to content

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented Sep 3, 2025

What is the purpose of the change

(For example: This pull request improves file read performance by buffering data, fixing AVRO-XXXX.)

Verifying this change

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Extended interop tests to verify consistent valid schema names between SDKs
  • Added test that validates that Java throws an AvroRuntimeException on invalid binary data
  • Manually verified the change by building the website and checking the new redirect

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Sep 3, 2025
@wgtmac
Copy link
Member Author

wgtmac commented Sep 3, 2025

There was a discussion with regard to the package name: mesonbuild/wrapdb#2270 (comment). So I made this PR and would like to hear from your opinion. @thiru-mg

@eli-schwartz
Copy link

For parity with avro-c which in stable releases already installs a file named avro-c.pc I would actually recommend:

  • create and install avro-cpp.pc (pkgconfig file)
  • create and install a CMake package config file which can be found via find_package(avro-cpp) -- note that CMake is case sensitive
  • bonus: for avro-c, you may want to ship a CMake package config file compatible with find_package(avro-c)

Please note in all cases that when providing the same package configs in CMake format as well as in standalone agnostic pkg-config format, the lookup name for pkg-config --cflags --libs ${name} and the CMake function find_package( ${name} ) should always use the same ${name} for both. This is necessary so that build systems which know how to scan for dependencies using multiple types of "finder", can find either one. It is a big problem with find_package() that it doesn't unify searching across pkgconfig and CMake configs, so you need to use ifelse ladders to correctly find dependencies across versions and in the presence of unofficial CMake ports for autotools dependencies. Most people forget those ifelse ladders. But they aren't needed with the Meson Build System, because Meson's dependency('foobar') function will try both pkg-config --cflags --libs foobar and CMake find_package(foobar), and things will simply work out of the box as long as the name for both is the same.

@stephanlachnit
Copy link
Contributor

The package config file should be named avro-cpp-config.cmake for this behavior btw (see https://cmake.org/cmake/help/latest/command/find_package.html#id18).

@wgtmac wgtmac changed the title AVRO-3088: [C++] Rename CMake package name to AvroCpp AVRO-3088: [C++] Rename CMake package name to avro-cpp Sep 4, 2025
@wgtmac
Copy link
Member Author

wgtmac commented Sep 4, 2025

Thanks for your suggestion, @eli-schwartz and @stephanlachnit. I have now switched to use avro-cpp as the CMake package name. Let me know what you think.

BTW, should I incorporate #3479 into this PR as well?

@stephanlachnit
Copy link
Contributor

LGTM! I would probably go with this PR first, then apply #3478 and merge #3479 separately since there is already a discussion.

@wgtmac
Copy link
Member Author

wgtmac commented Sep 4, 2025

Ping @thiru-mg @Fokko @martin-g for help :)

apache/iceberg-cpp#208 is the downstream PR to prove the correctness.

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me 👍

@martin-g
Copy link
Member

martin-g commented Sep 4, 2025

Looks good to me too but I wonder whether it would break someone's setup ?

@eli-schwartz
Copy link

eli-schwartz commented Sep 4, 2025

It is unlikely, because no released version of avro-cpp has the file under any name.

Renaming something before it's released for the first time should be a "free move", although it's possible someone out there is e.g. including this repository as a git submodule and will need to adapt to the rename the next time they update their submodule.

@martin-g
Copy link
Member

martin-g commented Sep 4, 2025

I see !
https://github.com/apache/avro/commits/main/lang/c%2B%2B/cmake/AvroConfig.cmake.in was introduced at Feb 1 2025 and not yet in the wild.
Thanks!

@martin-g martin-g merged commit e6c3087 into apache:main Sep 4, 2025
4 checks passed
@wgtmac
Copy link
Member Author

wgtmac commented Sep 5, 2025

Thanks everyone!

@wgtmac
Copy link
Member Author

wgtmac commented Sep 5, 2025

@martin-g May I ask if there is any plan for the next 1.13.0 release?

@martin-g
Copy link
Member

martin-g commented Sep 5, 2025

May I ask if there is any plan for the next 1.13.0 release?

There are talks about it for several months now...
No idea when it will happen.

opwvhk pushed a commit to opwvhk/avro that referenced this pull request Sep 5, 2025
* AVRO-3088: [C++] Rename CMake package name to AvroCpp

* rename to avro-cpp

(cherry picked from commit e6c3087)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ Pull Requests for C++ binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants