flowey: add --custom-protoc to build-igvm CLI#2620
Merged
justus-camp-microsoft merged 1 commit intomicrosoft:mainfrom Jan 8, 2026
Merged
flowey: add --custom-protoc to build-igvm CLI#2620justus-camp-microsoft merged 1 commit intomicrosoft:mainfrom
justus-camp-microsoft merged 1 commit intomicrosoft:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds the --custom-protoc CLI argument to the build-igvm pipeline, allowing users to override the protoc path with a local path. This mirrors the existing --custom-openvmm-deps functionality added in #2388. The PR refactors download_protoc to resolve_protoc to support both downloading and using local paths.
Key changes:
- Renamed
download_protocmodule toresolve_protocto better reflect its dual capability - Added new
Request::LocalPathvariant to support custom protoc paths - Updated
cfg_versionsto acceptLocalOpenvmmDepsandLocalProtocrequests independently - Added
is_executable()helper method to check file executability
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
flowey/flowey_lib_common/src/resolve_protoc.rs |
Refactored from download_protoc to support both downloading and local paths; added validation logic |
flowey/flowey_lib_hvlite/src/_jobs/cfg_versions.rs |
Updated to support independent local overrides for openvmm_deps and protoc |
flowey/flowey_hvlite/src/pipelines/build_igvm.rs |
Added --custom-protoc CLI argument that works with --use-local-deps |
flowey/flowey/src/lib.rs |
Added is_executable() helper to check file permissions on Unix systems |
flowey/flowey_lib_common/src/lib.rs |
Updated module export from download_protoc to resolve_protoc |
flowey/flowey_lib_hvlite/src/init_openvmm_magicpath_protoc.rs |
Updated import to use resolve_protoc |
flowey/flowey_lib_hvlite/src/_jobs/cfg_common.rs |
Updated import to use resolve_protoc |
flowey/flowey_hvlite/src/pipelines/*.rs |
Updated all pipelines to use Request::Init instead of Request::Download |
.github/workflows/*.yaml |
Auto-generated workflow files updated with module rename |
ci-flowey/*.yaml |
Auto-generated pipeline files updated with module rename |
Comments suppressed due to low confidence (3)
flowey/flowey_lib_common/src/resolve_protoc.rs:113
- The comment states "If a local path is specified, assume protoc is already executable." However, due to the bug in resolve_protoc_from_dir where make_executable() is always called on line 38, this assumption is violated. When make_executable is false, the function will still attempt to make the file executable if it's already executable. This comment needs to be updated after fixing the bug in resolve_protoc_from_dir.
flowey/flowey_lib_common/src/resolve_protoc.rs:38 - The logic in this function has a flaw: when make_executable is false and the file is not executable, it bails with an error. However, on line 38, make_executable() is called unconditionally regardless of the make_executable parameter value. This means:
- If make_executable is false and the file is not executable, it will bail on line 32 before reaching line 38
- If make_executable is false and the file is executable, it will still try to make it executable on line 38 (unnecessary but harmless)
- The make_executable parameter essentially becomes meaningless since line 38 always tries to make it executable when it can be reached
The logic should be: only call make_executable() on line 38 if the make_executable parameter is true. The current implementation makes the make_executable parameter misleading.
flowey/flowey_lib_common/src/resolve_protoc.rs:57
- The documentation comment says "Use a locally downloaded protoc" but the parameter name is LocalPath and it's used for custom/local paths that users specify, not necessarily "downloaded" copies. The comment should say "Use a local protoc from the specified path" or "Use a custom protoc path" to be more accurate.
a1e1e05 to
34dc04e
Compare
chris-oo
previously approved these changes
Jan 8, 2026
34dc04e to
6ee4ccb
Compare
chris-oo
approved these changes
Jan 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similar to #2388, adds the ability to override the protoc path with a local path. This should work independently of the other arguments but also with them. Tested all combinations of arguments specified and all work as expected.
This is another step towards reproducible builds where we're adding the capability to use "local" versions of everything that will eventually be supplied by nix.