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

Fixes issue where protoc did not recognize plugin.py as win32 application #38

Closed

Conversation

boukeversteegh
Copy link
Collaborator

Hi~

Thanks for this awesome library!

There's a bug however that I want to look into, but I couldn't run generate on my windows system. This PR fixes that.

When running pyenv run generate on windows, protoc does not accept ../plugin.py as an executable win32 application.

See for example: protocolbuffers/protobuf#3923

This PR adds a .bat file that executes python, which is used when running the generate.py on windows.

@cetanu cetanu self-assigned this May 21, 2020
@cetanu cetanu added the bug Something isn't working label May 21, 2020
@nat-n
Copy link
Collaborator

nat-n commented May 22, 2020

Out of curiosity (since I know very little about windows):

I take it the issue the .bat file solves is windows not understanding the shebang?

Am I correct in thinking this only matters if you're using the --plugin option on the protoc CLI, since if you use the --python_betterproto_out option as in the readme it'll pick up the protoc-gen-python_betterproto console script?

@boukeversteegh
Copy link
Collaborator Author

boukeversteegh commented May 22, 2020

Out of curiosity (since I know very little about windows):

I take it the issue the .bat file solves is windows not understanding the shebang?

Am I correct in thinking this only matters if you're using the --plugin option on the protoc CLI, since if you use the --python_betterproto_out option as in the readme it'll pick up the protoc-gen-python_betterproto console script?

Yes, this is only needed for development. Since I need this change for every other PR I'm working on, and my next PR also includes it with a refactor, we could drop this PR in favor of: #51

@boukeversteegh
Copy link
Collaborator Author

Closing in favor of #51

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

Successfully merging this pull request may close these issues.

None yet

3 participants