feat(internal/librarian): add protoc installation support#6583
feat(internal/librarian): add protoc installation support#6583JoeWang1127 wants to merge 23 commits into
protoc installation support#6583Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for downloading and installing the protoc compiler. It adds a new Protoc configuration struct, a ZIP extraction utility with Zip Slip protection, and the core installation logic along with corresponding tests. The review feedback focuses on improving cross-platform compatibility, suggesting that the download URL dynamically determine the platform suffix based on the host OS and architecture, that the path filtering handle the .exe extension on Windows using forward slashes, and that the tests mock the platform suffix to remain deterministic.
JoeWang1127
changed the title
protoc to librarian bin directoryprotoc installation support
|
|
||
| var ( | ||
| // downloadURL returns the download URL for the protoc binary for the given version and platform. | ||
| downloadURL = func(version string) string { |
There was a problem hiding this comment.
Maybe let's make the base (https://github.com) a global var that we can replace with the test server, and make this an actual function that we can test coverage for.
That would be cleaner code all around.
| var ( | ||
| // downloadURL returns the download URL for the protoc binary for the given version and platform. | ||
| downloadURL = func(version string) string { | ||
| return fmt.Sprintf("https://github.com/protocolbuffers/protobuf/releases/download/v%s/protoc-%s-%s.zip", version, version, platformSuffix()) |
There was a problem hiding this comment.
embedding execution of platformSuffix may make this a little harder to test on various platforms. Maybe we should have a platform string parameter that tests can just provide, rather than have the potential point of variance introduced by running this on different dev / CI machines.
| return filepath.Join(binDir, "protoc", fmt.Sprintf("v%s", version)), nil | ||
| } | ||
|
|
||
| func platformSuffix() string { |
There was a problem hiding this comment.
Nit: please add a comment, all the other funcs have them :)
|
Stoked about this! I'm guessing a follow up will introduce the code changes to language packages to use the librarian installed protoc ? |
2 participants
Add the ability to install the protoc compiler from GitHub releases.
The install function downloads the protoc zip archive for the host platform, verifies its checksum, and extracts it to the librarian bin directory.
For #6558