Build: Better Parallelization#64
Conversation
el8/el9 shared build/dist (raced on rm/rpmbuild) and the deb used 'sudo alien' which left a root-owned scratch dir a later non-root rm couldn't clear. Make RPMOUT_DIR per-distro (build/dist/$*), and build the deb with 'fakeroot alien' in an isolated mktemp scratch (no sudo/chown). Signed-off-by: kyler <kyler@lightbitslabs.com> Issue: LBM1-45815 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Makefile's packaging targets are refactored to use architecture-scoped output directories. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
Makefile (2)
152-153: 💤 Low valueOptional: Declare
discovery-client-debsas a PHONY target.Since
discovery-client-debsis not a file target (it's a task that produces files in different locations), we should declare it as.PHONYto ensure Make always runs it regardless of filesystem state.📝 Suggested addition
Near other
.PHONYdeclarations in the Makefile:.PHONY: discovery-client-debs🤖 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 `@Makefile` around lines 152 - 153, The discovery-client-debs target is not declared as a PHONY target, which can cause Make to skip execution if a file or directory with that name already exists. Add discovery-client-debs to the PHONY declarations in the Makefile by either adding it to an existing .PHONY line along with other non-file targets, or creating a new .PHONY: discovery-client-debs declaration near the other .PHONY declarations in the file.Source: Linters/SAST tools
168-168: ⚡ Quick winConsider using a shared variable for the RPM output path.
Line 168 hardcodes
build/dist/$*/x86_64/which duplicates the directory structure fromRPMOUT_DIRindiscovery-rpms-%(line 147). While this works, it creates a maintenance risk if we ever change the output directory structure—we'd need to update both locations.♻️ Suggested approach: Define a shared Make function
At the top of the Makefile (in the global scope), define:
rpm_out_dir = $(WORKSPACE_TOP)/discovery-client/build/dist/$1Then update both targets:
-discovery-rpms-%: RPMOUT_DIR = $(WORKSPACE_TOP)/discovery-client/build/dist/$* +discovery-rpms-%: RPMOUT_DIR = $(call rpm_out_dir,$*)- $(Q)cp build/dist/$*/x86_64/discovery-client*el*.rpm $(COMPONENT_PATH)/ + $(Q)cp $(call rpm_out_dir,$*)/x86_64/discovery-client*el*.rpm $(COMPONENT_PATH)/This ensures both targets use the same path pattern and reduces the risk of divergence.
🤖 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 `@Makefile` at line 168, The hardcoded path build/dist/$*/x86_64/ in the cp command duplicates the directory structure already defined in RPMOUT_DIR within the discovery-rpms-% target, creating maintenance risk. Define a shared Make function at the top of the Makefile (in global scope) named rpm_out_dir that accepts a parameter and returns the full path pattern incorporating WORKSPACE_TOP, discovery-client, build/dist, and the component parameter. Then update both the cp command in this target and the RPMOUT_DIR definition in discovery-rpms-% to use this shared function instead of hardcoding the path.
🤖 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 `@Makefile`:
- Around line 152-158: The discovery-client-debs target uses the global
RPMOUT_DIR variable to locate the RPM file, but the actual RPM is built in an
el8-specific subdirectory that is not accounted for in the current path. Fix
this by adding a target-specific RPMOUT_DIR variable assignment for the
discovery-client-debs target that includes the el8 subdirectory in the path,
matching where the discovery-rpms-el8 target actually creates the RPM file. This
will ensure the alien conversion command on line 156 can correctly locate the
RPM file at the expected location.
---
Nitpick comments:
In `@Makefile`:
- Around line 152-153: The discovery-client-debs target is not declared as a
PHONY target, which can cause Make to skip execution if a file or directory with
that name already exists. Add discovery-client-debs to the PHONY declarations in
the Makefile by either adding it to an existing .PHONY line along with other
non-file targets, or creating a new .PHONY: discovery-client-debs declaration
near the other .PHONY declarations in the file.
- Line 168: The hardcoded path build/dist/$*/x86_64/ in the cp command
duplicates the directory structure already defined in RPMOUT_DIR within the
discovery-rpms-% target, creating maintenance risk. Define a shared Make
function at the top of the Makefile (in global scope) named rpm_out_dir that
accepts a parameter and returns the full path pattern incorporating
WORKSPACE_TOP, discovery-client, build/dist, and the component parameter. Then
update both the cp command in this target and the RPMOUT_DIR definition in
discovery-rpms-% to use this shared function instead of hardcoding the path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
discovery-client-debs was shared by both package components, so concurrent el8/el9 builds still raced on the deb's rpmbuild --clean output (build/dist/x86_64) and the deb copy. Make it per-distro (discovery-client-debs-%) into build/deb/$*; install copies the per-distro deb. Signed-off-by: kyler <kyler@lightbitslabs.com> Issue: LBM1-45815 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Around line 152-161: The discovery-client-debs-% target is missing an explicit
prerequisite on build/discovery-client, which causes direct invocation to fail
if the binary hasn't been built. Additionally, the temporary directory created
in deb_scratch variable will leak if fakeroot alien fails because the && chain
exits before the cleanup command executes. Add build/discovery-client as a
prerequisite dependency to the discovery-client-debs-% target declaration, and
refactor the command block to ensure the rm -rf $$deb_scratch cleanup executes
regardless of whether fakeroot alien succeeds or fails, such as by using a trap
statement or restructuring the command logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Per-distro work dir so concurrent el8/el9 deb builds don't share a path. | ||
| discovery-client-debs-%: VERSION = $(or $(LIGHTOS_VERSION),$(DEFAULT_REL)) | ||
| discovery-client-debs-%: DEB_WORK = $(WORKSPACE_TOP)/discovery-client/build/deb/$* | ||
| discovery-client-debs-%: | ||
| $(Q) rm -rf $(DEB_WORK) | ||
| $(Q) rpmbuild -bb --clean --define="version ${VERSION}" --define="_builddir `pwd`" --define="dist $(DISCOVERY_CLIENT_RELEASE)" --define "_rpmdir $(DEB_WORK)/rpm" discovery-client.spec | ||
| $(Q) deb_scratch=`mktemp -d` && \ | ||
| ( cd $$deb_scratch && fakeroot alien --to-deb -v -k $(DEB_WORK)/rpm/x86_64/discovery-client-${VERSION}-${DISCOVERY_CLIENT_RELEASE}.x86_64.rpm ) && \ | ||
| cp $$deb_scratch/*.deb $(DEB_WORK)/ && \ | ||
| rm -rf $$deb_scratch |
There was a problem hiding this comment.
Missing prerequisite for direct invocation and temp directory may leak on failure.
The discovery-client-debs-% target runs rpmbuild but doesn't declare a dependency on build/discovery-client. While this works transitively through discovery-packages-el* (which depends on discovery-rpms-%), a direct make discovery-client-debs-el8 invocation would fail if the binary hasn't been built yet. Additionally, if fakeroot alien fails, the && chain exits and $$deb_scratch won't be cleaned up.
🛠️ Suggested fix with trap for cleanup and explicit dependency
# Per-distro work dir so concurrent el8/el9 deb builds don't share a path.
discovery-client-debs-%: VERSION = $(or $(LIGHTOS_VERSION),$(DEFAULT_REL))
discovery-client-debs-%: DEB_WORK = $(WORKSPACE_TOP)/discovery-client/build/deb/$*
-discovery-client-debs-%:
+discovery-client-debs-%: build/discovery-client
$(Q) rm -rf $(DEB_WORK)
$(Q) rpmbuild -bb --clean --define="version ${VERSION}" --define="_builddir `pwd`" --define="dist $(DISCOVERY_CLIENT_RELEASE)" --define "_rpmdir $(DEB_WORK)/rpm" discovery-client.spec
- $(Q) deb_scratch=`mktemp -d` && \
- ( cd $$deb_scratch && fakeroot alien --to-deb -v -k $(DEB_WORK)/rpm/x86_64/discovery-client-${VERSION}-${DISCOVERY_CLIENT_RELEASE}.x86_64.rpm ) && \
- cp $$deb_scratch/*.deb $(DEB_WORK)/ && \
- rm -rf $$deb_scratch
+ $(Q) deb_scratch=`mktemp -d`; \
+ trap "rm -rf $$deb_scratch" EXIT; \
+ ( cd $$deb_scratch && fakeroot alien --to-deb -v -k $(DEB_WORK)/rpm/x86_64/discovery-client-${VERSION}-${DISCOVERY_CLIENT_RELEASE}.x86_64.rpm ) && \
+ cp $$deb_scratch/*.deb $(DEB_WORK)/🧰 Tools
🪛 checkmake (0.3.2)
[warning] 155-155: Target body for "discovery-client-debs-%" exceeds allowed length of 5 lines (6).
(maxbodylength)
[warning] 153-153: Target "discovery-client-debs-%" should be declared PHONY.
(phonydeclared)
[warning] 154-154: Target "discovery-client-debs-%" should be declared PHONY.
(phonydeclared)
[warning] 154-154: Target "discovery-client-debs-%" defined multiple times (lines 153 and 154).
(uniquetargets)
[warning] 155-155: Target "discovery-client-debs-%" defined multiple times (lines 153 and 155).
(uniquetargets)
🤖 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 `@Makefile` around lines 152 - 161, The discovery-client-debs-% target is
missing an explicit prerequisite on build/discovery-client, which causes direct
invocation to fail if the binary hasn't been built. Additionally, the temporary
directory created in deb_scratch variable will leak if fakeroot alien fails
because the && chain exits before the cleanup command executes. Add
build/discovery-client as a prerequisite dependency to the
discovery-client-debs-% target declaration, and refactor the command block to
ensure the rm -rf $$deb_scratch cleanup executes regardless of whether fakeroot
alien succeeds or fails, such as by using a trap statement or restructuring the
command logic.
1 participant
When building el8/el9 targets in parallel the two targets would collide on their shared directory. This splits them such that they can build in parallel.
For testing I built the full light-app:packages-core-el8/el9 targets with v2 with force and no checkout.
Depends-On: https://github.com/LightBitsLabs/lb_containers/pull/152
Issue: LBM1-45815