BitBoxSwiss / bitbox02-firmware

Firmware code of the BitBox02 hardware wallet
https://bitbox.swiss/bitbox02
Apache License 2.0
256 stars 91 forks source link

py/bitbox02/communication: update protobuf generated files #951

Closed Beerosagos closed 2 years ago

Beerosagos commented 2 years ago

Recent HWI update requires protobuf files generated with protoc => 3.19.0. This update regenerates the files with protoc 3.21.1

EDIT: as @benma pointed out, the issue is not actually related to HWI, which currently requires protobuf 3.19.1. The compatibility issue is between protobuf >= 3.21 and current protobuf files.

To solve it protobuf files has been regenerated with protoc >= 3.19 and protobuf dependency of bitbox02 py lib set to >= 3.21

benma commented 2 years ago

The protobuf files are generated by executing cd py && make (inside Docker), and this should continue working so it's easy to generate the files when changing the protobuf messages.

Please update the Dockerfile accordingly :)

Beerosagos commented 2 years ago

The protobuf files are generated by executing cd py && make (inside Docker), and this should continue working so it's easy to generate the files when changing the protobuf messages.

Please update the Dockerfile accordingly :)

Didn't think about that! :| Now should be ok, thanks.

benma commented 2 years ago

Recent HWI update requires protobuf files generated with protoc

Are you sure it's HWI? The issue originally was this:

f this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.

So it was about the protobuf package being newer than 3.20. HWI's dep list uses 3.19.1. So maybe it was something else that upgraded your protobuf package :thinking:

Anyway, would be good to include the above in the commit msg :)

benma commented 2 years ago

We have a problem maybe. When I now run send_message.py, I get:

from google.protobuf.internal import builder as _builder

My local protobuf installation is at 3.19.4:

image

Upgrading it fixes it, but that still means it's broken for protobuf <3.20.

At the very least we need to then bump https://github.com/digitalbitbox/bitbox02-firmware/blob/57d62a98cf52fd4d700551083e925019e5eb16f3/py/bitbox02/setup.py#L78. Maybe that's sufficient :shrug:

benma commented 2 years ago

Please also update py/bitbox02/CHANGELOG.md under 6.1.0 (to be released).

Beerosagos commented 2 years ago

We have a problem maybe. When I now run send_message.py, I get:

from google.protobuf.internal import builder as _builder

My local protobuf installation is at 3.19.4:

image

Upgrading it fixes it, but that still means it's broken for protobuf <3.20.

At the very least we need to then bump

https://github.com/digitalbitbox/bitbox02-firmware/blob/57d62a98cf52fd4d700551083e925019e5eb16f3/py/bitbox02/setup.py#L78 . Maybe that's sufficient shrug

Thanks for pointing this out, didn't notice it! Yes, updating setup.py seems to be sufficient, and a good way to avoid compatibility issues.

Beerosagos commented 2 years ago

Recent HWI update requires protobuf files generated with protoc

Are you sure it's HWI? The issue originally was this:

f this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.

So it was about the protobuf package being newer than 3.20. HWI's dep list uses 3.19.1. So maybe it was something else that upgraded your protobuf package thinking

Anyway, would be good to include the above in the commit msg :)

You are right, I was misled by the fact that both I and the user in https://github.com/digitalbitbox/bitbox02-firmware/issues/949 encountered the error while working on HWI.

Beerosagos commented 2 years ago

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

benma commented 2 years ago

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

I think pylint is just very bad with the member checks with protobuf, they are probably false positives. That's why you see so many # pylint: disable=no-member scattered :( I don't know why more of them are triggered now... Maybe there is a way to disable that warning altogether, otherwise try putting # pylint: disable=no-member and seeing if that helps.

You can lint it locally with ./scripts/docker_exec.sh ./scripts/lint-python.

benma commented 2 years ago

Fyi the member check by pylint is not so important because we also have types everywhere, and mypy does a full type check on the package, so mypy would catch any such member mistakes.

Beerosagos commented 2 years ago

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

I think pylint is just very bad with the member checks with protobuf, they are probably false positives. That's why you see so many # pylint: disable=no-member scattered :( I don't know why more of them are triggered now... Maybe there is a way to disable that warning altogether, otherwise try putting # pylint: disable=no-member and seeing if that helps.

You can lint it locally with ./scripts/docker_exec.sh ./scripts/lint-python.

Thanks for the explanation. I disabled no-member checks for protobuf generated modules.