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

scripts: fixes/improvements to regenerate.sh / install_protoc.sh #7400

Closed
dfawley opened this issue Jul 10, 2024 · 0 comments · Fixed by #7409
Closed

scripts: fixes/improvements to regenerate.sh / install_protoc.sh #7400

dfawley opened this issue Jul 10, 2024 · 0 comments · Fixed by #7409
Assignees

Comments

@dfawley
Copy link
Member

dfawley commented Jul 10, 2024

There's some weird interactions going on, and some things I might like to see changed:

  • install_protoc.sh checks the protoc version in your path before downloading, and refuses to download if your path's protoc is a different version than you'd like to install
  • A function called download_binary shouldn't do other checks; those checks should be moved elsewhere.
  • This line makes no sense:
    INSTALL_PATH=${1:+"$1"}
  • This line needs quotes:
    source ./scripts/install_protoc.sh $WORKDIR
  • This line probably needs export in case a GOBIN isn't already exported:
    GOBIN="${WORKDIR}"/bin
    (go install needs to be able to read it.)
  • regenerate.sh calls install_protoc.sh after several other things have been done.
  • regenerate.sh apparently tries to "save your $PATH", but since it shouldn't be executed via source there's no need to do any of this.
  • Super nit-picky: install_protoc.sh sources vet_common.sh but is not a vet* script. Maybe rename vet_common.sh to common.sh?

IMO one of three models for these scripts seems pretty reasonable:

  1. Always download everything into a temp directory.
  • Downside: needs to download/recompile every time regenerate is run
  1. Check the versions in your path and print help/commands to update them to the needed versions.
  • Downside: it's hard to know where the user would want protoc to live.
  1. Carve out a directory like $HOME/grpc-go-tools or $HOME/tmp/grpc-go-tools or something and download the tools there if the versions don't match.
  • Downside: you're potentially polluting the user's directory.

Of these, I think I prefer (2) but am not strongly opposed to any of them. It looks like the scripts were already trying to implement (1), which is okay but is my least favorite option.

cc @arvindbr8 @aranjans

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

Successfully merging a pull request may close this issue.

2 participants