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
Add meson build system #340
base: next
Are you sure you want to change the base?
Conversation
This commit fixes the following compiler diagnostics: ../protoc-c/c_helpers.cc: In function ‘void google::protobuf::compiler::c::PrintComment(google::protobuf::io::Printer*, std::__cxx11::string)’: ../protoc-c/c_helpers.cc:221:25: warning: comparison of integer expressions of different signedness: ‘int’ and ‘std::vector<std::__cxx11::basic_string<char> >::size_type’ {aka ‘long unsigned int’} [-Wsign-compare] for (int i = 0; i < comment_lines.size(); i++) ~~^~~~~~~~~~~~~~~~~~~~~~ ../protoc-c/c_helpers.cc: In function ‘std::set<std::__cxx11::basic_string<char> > google::protobuf::compiler::c::MakeKeywordsMap()’: ../protoc-c/c_helpers.cc:273:21: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] for (int i = 0; i < GOOGLE_ARRAYSIZE(kKeywordList); i++) { ^
This commit removes "-I${top_srcdir}/protobuf-c" from the default AM_CPPFLAGS value and updates several files to include "protobuf-c/protobuf-c.h" rather than "protobuf-c.h". This makes it slightly easier to migrate to meson.
…ll-cxx-output.inc This commit drops the leading path components from the include of the generated file test-full-cxx-output.inc. This makes it easier to migrate to meson.
This commit updates a few tests where we aren't using the correct C signature for main() or are not using any of its parameters (in the case of t/version/version.c.)
This commit adds what should be a feature complete meson build system. The only real difference with the existing autotools build system is that, for the protobuf-c compiler, we now ship "protoc-c" as the actual binary, and "protoc-gen-c" (the plugin for protoc) as a symlink to the compiler, rather than the opposite. This is because meson doesn't have native support for shipping symlinks, and I could not find a way to add dependency edges from the generated .pb-c.[ch] files to the built compiler when the built compiler binary was "protoc-gen-c" the compiler was being invoked indirectly via something like "protoc --plugin=protoc-gen-c=...". There are two meson build options, "build-compiler" and "build-docs". build-compiler controls whether the protoc-c binary is built (and like the autotools build system, this is required to run the test suite), and build-docs controls whether the Doxygen HTML documentation is built.
…ARY_PATH This commit sets the 'build_rpath' argument on the cxx-generate-packed-data executable to the protobuf library's libdir, which supports the use case where the protobuf library is installed into a non-default path. For instance, in the CI environment protobuf is installed into a subdirectory of $HOME, which is not searched by the runtime linker. This approach was recommended on the mesonbuild mailing list.
This is pretty great! We switched to meson ourselves recently and this definitely makes our life easier |
About the proto-c symlink, it seems to me it could be a simple add_install_script. However have you considered simply dropping it altogether? using protoc compiler plugins is the documented way for protobuf and updating a project to use it is a one line change. It also avoids the whole proto-c vs protoc-c confusion that I saw happen many times due to the 'c' suffix of the compiler |
Cool stuff, I'll have a look right now |
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.
Can we get this in on master, it is not meson specific
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.
Looks pretty good to me, see comments inline though. Another general comments are:
- I would split in several meson files so you have the specific bits on the different parts of the project.
- I would remove the sym file and use the visibility flag so you avoid getting it out of sync.
@@ -17,15 +21,10 @@ env: | |||
- PKG_CONFIG_PATH=$HOME/protobuf-$PROTOBUF_VERSION-bin/lib/pkgconfig | |||
|
|||
install: | |||
- pip install --user cpp-coveralls | |||
- pip3 install meson |
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.
as a follow up patch we could add a build on windows now that we have meson. See as an example this:
https://gitlab.gnome.org/GNOME/wing/blob/master/.gitlab-ci.yml
'protobuf-c/protobuf-c.c', | ||
link_args : ['-Wl,--version-script=' + libprotobuf_c_sym_path], | ||
link_depends : libprotobuf_c_sym, | ||
soversion : '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 would suggest to define this as variables at the top of the project
@pbor Re:
During development I did try to use an However, I was getting build failures as I couldn't find a way to express the dependency from the generated Did I miss something, or is this maybe something that could be added to meson? I think I would be OK removing |
@nacho Re:
protobuf-c uses versioned symbols, and I'm not immediately seeing how to do versioned symbols using the gcc visibility attribute? |
This commit addresses the following review feedback * Use lists rather than splitting strings for possible_cc_flags, protoc_gen_c_sources. * Set 'HAVE_PROTO3' immediately after checking the protobuf dependency that it depends on. * Remove 'required : true' from the protobuf and libprotoc dependencies since this is the default.
in glib kind of libraries we create the following macros:
G_UNAVAILABLE looks like this:
And then on the meson.build you define the EXTERN macro as follows:
Not sure if this would be a good replacement for your use case but maybe it is worth checking... |
I can see the following warnings building with meson:
maybe you may want to remove the declaration-after-statement warning |
Filed a ticket to meson: mesonbuild/meson#4131 |
@nacho That doesn't appear to introduce any symbol versions into the built shared library. See this gist for details: https://gist.github.com/edmonds/7eed6a2dd6f4816f3a233b38701dc81f. libprotobuf-c has a stable ABI guarantee. Basically, there will be no backwards incompatible ABI changes until protobuf-c 2.0.0, at which point the SONAME will change, but there are no plans at the moment for a protobuf-c 2.0.0. For forwards compatible ABI changes (i.e., additions of new symbols), the new symbols are introduced with a symbol version, e.g. the In the gist linked above in the libprotobuf-c1 section, you can see this in the See e.g. https://git.kernel.org/pub/scm/linux/kernel/git/kay/libabc.git for a basic example of symbol versioning in a shared library in an autotools project. This is the pattern that libprotobuf-c currently uses in its autotools build system. Libraries that use symbol versioning allow distros to generate more precise dependency information (e.g., see https://wiki.debian.org/UsingSymbolsFiles, https://wiki.debian.org/Projects/ImprovedDpkgShlibdeps for details about how they are handled in Debian). If you are already using symbol versioning in a library, you can't switch to unversioned symbols, or else this will cause an ABI break. There is a way to embed symbol versioning script fragments into the C source files using |
@edmonds are we missing anything else here? |
@nacho This depends on meson 0.51.0 which was just released yesterday, so I'm not in a huge hurry to merge this. I'll probably rebase it and target it for the 1.4.0 release. |
@edmonds now that 0.51.2 was released we could probably start merging this. |
@edmonds any news? |
@nacho Needs a rebase and once we confirm that it still works I think it should be targeted at 1.4.0. |
Unfortunately 489d3b4 did not completely fix the problem - if you try cleaning and rebuilding protobuf-c-native it doesn't take long to reproduce the issue on a 32-core machine. I spent some time trying to debug this but failed, there is still a race between generating t.test-full.pb.h and compiling cxx_generate_packed_data.c despite BUILT_SOURCES and explicit dependencies. I even tried converting the multiple target rules to use grouped targets (&:), that didn't fix it either. Disabling parallelism as a workaround only costs ~20s and it turns out that upstream is switching to Meson soon anyway: protobuf-c/protobuf-c#340 Signed-off-by: Paul Eggleton <paul.eggleton@linux.microsoft.com> Signed-off-by: Khem Raj <raj.khem@gmail.com>
Unfortunately 489d3b4 did not completely fix the problem - if you try cleaning and rebuilding protobuf-c-native it doesn't take long to reproduce the issue on a 32-core machine. I spent some time trying to debug this but failed, there is still a race between generating t.test-full.pb.h and compiling cxx_generate_packed_data.c despite BUILT_SOURCES and explicit dependencies. I even tried converting the multiple target rules to use grouped targets (&:), that didn't fix it either. Disabling parallelism as a workaround only costs ~20s and it turns out that upstream is switching to Meson soon anyway: protobuf-c/protobuf-c#340 Signed-off-by: Paul Eggleton <paul.eggleton@linux.microsoft.com> Signed-off-by: Khem Raj <raj.khem@gmail.com> (cherry picked from commit 3251fe2) Signed-off-by: Armin Kuster <akuster808@gmail.com>
Unfortunately 489d3b4 did not completely fix the problem - if you try cleaning and rebuilding protobuf-c-native it doesn't take long to reproduce the issue on a 32-core machine. I spent some time trying to debug this but failed, there is still a race between generating t.test-full.pb.h and compiling cxx_generate_packed_data.c despite BUILT_SOURCES and explicit dependencies. I even tried converting the multiple target rules to use grouped targets (&:), that didn't fix it either. Disabling parallelism as a workaround only costs ~20s and it turns out that upstream is switching to Meson soon anyway: protobuf-c/protobuf-c#340 Signed-off-by: Paul Eggleton <paul.eggleton@linux.microsoft.com> Signed-off-by: Khem Raj <raj.khem@gmail.com> (cherry picked from commit 3251fe2) Signed-off-by: Armin Kuster <akuster808@gmail.com>
I'm not finding references to |
Probably auto-generated using this regex: Line 60 in 39cd58f
|
As a side note I just noticed a |
Just a question: I hope the original autoconf based build system will stay in place or are you planning to replace it completely with meson? |
meson would be great. |
Unfortunately 489d3b4 did not completely fix the problem - if you try cleaning and rebuilding protobuf-c-native it doesn't take long to reproduce the issue on a 32-core machine. I spent some time trying to debug this but failed, there is still a race between generating t.test-full.pb.h and compiling cxx_generate_packed_data.c despite BUILT_SOURCES and explicit dependencies. I even tried converting the multiple target rules to use grouped targets (&:), that didn't fix it either. Disabling parallelism as a workaround only costs ~20s and it turns out that upstream is switching to Meson soon anyway: protobuf-c/protobuf-c#340 Signed-off-by: Paul Eggleton <paul.eggleton@linux.microsoft.com> Signed-off-by: Khem Raj <raj.khem@gmail.com>
Unfortunately 489d3b4 did not completely fix the problem - if you try cleaning and rebuilding protobuf-c-native it doesn't take long to reproduce the issue on a 32-core machine. I spent some time trying to debug this but failed, there is still a race between generating t.test-full.pb.h and compiling cxx_generate_packed_data.c despite BUILT_SOURCES and explicit dependencies. I even tried converting the multiple target rules to use grouped targets (&:), that didn't fix it either. Disabling parallelism as a workaround only costs ~20s and it turns out that upstream is switching to Meson soon anyway: protobuf-c/protobuf-c#340 Signed-off-by: Paul Eggleton <paul.eggleton@linux.microsoft.com> Signed-off-by: Khem Raj <raj.khem@gmail.com> (cherry picked from commit 3251fe2) Signed-off-by: Armin Kuster <akuster808@gmail.com>
This branch adds a meson build system to protobuf-c. I posted some details on the mesonbuild mailing list here: https://groups.google.com/forum/#!msg/mesonbuild/NOIpv8KaOjE/S6n3JrGnBAAJ.
Unfortunately, the features used in the
meson.build
file are relatively recent (meson ≥ 0.47.0 is required, which is only a two month old release). In the Travis-CI environment it seems sufficient to install the python3 and ninja-build packages that are shipped in xenial and install meson via pip3.The meson build system in this branch is mostly equivalent to the current autotools build system. I did not bother with setting up code coverage reports, though, as I've not found the reports generated by coveralls to be all that useful, as we aren't really adding a whole lot of new code all that frequently.
The only real concession I had to make was to reverse the symlink relationship between
protoc-c
andprotoc-gen-c
. I couldn't figure out a way to express the dependencies on the code generator whenprotoc-c
was a symlink toprotoc-gen-c
, so I swapped them around.I'd like to eventually remove the autotools and CMake build systems provided that meson ends up being a sufficient replacement. (Note that this branch doesn't update the build instructions in the README.)
To build/test/install, do something like:
If everything went correctly, you should get an installation tree like:
@pbor, @lipnitsk, do you have any interest in reviewing this?
Thanks!