Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

protoc error: Explicit 'optional' labels are disallowed in the Proto3 syntax #482

Open
duskmoon314 opened this issue May 11, 2024 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@duskmoon314
Copy link
Contributor

I'm trying to add scripts to generate rust code and encountered this error.

In proto/p4/config/v1/p4info.proto, an optional is used at L314:

312 message SumOfMembers {
313   // the maximum weight of each individual member in a group.
314>  optional int32 max_member_weight = 1;
315 }

When generating codes with protoc, an error occurs (no matter what language is targeted):

# exec protoc via compile_protos.sh with protoc v3.12.4
p4/config/v1/p4info.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

# exec protoc via update.sh
p4/config/v1/p4info.proto:314:14: Explicit 'optional' labels are disallowed in the Proto3 syntax. To define 'optional' fields in Proto3, simply remove the 'optional' label, as fields are 'optional' by default.
p4/v1/p4runtime.proto: Import "p4/config/v1/p4info.proto" was not found or had errors.
p4/v1/p4runtime.proto:735:3: "config.v1.P4Info" is not defined.

I'm not very familiar with protobuf. Should this optional be removed, or should the flag --experimental_allow_proto3_optional be used after all protoc usage?

@smolkaj
Copy link
Member

smolkaj commented May 13, 2024

@jonathan-dilorenzo, I believe you recently added this field. I suspect the optional qualifier may have slipped in by acident?

Thanks for reporting the issue, @duskmoon314.

@antoninbas
Copy link
Member

optional support was added in proto 3.15
I think we discussed this at the time and decided to keep it.
@duskmoon314 is using protoc v3.12.4 which is very old.
Even the "p4lang toolchain", which is not updated frequently, is using v3.18

@jonathan-dilorenzo
Copy link
Contributor

Nonetheless, good question. No idea why I put the optional qualifier in there. Let me take an AI to get rid of it (and add a comment about when the field was introduced at the same time).

@duskmoon314
Copy link
Contributor Author

duskmoon314 commented May 13, 2024

@duskmoon314 is using protoc v3.12.4 which is very old.

This one is installed via apt, and it is v3.12.4 on Ubuntu 22.04.

But I also see the error when using update.sh. It uses the image p4lang/third-party. There may still be some version issues that need to be fixed.


Oh, I didn't notice that the last build of that image was two years ago. Should the image be updated, or should I use Bazel to build the proto instead of codegen/?

@antoninbas
Copy link
Member

But I also see the error when using update.sh. It uses the image p4lang/third-party. There may still be some version issues that need to be fixed.

That sounds unlikely. The script is run every time the main branch is updated. This is the latest run, from last week: https://github.com/p4lang/p4runtime/actions/runs/8991480949

Oh, I didn't notice that the last build of that image was two years ago.

This is accurate, but it still comes with protoc v3.18.1:

$ docker run -ti p4lang/third-party:latest protoc --version
libprotoc 3.18.1

It's unlikely anyone is going to update this image unless it is required (e.g., for some CI purposes).

@duskmoon314
Copy link
Contributor Author

duskmoon314 commented May 14, 2024

Em, I think I got the wrong image. I need to check what's wrong with docker.

Here is the output when I run p4lang/third-party on my machine:

❯ docker pull p4lang/third-party
Using default tag: latest
latest: Pulling from p4lang/third-party
4f53fa4d2cf0: Already exists 
6af7c939e38e: Already exists 
903d0ffd64f6: Already exists 
04feeed388b7: Already exists 
fda0447a2aef: Already exists 
d542ede27a5b: Already exists 
b51b1233ed05: Already exists 
c82aee7e3df0: Already exists 
dfbd36aecb5f: Already exists 
e4a0a292616b: Already exists 
2a545cc6126f: Already exists 
fddeaa06596f: Already exists 
f9b99e89d373: Already exists 
4e7b4af9333f: Already exists 
a87f1c53893b: Already exists 
11b7960c4654: Already exists 
846e5f482b63: Already exists 
5c787c58e206: Already exists 
34599a8d9776: Already exists 
a6c7643c8f08: Already exists 
Digest: sha256:a0f2e9a741bab06cc8dd478183541c1260ca4e174d954a21af3eb7e63559dd8b
Status: Downloaded newer image for p4lang/third-party:latest
docker.io/p4lang/third-party:latest

❯ docker run -it --rm p4lang/third-party bash
root@aee361934b1f:/# protoc --version
libprotoc 3.6.1

For some unknown reason, the image I got via docker pull p4lang/third-party is one built four years ago. Once I explicitly pulled the latest one via digest, I got the correct one with protoc v3.18.1.

@antoninbas
Copy link
Member

This is not the correct sha for the current p4lang/third-party:latest image.
The correct one is sha256:9c5f3218965dc63c0a03023721a44ea9920067de0ff8a2c69a2d8baac34b1818, as per https://hub.docker.com/repository/docker/p4lang/third-party/general

I am able to pull the same image as you if I provide the same sha explicilty: docker image pull p4lang/third-party@sha256:a0f2e9a741bab06cc8dd478183541c1260ca4e174d954a21af3eb7e63559dd8b. It must be an older version of the image that is still present in the Docker registry, but without a tag.

You could try pulling with the correct sha.

@antoninbas
Copy link
Member

Looks like I replied just as you edited the post. Not sure why you are seeing a stale image by default when you use latest... Does your employer / university use a proxy cache for docker access?

@duskmoon314
Copy link
Contributor Author

Does your employer / university use a proxy cache for docker access?

I'm using the machine of our lab and the manager added a register-mirror several month ago before granting access to me. I delete that config and can pull the correct image now.

I'm testing update.sh now. Hopes everything works fine.

@duskmoon314
Copy link
Contributor Author

Thanks for your help. No error occurs now!

I assume the use of this keyword in intentional and close this issue for now.

@smolkaj
Copy link
Member

smolkaj commented May 14, 2024

Let's keep this open until we have also resolved #482 (comment). The optional keyword was not intentional here.

@smolkaj smolkaj reopened this May 14, 2024
@smolkaj smolkaj added the bug Something isn't working label May 14, 2024
@smolkaj
Copy link
Member

smolkaj commented Jun 14, 2024

@jonathan-dilorenzo, would you be able to send a quick follow-up so we can close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants