AOSPAlliance / android-prepare-vendor

Set of scripts to automate AOSP compatible vendor blobs generation from factory images
25 stars 8 forks source link

Add source .proto file for update_metadata_pb2.py #59

Closed nickrbogdanov closed 3 years ago

nickrbogdanov commented 3 years ago

_pb2.py files are autogenerated by the protobuf compiler. Sometimes the format changes in a way that is incompatible with the python-protobuf runtime, so it is unwise to maintain just* the compiled version in our tree. Add the corresponding source file and a Makefile (borrowed from carriersettings-extractor) to let the user easily regenerate it from source.

I found update_metadata.proto in this repo: https://android.googlesource.com/platform/system/update_engine

By comparing the compiled output I determined that our current version of update_metadata_pb2.py contains the deprecation flags introduced in upstream commit 0f59a9a41177, and it does not contain the unpadded_signature_size field added in the subsequent commit (7bbe015a1bd1c). So I synced to 0f59a9a41177.

Fixes AOSPAlliance/android-prepare-vendor#58.

dan-v commented 3 years ago

@nickrbogdanov - thanks for this contribution! What version of protobuf did you have installed when you were bumping into #58? It would be nice if we could at least specify what version(s) of python-protobuf are compatible with the currently generated update_metadata_pb2.py.

Not necessary for this PR, but it also might be nice to add a Dockerfile with a pinned version of protobuf that could be used to regenerate these files, as the generated output appears to vary based on the version of protoc.

nickrbogdanov commented 3 years ago

I have version 3.0.0-9.1ubuntu1 and it is not compatible with the current version of update_metadata_pb2.py.

I think that if we're willing to require users to install an external dependency (protobuf compiler), any modern version of the compiler + runtime should be fine as long as the generated code is compatible with the installed python library. Maybe part of the problem here is that python-protobuf is a dependency but it isn't mentioned in the README? We could just ask users to install both packages and then regenerate from the .proto file every time.

dan-v commented 3 years ago

Thanks for the additional details. @nickrbogdanov.

Maybe part of the problem here is that python-protobuf is a dependency but it isn't mentioned in the README

Agreed. I think it makes sense to call out these dependencies and probably makes sense to regenerate from proto file as part of the normal script execution. I wouldn't want to force generation of these python files without it being nicely integrated, as that changes the workflow to something like "first run make ...".

Anyways, for now, the changes you have made at least allow people to regenerate which is a first good step. I'm going to approve and wait for one other person to stamp this before merge.

dan-v commented 3 years ago

@chirayudesai this could use a look when you get a chance

chirayudesai commented 3 years ago

This looks fine. Re-generating via the makefile gets rid of the python2 compat present in the files currently but that's fine given that python2 has been deprecated anyway.

Thanks!