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

Cannot set --experimental_allow_proto3_optional options #234

Closed
xprezesx opened this issue Mar 18, 2021 · 11 comments
Closed

Cannot set --experimental_allow_proto3_optional options #234

xprezesx opened this issue Mar 18, 2021 · 11 comments

Comments

@xprezesx
Copy link

There is new option in protobuf 3 optional.
I can't find possibility to set this flag on protoc function

docker run -v pwd:/defs namely/protoc-all -l php -o library/ -i --experimental_allow_proto3_optional -d ./ 1 ↵ jacekbednarek@Jacek-Shoplo-Macbook-2019 --experimental_allow_proto3_optional: warning: directory does not exist. catalog/product/domain/product.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

@ido-namely
Copy link
Contributor

Hi @xprezesx,
please note that the docker image entrypoint does not expose protoc command line directly.
as a workaround I suppose you can override the entrypoint to protoc and invoke it however way you want.
Otherwise this option will need to be added as a flag to the current entrypoint script.

@wespen
Copy link
Contributor

wespen commented Mar 30, 2021

I can add it in the new MR tomorrow.
Already playing with entrypoint vars.

@wespen
Copy link
Contributor

wespen commented Mar 31, 2021

Ok, took a deeper look and the problem is that a lot of generators still don't support the optional flag.

I re-test when generators include it.

@wespen
Copy link
Contributor

wespen commented Apr 29, 2021

Once all packages support it Ill do MR.
(Opened up new issue in all affected repos)

Wating for proto3 optional support in packages:
protoc-gen-validate (bufbuild/protoc-gen-validate#431)
protoc-gen-micro (micro/micro#1766)
protoc-gen-rbi (coinbase/protoc-gen-rbi#24)
protoc-gen-gogofast (gogo/protobuf#713)

@ido-namely
Copy link
Contributor

looks like it is now supported by most of these except gogo.
Is the original issue still relevant though @xprezesx , @wespen ?

@esparzak
Copy link

Hi @ido-namely,
Still relevant here

@esparzak
Copy link

esparzak commented Aug 3, 2022

Hi @ido-namely, are you still looking at implementing this?

@ido-namely
Copy link
Contributor

Hey @esparzak,
I've looked into this further, It seems like the flag is already enabled by default since protobuf v3.15.0 and by now this project uses protobuf v3.19.4.

Are you sure the optional option is not enabled for you?
Also, we are using optional in one of our tests.

@ido-namely
Copy link
Contributor

Hey @xprezesx ,
Would you agree this issue was resolved?

@ido-namely
Copy link
Contributor

Hi, unless anyone objects, I will consider this issue resolved and close this at the end of this week.
Thanks.

@ido-namely
Copy link
Contributor

As mentioned in previous comments, the flag in question seems to already enabled by default on the version of protoc that is used in this repo, so I'm closing this issue.

Feel free to reopen or further discuss in case you disagree.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants