SWI-Prolog / contrib-protobufs

An interface to Google Protocol Buffers (protobuf)
8 stars 7 forks source link

Serialize/parse using protoc #10

Closed kamahen closed 3 years ago

kamahen commented 3 years ago

This supersedes PR https://github.com/SWI-Prolog/contrib-protobufs/pull/6 , which got messed up due to a botched git commit --amend and a git push --force.

Main changes:

ENHANCED: handle repeated [packed=true] ADDED: protoc-gen-swipl ADDED: protobuf_parse_from_codes FIXED: handling of negative and large numbers ENHANCED: added unsigned32, unsigned64 types TEST: Added unittest*.proto and "golden" tests, edge case tests BUILD: Run developer and "demo" tests as part of cmake/ctest build ENHANCED: handle "group" ADDED: protobuf_parse_from_codes/3 (using output of protoc-gen-swipl plugin) ADDED: protobuf_serialize_to_codes/3 TESTING: added many unit tests

kamahen commented 3 years ago

Recording for posterity some comments in https://github.com/SWI-Prolog/contrib-protobufs/pull/6 (which got messed up with a "git push --force")

Summary: the main CMakeLists.txt added a TEST_PROTOBUFS_PROTOC flag (default: OFF); some deployment tests seem to be too difficult and need to instead be done after doing cmake install.

================= @JanWielemaker wrote (Jun 18, 2021):

I had a first look at the CMakeLists.txt. Seems you still struggle installing an executable script? I think I can deal with that.

The tests also need some consideration. You cannot call make. CMake is a meta build system and make is just one of its backends. (I also do not find a Makefile?) The way to add tests is to add a file test_xyz.pl that defines a module test_xyz with exports test_xyz/0 which should succeed. Then use in CMakeLists.txt test_libs(xyz). The macro takes multiple arguments, so if you have multiple test files just add them to a single test_libs() call.

If the test needs to run python, use Prolog process_create/3 to call it from Prolog.

I'm also wondering about the need for pkg-config. You do not seem to do anything with it?

================= (response)

The Makefiles are in demo/Makefile, interop/Makefile, bootstrap/Makefile.

demo/Makefie has been in existence since 2011. I added running it under ctest as a "smoke test", to ensure that the examples will work (demo/* are used as in "swipl_examples"). This is not an ideal way of running it ... I should probably run it in the build/home/doc/packages/examples/protobufs/demo directory ... I'll try fixing that (I suppose it doesn't matter if some generated files are left lying around in the examples directory if they're not mentioned in CMakeList.txt).

Directory bootstrap is also run as a "smoke test" - it would normally run as part of the development cycle, to generate two files that are used by the protoc plugin (and which are checked into git: protobufs/bootstrap/protoc_gen_prolog_pb/{plugin_pb.pl,descriptor_pb.pl}). (Normally I wouldn't check them in, but somethng is needed for the bootstrap, and it's easiest if I use the generated files rather than use my original files.)

As for interop/Makefile ... the "interop" directory is for verifying interoperability amongst Prolog, Python, and C++ (there's also a partial interoperability test in demo/Makefile, with the foo.cpp program that's been around for 11 years). I could rewrite this with cmake/Prolog, I suppose, but is there much value in doing that? Some of the rules invoke protoc (e.g., %_pb.pl: %.proto), for example, so I'd have to move those into cmake, etc. etc.

As for pkg-config ... it's a "soft" prereq for protoc, used to generate the appropriate C++ flags. As these are unlikely to change, I could hard-code them into the makefiles (actually in bootstrap/common.mk), but pkg-config makes things more likely to work on other platforms. For reference, these are the Make variables that are created using pkg-config:

CPPFLAGS = -pthread -I/home/peter/.local/include
LDFLAGS = -L/home/peter/.local/lib -lprotobuf -lpthread

where /home/peter/.local is where I've installed protoc (if it were installed from the Ubuntu PPA, it would be in /usr/lib, /usr/include, or similar).

================= @JanWielemaker wrote:

I see. Can't we leave it as is, and only have these tests run by the package maintainer? As is they are likely to be a maintenance hassle. Make doesn't need to be there, is sometimes called gmake, there is currently no dependency on C++ for most platforms, neither on protoc. pkg-config is not widely available either. Things like calling $(SWIPL) doesn't generally work. CMake has is rules to run targets in the build environment regardless of the platform. Portability is a challenge :) A compromise could be to make this part of the test conditional similar to the -DSKIP_SSL_TESTS=ON config, but by default disabling the tests, i.e. -DTEST_PROTOBUFS=ON

As is, the core serialization library is tested if I understand it correctly. As far as I'm concerned the tests only need to test that the Prolog library functions.

================= (response) I can check for the existence of make, g++, etc. also. ;)

As for the locations of files, those can be passed into make; I already do this with make SWIPL=${PROG_SWIPL} ... and that technique could be extended to other executables and files..

Shall I add a SKIP_PROTOBUF_SMOKE_TESTS flag? That would require coordinating a change in swipl/devel/CMakeLists.txt and contrib-protobufs -- if you make that change (picking whichever name you think is suitable), I'll do a "git pull" and make appropriate changes to protobufs/CMakeLists.txt.

(There would still be the existing test_protobufs.pl -- so SKIP_PROTOBUF_TESTS doesn't seem to be the correct name for the flag)

As for only testing the Prolog library functions ... some of my new code depends on running the protoc plugin, which generates "meta data facts" that describe the protobuf. So, I consider them to be part of the core tests. That's why I'm tempted to not have SKIP_PROTOBUF_SMOKE_TESTS but instead check for all the external programs and run the tests if they're all there (protoc, g++, python3, pkg-config, make ... maybe there are one or two others). An alternative is to generate the meta-data-facts as part of the development cycle and add them to git, but I don't like that much.

Question: what cmake incantation expands to home/doc/packages/examples/protobufs, so that my smoke test can verify that all the necessary files are present in the examples directories? I tried ${SWIPL_INSTALL_DOC}/packages/examples/${SWIPL_PKG}, but that produced lib/swipl/doc/packages/examples/protobufs, which of course didn't work.

================= @JanWielemaker wrote What about -DTEST_PROTOBUFS_PROTOC=ON? I don't see the point in SMOKE. All tests are smoke tests, no?

which generates "meta data facts" that describe the protobuf

What about including these files in the repo as you suggest at the end? I don't like generated files, but I guess they are pretty stable and I dislike all these dependencies a lot more. I've been at that square often enough and it is typically me who has to figure out how to deal with this stuff in the various scenarios cry

what cmake incantation expands to home/doc/packages/examples/protobufs

Nothing. The examples are directly installed from the sources to the final installation location. The SSL package is an example of a package with complicated test file management.

================= (response) I'm fine with adding the generated meta-data files to github. This could cause problems if anyone has read-only source directories (I had to deal with that when importing "third party" libraries into Google's Bazel-based build system ... not fun).

-DETEST_PROTOBUFS_PROTOC=ON seems good to me. As soon as you add this, I'll incorporate the flag into my CMakeLists.txt.

What's the estimated date of the next swipl release, so that I can get the changes done in time?

About home/doc/packages/... (vs lib/swipl/doc/packages/...) ... does this mean that there's no (easy) way to run a validation test in the examples, to ensure that all the files have been specified in the swipl_examples rule (it's easy to forget to update CMakeLists.txt when adding to the examples).