Initial support for .proto files

[kamahen]

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

[JanWielemaker]

This pull request has been mentioned on SWI-Prolog. There might be relevant details there:

https://swi-prolog.discourse.group/t/does-anyone-care-about-protobufs/3860/84

[JanWielemaker]

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?

[kamahen]

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]

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.

[kamahen]

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]

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.

[kamahen]

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

[kamahen]

I made a mistake: "git commit --amend" followed by "git push --force". I'll have to redo the entire PR, but in the meantime, the change is here: https://github.com/kamahen/contrib-protobufs/commit/15918eba1cfc8a710181193b20aa45ade54fcfc3 (adds a test for TEST_PROTOBUFS_PROTOC) and removes some obsolete stuff).

If you wish, I can start a delete this PR and start a new PR and we can continue discussing it there.

[JanWielemaker]

Unless you already redid the work, there is no problem. Just look in .git/logs for the log file of the right branch and find the forced push in the history. There is the hash that was discarded. checkout this hash (git checkout <hash>) and give it a branch (git branch <name>). Now you can do all the usual stuff with it, including deleting the broken branch and rename your temporary branch to it.

Unless you remove .git, you really cannot mess up. The GIT db is graph of hashes. No file is ever changed, so everything stays there. Only git gc reclaims objects, but by default only when they have not been accessed for some time (I think 2 weeks). You only need to find the hash. That is where the logs are for. If you really want you can even use git cat on the objects and grep through them.

[kamahen]

I've lost nothing; but I don't know how to get it into this PR, that's all.

This, as far as I know, is the latest version: https://github.com/kamahen/contrib-protobufs/commit/15918eba1cfc8a710181193b20aa45ade54fcfc3

[JanWielemaker]

Managed to merge it. Seems to build and test fine and doesn't complicate the whole thing too much. There are quite some TODO's, notably in the CMakeFile.txt. Did you plan to clean these and/or cleanup the history? Small other thing I found glancing the code is the duplication of discontiguous and multifile. AFAIK even the standard implies multifile implies discontiguous.

If you are happy with it as is, I'm happy to merge into master.

[kamahen]

I hope you didn't have to spend much time doing the merge after my github mistake.

Thanks for pointing out the TODOs -- I'll review them and do a cleanup. (One of my bad habits is making a number of simultaneous changes, e.g., when I notice a bit of refactoring while doing something else, and copious TODOs are my way of slightly controlling the chaos.)

I can do the "git push" to the main repository (I don't think that there are any comments in this PR worth preserving for posterity -- I'll add a few comments during my cleanup to summarize) ... but before I do the push, please add the following to swipl-devel/CMakeLists.txt:

option(TEST_PROTOBUFS_PROTOC
       "Run protobuf tests that require protoc, etc."
       OFF)

[JanWielemaker]

Added. Just go ahead (any notify me).