spacetelescope / catkit2

Package for controlling testbed hardware.
https://spacetelescope.github.io/catkit2/
BSD 3-Clause "New" or "Revised" License
5 stars 0 forks source link

Protobuffer warnings and weirdness #28

Open ehpor opened 2 years ago

ehpor commented 2 years ago

With #3 being merged (soon), there were a few minor issues that came up during development.

Compilation options

I had to set the following options in CMake:

set_property(TARGET catkit_core PROPERTY POSITION_INDEPENDENT_CODE ON)
target_compile_definitions(catkit_core PUBLIC PROTOBUF_USE_DLLS)

if (MSVC)
  # disable warning: 'identifier': class 'type' needs to have dll-interface to be used by clients of class 'type2'
  target_compile_options(catkit_core PUBLIC /wd4251)
endif()

The first two lines are expected and correspond to the recommendations when compiling a DLL. However, I don't think this should be necessary, since I'm not directly exposing any protobuffers to pybind, they are just in the DLL, being used internally. This might be intended.

Even with this, I still get many warnings on Windows, that I ignored with the last four lines. I'm minorly concerned that these warnings are signifying something worse, but so far I haven't seen anything bad.

Directory structure

Currently, I have a

sys.path.append(os.path.dirname(os.path.realpath(__file__)))

inside the catkit2/proto/__init__.py file. Protobuffers for Python are compiled with absolute references. This has to do with the folder structure in which Google compiles protobuffers. I get how to do that with a Python-only project, since the protobuffers then can be part of the Python directory structure, and so the compiled Python version will be next to the .proto file. However, when you have a mixed language project, you'd want the proto files in a separate directory from the Python files, since the proto definitions do not "belong" to the Python but also to the C++ part. In that case, absolute imports are wrong. I could either copy over the proto files into the Python directory structure before compiling them, or put in a cludge for "allowing absolute imports as relative imports".

I'm thinking that I'm misunderstanding something fundamental with how proto files are used and compiled by Google, since they obviously have a multi-language project as well.

raphaelpclt commented 4 weeks ago

Very minor, highly involved.