setuptools: Fix passing empty include paths #418
Merged
Conversation
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
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue of passing empty or whitespace-only include paths to protoc by properly processing include paths passed via the command line or pyproject.toml.
- Update include_paths type annotation to accept both a string and an iterable of strings.
- Process include_paths using a pattern matching construct to remove extra white-spaces and empty strings.
- Update RELEASE_NOTES.md to document these changes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/frequenz/repo/config/setuptools/grpc_tools.py | Refactored include_paths handling to support both string and iterable inputs. |
| RELEASE_NOTES.md | Updated notes to reflect the fix in passing include paths to protoc. |
|
Sadly no tests because we still have no tests for this, as it is complicated because this is a setuptools plugin. I plan to move this to a separate repo in the future, which can make writing some integration test for it more easily. Manually tested with frequenz-floss/frequenz-api-common#374. |
tiyash-basu-frequenz
previously approved these changes
Jun 16, 2025
github-actions
bot
added
the
part:template
When loading protobuf include paths from the pyproject.toml file, the paths are received as an array/list. If we convert them to a comma-separated string there is the risk of a path containing commas to be split incorrectly, leading to compilation errors. Using the original array/list of paths instead of a comma-separated string is safer and more flexible. Signed-off-by: Leandro Lucarella <[email protected]>
When include paths are passed via the commnad-line, they are passed as a comma-separated string. This means the user could add extra spaces or pass a double-comma, which would result in empty strings in the include paths list, making the protobuf compiler fail. This commit cleans the include paths by stripping whitespace and ignoring empty strings. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
|
Rebased and fixed release notes conflicts, needs a new approval, but probably from the @frequenz-floss/python-sdk-team to be able to be queued for merging. |
Marenz
approved these changes
Jun 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fix wrong passing of include paths when passed via:
protoc -I.pyproject.toml: Now an empty array/list can be passed to override the default paths, before this resulted in an empty string being passed toprotoc -I.