Portable builds#17
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a ChangesBuild System, Toolchain, CI, and Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/additional-resources/wsl-notes.md (1)
17-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix broken step numbering: step 4 is missing.
The list jumps from step 3 (line 17) to step 5 (line 26). Either renumber step 5 to 4, or add the missing step (typically
usbipd attach --busid <BUSID>afterbind).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/additional-resources/wsl-notes.md` around lines 17 - 26, The numbered list in the WSL notes skips a step between the USBIPD bind commands and the WSL visibility check. Update the sequence in the markdown list by either renumbering the current “5. On WSL confirm visibility of USB device” entry to the next correct number, or insert the missing step after the bind commands in the same section (the step should align with the USBIPD workflow), so the numbering is continuous in docs/additional-resources/wsl-notes.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 24-26: The CI Build step currently exercises only the serial make
path, so update the job to run the build target under parallel execution instead
of plain make build. Adjust the Build step in the workflow to use make --jobs=2
build so the same prerequisite scheduling behavior that can expose dependency
races is covered by CI.
- Around line 1-32: The CI workflow uses mutable action tags and lacks explicit
token scoping; update the workflow in ci.yaml by adding a top-level permissions
block for the jobs, pin the actions/checkout and actions/upload-artifact
references in the fmt and build jobs to full commit SHAs, and set
persist-credentials to false on each actions/checkout step if git authentication
is not required. Use the existing fmt and build job steps to locate the two
checkout usages and the upload-artifact step.
In `@docs/main-content/build-flash-run.md`:
- Around line 33-37: In the note within the build-flash-run documentation,
update the operating system name from “MacOS” to “macOS” to match Apple’s
official capitalization. Locate the text in the note near the serial device
example and replace the incorrect casing only, without changing the surrounding
command or link text.
In `@Makefile`:
- Around line 59-67: The artifact-consuming helper targets are missing an
explicit dependency on the build step, so they can run before the generated
files exist on a clean tree. Update the Make graph so gds, gdb*, and attach*
depend on build the same way flash* already does, and keep the existing helper
prerequisites like check-no-gds and fprime-venv intact. Use the target names gds
and the related gdb/attach rules to locate and adjust their dependency lists.
- Around line 38-49: The Makefile is missing the documented generate-force
recovery entry point, so contributors hitting a corrupted cache will fail on
make generate-force. Add a thin alias target named generate-force that routes to
the existing generate behavior (or otherwise preserves the documented recovery
path), and keep the related generate, generate-if-needed, and build targets
consistent with the new entry point.
In `@makelib/build-tools.mk`:
- Line 22: UV_RUN currently only sets up the virtual environment, so the build
still falls back to a globally installed uv when UV_BIN is unset in the uv
command path. Update UV_RUN in build-tools.mk to also export the managed uv
binary path from the local fprime-venv, matching the fallback logic in uv.py and
ensuring zephyr.mk invokes the bundled uv consistently on clean machines.
- Around line 19-20: The installer step in the `$(UV)` target hides failures
because piping `curl` directly into `sh` can succeed even when the download
breaks; update the `$(UV)` rule to avoid the pipeline by saving the installer
script to a temporary file first and then executing it. Use the existing
`$(UV)`, `$(UV_DIR)`, and `UV_VERSION` recipe in `build-tools.mk` as the place
to make this change, keeping the same install behavior while making download
errors surface properly.
In `@makelib/zephyr.mk`:
- Around line 33-34: The clean-zephyr-workspace target currently only
deinitializes the Zephyr submodule, so the west-managed checkouts are not
removed. Update the clean-zephyr-workspace recipe in zephyr.mk to explicitly
remove the lib/zephyr-workspace/{bootloader,modules,tools} directories instead
of relying on git submodule deinit --all -f, and keep the target focused on
fully cleaning the workspace state.
- Around line 10-13: Delay SDK_VERSION evaluation in zephyr.mk so it is expanded
only after zephyr-workspace has populated ZEPHYR_PATH. Update the SDK_VERSION
assignment to use recursive expansion instead of immediate expansion, and keep
the existing ZEPHYR_SDK_PATH fallback tied to SDK_VERSION so it resolves
correctly on a fresh checkout.
---
Outside diff comments:
In `@docs/additional-resources/wsl-notes.md`:
- Around line 17-26: The numbered list in the WSL notes skips a step between the
USBIPD bind commands and the WSL visibility check. Update the sequence in the
markdown list by either renumbering the current “5. On WSL confirm visibility of
USB device” entry to the next correct number, or insert the missing step after
the bind commands in the same section (the step should align with the USBIPD
workflow), so the numbering is continuous in
docs/additional-resources/wsl-notes.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c020ad1f-35d7-414a-94ce-5b4470ad0012
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.agents/skills/confirm-crash-and-gather-logs/SKILL.md.clang-format.gdbinit.github/actions/setup/action.yml.github/workflows/ci.yaml.gitignore.pre-commit-config.yaml.west/configAGENTS.mdCMakeLists.txtCMakePresets.jsonFprimeZephyrReference/Components/ByteComBridge/docs/sdd.mdFprimeZephyrReference/Components/FatalHandler/FatalHandler.hppFprimeZephyrReference/ReferenceDeployment/CMakeLists.txtFprimeZephyrReference/ReferenceDeployment/README.mdFprimeZephyrReference/ReferenceDeployment/test/int/int_config.jsonFprimeZephyrReference/ReferenceDeployment/test/int/test_reboot_machine.pyFprimeZephyrReference/project/config/CdhCoreConfig.fppFprimeZephyrReference/project/config/CdhCoreTlmConfig.fppFprimeZephyrReference/project/config/ComCcsdsConfig.fppMakefileREADME.mddocs/additional-resources/board-list.mddocs/additional-resources/specifying-board-configuration.mddocs/additional-resources/troubleshooting.mddocs/additional-resources/wsl-notes.mddocs/main-content/build-flash-run.mddocs/main-content/initial-setup.mddocs/uploading/stm32/Arduino-STM32CubeProgrammer-Arm-Issue.mddocs/uploading/stm32/stm32.mddocs/uploading/teensy.mdfprime-gds.ymlgithooks/pre-commitlib/west-commands/uv.pymakelib/build-tools.mkmakelib/zephyr.mksettings.initest/conftest.pytest/two-board/main_test.pywest-commands.ymlwest.yml
💤 Files with no reviewable changes (6)
- .west/config
- githooks/pre-commit
- .clang-format
- test/two-board/main_test.py
- .gdbinit
- .agents/skills/confirm-crash-and-gather-logs/SKILL.md
jrpear
left a comment
jrpear
left a comment
There was a problem hiding this comment.
This is a big improvement to the approachability of the project, thanks Nate!
I left some comments and suggestions if you want to take a look when you've got a chance.
| .PHONY: uv | ||
| uv: $(UV) ## Download uv | ||
| $(UV): $(TOOLS_DIR) | ||
| @test -s $(UV) || { mkdir -p $(UV_DIR); curl -LsSf https://astral.sh/uv/$(UV_VERSION)/install.sh | UV_UNMANAGED_INSTALL=$(UV_DIR) sh > /dev/null; } |
There was a problem hiding this comment.
I am a leery about running a shell script downloaded from the internet. It's from astral.sh so I'm not worried about malware, but these scripts can sometimes do very annoying things like create nodes in your home directory, modify your rc files, etc. And they can be changed at any point. Can we download an executable directly or maybe install via a tool that will just put the executable where we want like cargo? We already require cargo in order to install a particular probe-rs version for the attach and gdb targets.
There was a problem hiding this comment.
I understand the concern. We are configuring the install script to do exactly that. It will download the binary for your distribution into the UV_DIR which will be ./build/uv-someversion/{uv|uvx}. Try it out!
There was a problem hiding this comment.
OK wonderful.
If the recipe was just downloading the release from e.g. https://github.com/astral-sh/uv/releases/download/0.11.26/uv-x86_64-unknown-linux-gnu.tar.gz and extracting uv I think it would be a bit clearer that that's all that's going on. What do you think?
| $(UV): $(TOOLS_DIR) | ||
| @test -s $(UV) || { mkdir -p $(UV_DIR); curl -LsSf https://astral.sh/uv/$(UV_VERSION)/install.sh | UV_UNMANAGED_INSTALL=$(UV_DIR) sh > /dev/null; } | ||
|
|
||
| UV_RUN ?= VIRTUAL_ENV=$(shell pwd)/fprime-venv PATH=$(shell pwd)/fprime-venv/bin:$$PATH |
There was a problem hiding this comment.
Is UV_RUN a good name for this? There doesn't seem to be anything uv-related going on here unless I am missing something.
|
|
||
| .PHONY: zephyr-config | ||
| zephyr-config: fprime-venv ## Configure west | ||
| @test -f .west/config || { \ |
There was a problem hiding this comment.
Could we just call the target .west/config and skip this check?
| .PHONY: zephyr-export | ||
| zephyr-export: zephyr-workspace ## Export Zephyr environment variables | ||
| @test -d $(CMAKE_PACKAGES)/Zephyr/ || \ | ||
| test -d $(CMAKE_PACKAGES)/ZephyrUnittest/ || \ |
There was a problem hiding this comment.
Same thing here, couldn't we have two targets named each of these files and the recipe be just west zephyr-export?
| .PHONY: generate-if-needed | ||
| BUILD_DIR ?= $(shell pwd)/build-fprime-automatic-zephyr | ||
| generate-if-needed: submodules fprime-venv zephyr | ||
| @test -d $(BUILD_DIR) || $(UV_RUN) fprime-util generate --force |
There was a problem hiding this comment.
Same thing here, seems silly to test dir existence in a recipe then run if it's not there, that's what Make is for, I think we should just name the target the thing that's being made.
| probe-rs attach --probe $(PROBE_USB_ID):$(ACTIVE_PROBE) build-artifacts/zephyr.elf | ||
|
|
||
| include makelib/build-tools.mk | ||
| include makelib/zephyr.mk |
There was a problem hiding this comment.
Can we put these up with the other include?
| modules, so they must depend on `zephyr-workspace`, not just `fprime-venv`. | ||
|
|
||
| When adding a build step that consumes files produced by another step, add the | ||
| corresponding `make` prerequisite rather than assuming run order. |
There was a problem hiding this comment.
Do we need all this explicit guidance about how the Makefile works in AGENTS.md? I have found coding agents to be pretty good at figuring this stuff out on the fly when necessary, so if we can keep AGENTS.md shorter for the sake of each of our own "context windows" then let's do it. But if you've had an agent getting tripped up writing Make rules I don't have a problem with this.
| When adding a build step that consumes files produced by another step, add the | ||
| corresponding `make` prerequisite rather than assuming run order. | ||
|
|
||
| The west manifest is defined in `west.yml` and `west.yml` specifies which Zephyr modules to download. When modifying the build system or Zephyr dependencies, keep `west.yml` in sync with `proves-core-reference` as a reference implementation. |
There was a problem hiding this comment.
I'm a little dubious about this instruction. proves-core-reference is not necessarily visible to an agent in the repo. And just because a change in west.yml is suitable for proves-core-reference doesn't mean it's suitable for ground-radio-controller.
jrpear
left a comment
jrpear
left a comment
There was a problem hiding this comment.
Going to merge and move the comments to an issue in the interest of keeping PR from rotting.
2 participants
This PR should make it simpler to build the software in the repo.
Just run
make buildThis PR also includes CI to verify that the Make commands work and the software builds.