Add meson build system by edmonds · Pull Request #340 · protobuf-c/protobuf-c

edmonds · 2018-09-03T23:00:17Z

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 and protoc-gen-c. I couldn't figure out a way to express the dependencies on the code generator when protoc-c was a symlink to protoc-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:

meson build
ninja -C build
ninja -C build test
DESTDIR=/tmp/inst ninja -C build install

If everything went correctly, you should get an installation tree like:

.
└── [4.0K]  usr
    └── [4.0K]  local
        ├── [4.0K]  bin
        │   ├── [6.4M]  protoc-c
        │   └── [   8]  protoc-gen-c -> protoc-c
        ├── [4.0K]  include
        │   ├── [4.0K]  google
        │   │   └── [4.0K]  protobuf-c
        │   │       └── [  29]  protobuf-c.h -> ../../protobuf-c/protobuf-c.h
        │   └── [4.0K]  protobuf-c
        │       └── [ 33K]  protobuf-c.h
        └── [4.0K]  lib
            └── [4.0K]  x86_64-linux-gnu
                ├── [278K]  libprotobuf-c.a
                ├── [  18]  libprotobuf-c.so -> libprotobuf-c.so.1
                ├── [  22]  libprotobuf-c.so.1 -> libprotobuf-c.so.1.0.0
                ├── [208K]  libprotobuf-c.so.1.0.0
                └── [4.0K]  pkgconfig
                    └── [ 216]  libprotobuf-c.pc

10 directories, 9 files

@pbor, @lipnitsk, do you have any interest in reviewing this?

Thanks!

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.

pbor · 2018-09-04T06:02:11Z

This is pretty great! We switched to meson ourselves recently and this definitely makes our life easier
@nacho could you take a look?

pbor · 2018-09-04T06:14:38Z

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

nacho · 2018-09-04T07:01:59Z

Cool stuff, I'll have a look right now

pbor

Can we get this in on master, it is not meson specific

nacho

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.

nacho · 2018-09-04T07:05:35Z

.travis.yml


 install:
-    - pip install --user cpp-coveralls
+    - pip3 install meson


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

meson.build

nacho · 2018-09-04T07:09:33Z

meson.build

+  'protobuf-c/protobuf-c.c',
+  link_args : ['-Wl,--version-script=' + libprotobuf_c_sym_path],
+  link_depends : libprotobuf_c_sym,
+  soversion : '1',


I would suggest to define this as variables at the top of the project

meson.build

edmonds · 2018-09-05T00:20:06Z

@pbor Re:

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.

During development I did try to use an add_install_script to create a symlink from the protoc-gen-c binary to protoc-c, and then, when building the .pb-c.[ch] files in the test suite, calling the just built protoc-gen-c indirectly via the protoc compiler with --plugin=protoc-gen-c=[…]/protoc-gen-c.

However, I was getting build failures as I couldn't find a way to express the dependency from the generated .pb-c.[ch] files to the protoc-gen-c executable. Without being able to express the dependency between the generator and the actual plugin it's possible for a parallel build to attempt to generate .pb-c.[ch] files before the plugin has been built. This holds true even if we get rid of the symlink and just ship a protoc-gen-c binary.

Did I miss something, or is this maybe something that could be added to meson? I think I would be OK removing protoc-c and just shipping the plugin binary if we can make this work.

edmonds · 2018-09-05T00:37:59Z

@nacho Re:

I would remove the sym file and use the visibility flag so you avoid getting it out of sync.

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.

nacho · 2018-09-05T07:11:28Z

in glib kind of libraries we create the following macros:

#if SOUP_VERSION_MIN_REQUIRED >= SOUP_VERSION_2_32
# define SOUP_DEPRECATED_IN_2_32                G_DEPRECATED
# define SOUP_DEPRECATED_IN_2_32_FOR(f)         G_DEPRECATED_FOR(f)
#else
# define SOUP_DEPRECATED_IN_2_32
# define SOUP_DEPRECATED_IN_2_32_FOR(f)
#endif

#if SOUP_VERSION_MAX_ALLOWED < SOUP_VERSION_2_32
# define SOUP_AVAILABLE_IN_2_32                 G_UNAVAILABLE(2, 32) _SOUP_EXTERN
#else
# define SOUP_AVAILABLE_IN_2_32                 _SOUP_EXTERN
#endif

G_UNAVAILABLE looks like this:

#if    __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
#define G_UNAVAILABLE(maj,min) __attribute__((deprecated("Not available before " #maj "." #min)))
#elif defined(_MSC_FULL_VER) && (_MSC_FULL_VER > 140050320)
#define G_UNAVAILABLE(maj,min) __declspec(deprecated("is not available before " #maj "." #min))
#else
#define G_UNAVAILABLE(maj,min) G_DEPRECATED
#endif

And then on the meson.build you define the EXTERN macro as follows:

hidden_visibility_flag = []
if get_option('default_library') != 'static'
  if host_machine.system() == 'windows'
    cdata.set('DLL_EXPORT', true)
    cdata.set('_SOUP_EXTERN', '__declspec(dllexport) extern')
    if cc.get_id() != 'msvc'
      hidden_visibility_flag += ['-fvisibility=hidden']
    endif
  else
    cdata.set('_SOUP_EXTERN', '__attribute__((visibility("default"))) extern')
    hidden_visibility_flag += ['-fvisibility=hidden']
  endif
endif

Not sure if this would be a good replacement for your use case but maybe it is worth checking...

nacho · 2018-09-05T07:32:55Z

I can see the following warnings building with meson:

