Skip to content

proto: add curl fallback and validate downloaded status.proto#622

Open
Devansh-567 wants to merge 1 commit into
p4lang:mainfrom
Devansh-567:proto/update-google-rpc-status-fallback
Open

proto: add curl fallback and validate downloaded status.proto#622
Devansh-567 wants to merge 1 commit into
p4lang:mainfrom
Devansh-567:proto/update-google-rpc-status-fallback

Conversation

@Devansh-567

Copy link
Copy Markdown
Contributor

Summary

Improve proto/update_google_rpc_status.sh by making the download process more robust.

Changes

  • Prefer curl when available and fall back to wget
  • Exit with a clear error if neither download tool is installed
  • Download to a temporary file before replacing the checked-in status.proto
  • Add a basic sanity check to verify the downloaded file is a valid .proto before overwriting the existing file

This helps prevent invalid downloads from replacing status.proto and aligns the script with the download fallback pattern used elsewhere in the repository.

Signed-off-by: Devansh-567 <devansh.jay.singh@gmail.com>
@matthewtlam

Copy link
Copy Markdown
Contributor

Could you explain why curl over wget is more robust?

@chrispsommers

Copy link
Copy Markdown
Collaborator

What motivated this PR, did you experience real failures or is this just trying to incorporate best practices?

@Devansh-567

Copy link
Copy Markdown
Contributor Author

Could you explain why curl over wget is more robust?

Thanks for the question! To clarify, the change isn't about curl being inherently more robust than wget, both are reliable tools. The intent is: curl is preferred first because it's already the more commonly available tool in minimal CI/container images (and is used elsewhere in this repo's scripts), so preferring it keeps behavior consistent across the codebase. wget is kept as a fallback so the script still works on systems where only wget is installed. If neither tool is present, the script now fails fast with a clear error instead of silently proceeding or failing with a confusing message later on.

So this is less "curl > wget" and more "support both, prefer the one used elsewhere in the repo, and fail clearly if neither exists."

@Devansh-567

Copy link
Copy Markdown
Contributor Author

What motivated this PR, did you experience real failures or is this just trying to incorporate best practices?

@chrispsommers, this wasn't prompted by a specific failure I hit personally. It's a defensive/best-practices improvement:

The original script assumed curl was always present, which isn't guaranteed on all dev/CI environments. It also overwrote status.proto directly during download, so a partial/failed download (e.g., network hiccup, HTTP error page instead of the proto) could silently corrupt the checked-in file. This PR downloads to a temp file first and does a basic sanity check (that it looks like a valid .proto file) before replacing the real file, so a bad download fails loudly instead of quietly breaking the repo. I don't have a specific incident to point to, but this pattern (fallback + validate-before-replace) is already used elsewhere in the repo, so this brings update_google_rpc_status.sh in line with that convention. Happy to add more detail to the PR description if that's helpful for review. 😊

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

Successfully merging this pull request may close these issues.

3 participants