Conversation
Added function to install freertos-gdb into GDB tool packages if not already present.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Platform as Platform
participant Penv as penv_setup.install_freertos_gdb
participant UV as uv (pip)
participant Tools as GDB tool packages
participant FS as Filesystem/Logger
rect rgba(200,230,255,0.5)
Platform->>Penv: call install_freertos_gdb(platform, uv_exe, penv_exe, cache_dir)
end
rect rgba(220,255,200,0.5)
Penv->>Tools: enumerate installed GDB tool packages (GDB_TOOL_PACKAGES)
Penv->>UV: run "uv --pip install --target=... freertos-gdb" (use UV_CACHE_DIR if set)
UV->>Tools: install files into share/gdb/python/
UV-->>Penv: return success/failure
Penv->>FS: log install result or warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
builder/penv_setup.py (1)
682-685: Derive the GDB package list from shared platform metadata.This hardcoded list duplicates the GDB discovery logic already used in
platform.py. It works today, but any new*-gdbtool added there will load the Python hook without ever gettingfreertos-gdbinstalled here. Reusing a shared helper or filtering known platform packages would keep these paths in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 682 - 685, Replace the hardcoded gdb_tool_packages list with the shared platform package discovery used in platform.py so new "*-gdb" tools are picked up automatically; import and call the existing helper in platform.py that returns platform tool/package metadata (the same function used for GDB discovery) and filter that result for packages that end with "-gdb" (or otherwise match the platform's GDB tool predicate), then use that derived list where gdb_tool_packages is referenced and ensure "freertos-gdb" is still included if required by the existing installation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/penv_setup.py`:
- Around line 523-524: The call to install_freertos_gdb(...) is executed too
early and should be moved so it runs after the MCU toolchain GDB packages exist;
specifically, relocate the install_freertos_gdb(platform, uv_executable,
penv_python, uv_cache_dir) invocation to run after _configure_mcu_toolchains()
completes or invoke it from the code path in platform.py that installs
tool-xtensa-esp-elf-gdb / tool-riscv32-esp-elf-gdb so the GDB packages are
present when install_freertos_gdb runs (search for the platform.py GDB-install
function or _configure_mcu_toolchains to find the correct insertion point).
---
Nitpick comments:
In `@builder/penv_setup.py`:
- Around line 682-685: Replace the hardcoded gdb_tool_packages list with the
shared platform package discovery used in platform.py so new "*-gdb" tools are
picked up automatically; import and call the existing helper in platform.py that
returns platform tool/package metadata (the same function used for GDB
discovery) and filter that result for packages that end with "-gdb" (or
otherwise match the platform's GDB tool predicate), then use that derived list
where gdb_tool_packages is referenced and ensure "freertos-gdb" is still
included if required by the existing installation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14120923-f28b-430d-bff2-d224f51aa13a
📒 Files selected for processing (2)
builder/penv_setup.pyplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
builder/penv_setup.py (2)
700-710: Fix inconsistent indentation insubprocess.check_callarguments.The list elements have misaligned indentation which hurts readability:
🔧 Proposed fix for consistent indentation
try: subprocess.check_call([ - uv_executable, "pip", "install", "--quiet", - f"--python={penv_executable}", - "--target", target_dir, - "freertos-gdb"], + uv_executable, "pip", "install", "--quiet", + f"--python={penv_executable}", + "--target", str(target_dir), + "freertos-gdb" + ], stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT, timeout=60, env=uv_env, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 700 - 710, The subprocess.check_call invocation has inconsistent indentation for its argument list; fix it by reformatting the list elements so they align consistently (e.g., each list item on its own line indented one level inside the call), and align the keyword arguments (stdout, stderr, timeout, env) at the same indentation level as the list closing bracket; update the call to subprocess.check_call so uv_executable, "pip", "install", "--quiet", f"--python={penv_executable}", "--target", target_dir, "freertos-gdb" and the kwargs stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT, timeout=60, env=uv_env are consistently indented for readability.
683-686: Consider centralizing GDB tool package names.These package names are duplicated in
platform.py(MCU_TOOLCHAIN_CONFIG),builder/main.py, and now here. While they currently match, a future rename would require coordinated updates across all locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/penv_setup.py` around lines 683 - 686, gdb_tool_packages is duplicated; replace the local list with a single shared constant and import it where needed: create a module-level constant (e.g., GDB_TOOL_PACKAGES) in the existing central config (where MCU_TOOLCHAIN_CONFIG is defined) and export it, then update this file to use that constant instead of the inline gdb_tool_packages list and update other locations (they currently reference MCU_TOOLCHAIN_CONFIG) to import the new constant as needed; ensure the symbol name matches across modules and remove the duplicated literal lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/penv_setup.py`:
- Line 39: The module-level assignment has_network = has_internet_connection()
or github_actions runs before has_internet_connection is defined, causing a
NameError; move that assignment to after the has_internet_connection function
definition (i.e., place it below the has_internet_connection(...) function) so
the function exists when called, and keep the same variable name has_network and
reference to github_actions to avoid breaking callers.
---
Nitpick comments:
In `@builder/penv_setup.py`:
- Around line 700-710: The subprocess.check_call invocation has inconsistent
indentation for its argument list; fix it by reformatting the list elements so
they align consistently (e.g., each list item on its own line indented one level
inside the call), and align the keyword arguments (stdout, stderr, timeout, env)
at the same indentation level as the list closing bracket; update the call to
subprocess.check_call so uv_executable, "pip", "install", "--quiet",
f"--python={penv_executable}", "--target", target_dir, "freertos-gdb" and the
kwargs stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT, timeout=60,
env=uv_env are consistently indented for readability.
- Around line 683-686: gdb_tool_packages is duplicated; replace the local list
with a single shared constant and import it where needed: create a module-level
constant (e.g., GDB_TOOL_PACKAGES) in the existing central config (where
MCU_TOOLCHAIN_CONFIG is defined) and export it, then update this file to use
that constant instead of the inline gdb_tool_packages list and update other
locations (they currently reference MCU_TOOLCHAIN_CONFIG) to import the new
constant as needed; ensure the symbol name matches across modules and remove the
duplicated literal lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd051284-5caa-4c87-9b54-e86129a8f9d8
📒 Files selected for processing (2)
builder/penv_setup.pyplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
1 participant
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.
Checklist:
Summary by CodeRabbit