[43/44] Compiling C object 't_test-generated-code2@exe/t_generated-code2_test-generated-code2.c.o'.
../t/generated-code2/test-generated-code2.c: In function ‘test_oneof_merge’:
../t/generated-code2/test-generated-code2.c:982:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   Foo__SubMess submess = FOO__SUB_MESS__INIT;
   ^
../t/generated-code2/test-generated-code2.c:986:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   ProtobufCBinaryData bd_hello = { 5, (uint8_t*)"hello" };
   ^
../t/generated-code2/test-generated-code2.c: In function ‘test_optional_default_values’:
../t/generated-code2/test-generated-code2.c:1764:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
   Foo__TestMessOptional mess3 = FOO__TEST_MESS_OPTIONAL__INIT;
   ^
../t/generated-code2/test-generated-code2.c: In function ‘test_field_merge’:
../t/generated-code2/test-generated-code2.c:1808:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    int32_t arr1[] = {0, 1};
    ^
../t/generated-code2/test-generated-code2.c:1817:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    size_t msg_size = foo__test_mess_optional__get_packed_size (&msg1);
    ^
../t/generated-code2/test-generated-code2.c:1831:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    int32_t arr2[] = {2, 3, 4};
    ^
../t/generated-code2/test-generated-code2.c:1840:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    uint8_t *packed_buffer = (uint8_t *)malloc (msg_size);
    ^
../t/generated-code2/test-generated-code2.c:1847:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    Foo__TestMessOptional *merged = foo__test_mess_optional__unpack
    ^
../t/generated-code2/test-generated-code2.c:1858:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    int32_t merged_arr[] = {0, 1, 2, 3, 4};

maybe you may want to remove the declaration-after-statement warning

nacho · 2018-09-05T08:05:15Z

Filed a ticket to meson: mesonbuild/meson#4131

edmonds · 2018-09-05T17:25:49Z

@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 protobuf_c_empty_string symbol was introduced with symbol version LIBPROTOBUF_C_1.3.0, while all the other symbols in the ABI were introduced with symbol version LIBPROTOBUF_C_1.0.0.

In the gist linked above in the libprotobuf-c1 section, you can see this in the Version definitions section in the objdump output, and the symbol version appended to each symbol name in the nm --with-symbol-versions output. If you compare that to the output for libsoup, there is no Version definitions section in the objdump output, and the symbol version for each symbol is Base. This means libsoup is not using the symbol versioning mechanism.

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 __asm__(".symver […]"); statements (see https://sourceware.org/binutils/docs/ld/VERSION.html), but I would really rather just maintain the symbol versioning script as a separate file. There is very little maintenance burden by having a separate file since libprotobuf-c has so few symbols.

nacho · 2019-06-11T13:08:11Z

@edmonds are we missing anything else here?

edmonds · 2019-06-17T20:51:17Z

@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.

nacho · 2019-09-25T08:40:40Z

@edmonds now that 0.51.2 was released we could probably start merging this.

nacho · 2020-02-09T14:14:22Z

@edmonds any news?

edmonds · 2020-02-13T23:03:02Z

@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>

morrisonlevi · 2021-04-13T01:37:26Z

I'm not finding references to libprotobuf-c.sym in the autotools stuff. Does it get automatically picked up somehow?

lipnitsk · 2021-04-13T02:21:01Z

Probably auto-generated using this regex:

protobuf-c/Makefile.am

Line 60 in 39cd58f

-export-symbols-regex "^(protobuf_c_[a-z].*)"

lipnitsk · 2021-04-13T02:22:46Z

As a side note I just noticed a ninja reference here. Ninja supports CMake too, but not sure if you wanted to add that into the CI tests. Your call.

jin-eld · 2021-05-20T14:32:01Z

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?

neheb · 2021-06-13T03:57:31Z

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>

edmonds added 11 commits September 2, 2018 20:26

parse_oneof_member(): Fix a -Wdeclaration-after-statement diagnostic

d973390

test-generated-code2.c: Strip path components from include of test-fu…

b59fbf5

…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.

t/: Use "int main(void)" for the main C signature

5991416

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.)

.gitignore: /build/

84999dd

.travis.yml: Drop code coverage reports

7cc1b80

protoc-c: Rename 'legacy_name' to 'standalone_name'

9194116

.travis.yml: Build using meson

01973fb

pbor previously approved these changes Sep 4, 2018

View reviewed changes

nacho reviewed Sep 4, 2018

View reviewed changes

edmonds dismissed pbor’s stale review via 1ec9c1f September 5, 2018 01:04

Conversation

edmonds commented Sep 3, 2018

Uh oh!

pbor commented Sep 4, 2018

Uh oh!

pbor commented Sep 4, 2018

Uh oh!

nacho commented Sep 4, 2018

Uh oh!

pbor left a comment

Choose a reason for hiding this comment

Uh oh!

nacho left a comment

Choose a reason for hiding this comment

Uh oh!

nacho Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nacho Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edmonds commented Sep 5, 2018

Uh oh!

edmonds commented Sep 5, 2018

Uh oh!

nacho commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nacho commented Sep 5, 2018

Uh oh!

nacho commented Sep 5, 2018

Uh oh!

edmonds commented Sep 5, 2018

Uh oh!

nacho commented Jun 11, 2019

Uh oh!

edmonds commented Jun 17, 2019

Uh oh!

nacho commented Sep 25, 2019

Uh oh!

nacho commented Feb 9, 2020

Uh oh!

edmonds commented Feb 13, 2020

Uh oh!

morrisonlevi commented Apr 13, 2021

Uh oh!

lipnitsk commented Apr 13, 2021

Uh oh!

lipnitsk commented Apr 13, 2021

Uh oh!

jin-eld commented May 20, 2021

Uh oh!

neheb commented Jun 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nacho commented Sep 5, 2018 •

edited

Loading