GobySoft / dccl

Dynamic Compact Control Language
Other
17 stars 13 forks source link

Add parameter to dccl protoc plugin to allow generation of load file for processed protos #106

Closed tsaubergine closed 1 year ago

tsaubergine commented 1 year ago

Overview

The DCCL protoc plugin (protoc-gen-dccl) now optionally accepts a parameter dccl3_load_file that provides a path to the desired output of a .cpp file which will include the required code to load and unload all DCCL messages compiled with this parameter using the existing C-functions dccl3_load and dccl3_unload.

If the file already exists protoc-gen-dccl will not regenerate it, but rather add any new loaders that are missing to the end of the file. This allows the option of this file being part of the (version-controlled) source code and other custom manual actions performed in the dccl3_load and dccl3_unload functions (such as FieldCodecManager add and remove functions).

Closes #103

Usage

Set protoc to use the DCCL plugin (protoc-gen-dccl) and add the dccl3_load_file parameter to the end

BUILD_INCLUDE="/path/to/my/build/include"
PROTO_SRC_BASE="/path/to/my/src/"
PROTOC_GEN_DCCL="/path/to/protoc-gen-dccl"
protoc --cpp_out ${BUILD_INCLUDE} ${PROTO_SRC_BASE}/my.proto --dccl_out=dccl3_load_file=/path/to/my/desired/dccl_load.cpp:${BUILD_INCLUDE} --plugin ${PROTOC_GEN_DCCL}

protoc can be run with multiple protos in a single call or multiple times with the same dccl3_load_file and they will all be appended to the dccl3_load_file given.

At this point you can build a shared library (e.g. libmy_protos.so) as desired with the your my.pb.cc and dccl_load.cpp file and then you can load it with:

dccl::Codec codec;
codec.load_library("libmy_protos.so");

Example Generated File

The dccl3_load_file that is generated by new unit test dccl_load_file:

#include <dccl/codec.h>

// DO NOT REMOVE: required by loader code
std::vector<const google::protobuf::Descriptor*> descriptors;
template<typename PB> struct DCCLLoader { DCCLLoader() { descriptors.push_back(PB::descriptor()); }};
// END: required by loader code

extern "C"
{
    void dccl3_load(dccl::Codec* dccl)
    {
        // DO NOT REMOVE: required by loader code
        for(auto d : descriptors)
            dccl->load(d);
        // END: required by loader code
    }

    void dccl3_unload(dccl::Codec* dccl)
    {
        // DO NOT REMOVE: required by loader code
        for(auto d : descriptors)
            dccl->unload(d);
        // END: required by loader code
    }
}

// BEGIN (TO END OF FILE): AUTOGENERATED LOADERS
#include "dccl/test/dccl_load_file/test2.pb.h"
DCCLLoader<dccl::test::TestMessage3> dccl_test_testmessage3_loader;
#include "dccl/test/dccl_load_file/test1.pb.h"
DCCLLoader<dccl::test::TestMessage1> dccl_test_testmessage1_loader;
DCCLLoader<dccl::test::TestMessage2> dccl_test_testmessage2_loader;
psmskelton commented 1 year ago

Thanks for this @tsaubergine! Certainly simplifies some things. However, there are 2 issues that I'm noticing.

Issue 1

I'm missing some DCCLLoader<> entries from each .proto input when there are messages with similar names.

Given the input files:

bits.proto : 50 message definitions
pieces.proto : 11 message definitions

The generated dccl3_load_file file only has:

#include bits.pb.h
DCCLLoader<...> * 49 (missing 1)

#include pieces.pb.h
DCCLLoader<...> * 9 (missing 2)

Manually checking both the CPP and Python output files generated by protoc shows protoc generated the correct number of classes:

# grep -c "@@protoc_insertion_point(class_definition:" bits.pb.h
# 50
# grep -c "_reflection.GeneratedProtocolMessageType("  bits_pb2.py
# 50
# grep -c "@@protoc_insertion_point(class_definition:" pieces.pb.h
# 11
# grep -c "_reflection.GeneratedProtocolMessageType("  pieces_pb2.py
# 11

One message that is persistently not generating a DCCLLoader entry is (grossly simplified):

message Waypoint
{
  option (dccl.msg).codec_version = 4;
  option (dccl.msg).id = 175;
  option (dccl.msg).max_bytes = 2;

  required uint32 testing = 1
  [
    (dccl.field).min = 0,
    (dccl.field).max = 1000000,
    (dccl.field).resolution = 1000
  ];
}

Which is message 47 of 50 in the file (by ID and placement), so it's not EOF problems. Messages that build upon this message, for example:

message WaypointTask
{
  option (dccl.msg).codec_version = 4;
  option (dccl.msg).id = 173;
  option (dccl.msg).max_bytes = 2;

  repeated Waypoint tasks = 1
  [
    (dccl.field).min_repeats = 1,
    (dccl.field).max_repeats = 5
  ];
}

Are compiling fine. If I rename bits.proto ID 175 from Waypoint to Testing, it now generates DCCLLoader<> correctly. The same pattern emerges with pieces.proto, where a message with name VehicleStatus has no DCCLLoader<> entry generated when a message with name VehicleStatusRequest is also present.

Issue 2

It is now difficult to identify non-compliant messages as the exception thrown does not state what descriptor it has a problem with. For example, in a Python development workflow, no further traceback is available beyond:

  File "/usr/local/lib/python3.8/dist-packages/<our_package>/__init__.py", line 18, in get_codec
    codec.load_library("/usr/local/lib/lib<our_library>.so")
dccl.DcclException: Actual maximum size of message exceeds allowed maximum (dccl.max_bytes). Tighten bounds, remove fields, improve codecs, or increase the allowed dccl.max_bytes

Suggested change is for all Exceptions to include the descriptor.full_name in their messages, e.g.:

dccl.DcclException: Actual maximum size of '<descriptor.full_name>' exceeds allowed maximum (dccl.max_bytes). Tighten bounds, remove fields, improve codecs, or increase the allowed dccl.max_bytes
tsaubergine commented 1 year ago

Thanks for your feedback, it's very helpful!

Issue 1: Stupid bug on my part, fixed now (protoc-gen-dccl searches for the message loader to avoid adding it multiple times, but this would falsely pick up messages that were a substring of an existing message. Avoided by adding "<" and ">" to the search string to pick up the complete template) Issue 2: Good point, I've added it now so Exception messages will look like:

Message: dccl.test.TestMsg: Actual maximum size of message (265B) exceeds allowed maximum (1B). Tighten bounds, remove fields, improve codecs, or increase the allowed value in (dccl.msg).max_bytes

where "Message: dccl.test.TestMsg" will be added whenever the descriptor is known.

tsaubergine commented 1 year ago

@philboske Let me know if this works OK for you now and I'll merge it in (will be part of release 4.1.0)

psmskelton commented 1 year ago

@tsaubergine Seems to be functioning correctly for us. Thanks again.