Conversation
add another build target for deb packages to fix package naming issue when we use alien to convert from rpm to deb. Issue: LBM1-41980 Signed-off-by: Sergei Me <sergei-lb@lightbitslabs.com>
📝 WalkthroughWalkthroughThe pull request consolidates the Makefile's discovery client build process by merging the distribution-specific pattern rule Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
170-170: Update .PHONY declaration to reference the new target.The .PHONY declaration still references
discovery-client-debs-%which was removed in this PR. We should update it to referencediscovery-client-debsinstead.🔧 Proposed fix
-.PHONY: discovery-rpms-% discovery-client-debs-%\ +.PHONY: discovery-rpms-% discovery-client-debs\ discovery-packages-el8 \ discovery-packages-el9 \ install-discovery-client-packages-% \
🤖 Fix all issues with AI agents
In @Makefile:
- Around line 145-148: The Makefile references WORKSPACE_TOP in the
discovery-client-debs target (and earlier at line ~118) but never defines it;
add a safe default fallback such as setting WORKSPACE_TOP ?= $(shell pwd) near
the top of the Makefile (or explicitly document WORKSPACE_TOP as a required env
var in your README/build docs) so RPMOUT_DIR and the chown invocation in the
discovery-client-debs target operate on correct paths; update the Makefile to
use the fallback and/or add a brief comment explaining the env requirement.
🧹 Nitpick comments (2)
Makefile (2)
150-152: Consider parallel build safety for the shared discovery-client-debs target.Both
discovery-packages-el8anddiscovery-packages-el9now depend on the samediscovery-client-debstarget. If we invokemake -j discovery-packages-el8 discovery-packages-el9(parallel builds), both could executediscovery-client-debsconcurrently, potentially causing race conditions with rpmbuild and alien operating on the same output files.We might want to add an order-only prerequisite or document that these targets shouldn't be built in parallel.
154-158: Consider making the RPM pattern more specific to prevent cross-version contamination.Line 158 uses
discovery-client*el*.rpmwhich would match both el8 and el9 RPMs if both somehow exist in the directory. Whilediscovery-rpms-%cleans before building, we could be more explicit by using the$*variable (which expands to el8 or el9):- $(Q)cp ${RPMOUT_DIR}/x86_64/discovery-client*el*.rpm $(COMPONENT_PATH)/ + $(Q)cp ${RPMOUT_DIR}/x86_64/discovery-client*$*.rpm $(COMPONENT_PATH)/This ensures we copy only the RPM matching the specific target we're building, reducing the risk of cross-version contamination.
♻️ Proposed change
- $(Q)cp ${RPMOUT_DIR}/x86_64/discovery-client*el*.rpm $(COMPONENT_PATH)/ + $(Q)cp ${RPMOUT_DIR}/x86_64/discovery-client*$*.rpm $(COMPONENT_PATH)/
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Makefile
| discovery-client-debs: VERSION = $(or $(LIGHTOS_VERSION),$(DEFAULT_REL)) | ||
| discovery-client-debs: | ||
| $(Q) rpmbuild -bb --clean --define="version ${VERSION}" --define="_builddir `pwd`" --define="dist $(DISCOVERY_CLIENT_RELEASE)" --define "_rpmdir $(RPMOUT_DIR)" discovery-client.spec | ||
| (cd build/dist && sudo alien --to-deb -v -k ${RPMOUT_DIR}/x86_64/discovery-client-${VERSION}-${DISCOVERY_CLIENT_RELEASE}.x86_64.rpm && sudo chown ${USER}:${USER} ${WORKSPACE_TOP}/discovery-client/build/dist/*.deb) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for WORKSPACE_TOP definition or export across the codebase
echo "=== Searching for WORKSPACE_TOP variable definitions ==="
echo ""
# Search in Makefiles and related build files
echo "1. Make variable assignments (Makefiles, *.mk):"
rg -n 'WORKSPACE_TOP\s*[:?]?=' --type=make || echo "No matches found"
echo ""
echo "2. Shell exports:"
rg -n 'export\s+WORKSPACE_TOP' || echo "No matches found"
echo ""
echo "3. Any WORKSPACE_TOP references (to understand usage pattern):"
rg -n 'WORKSPACE_TOP' --type=make -B 2 -A 2 | head -50
echo ""
echo "4. Check root Makefile for variable definitions:"
head -100 Makefile | grep -E 'WORKSPACE|VERSION|RELEASE' || echo "No matches in first 100 lines"Repository: LightBitsLabs/discovery-client
Length of output: 1219
WORKSPACE_TOP variable must be defined or documented as an environment requirement.
We should ensure that WORKSPACE_TOP is either defined within the Makefile or clearly documented as an environment variable requirement. Currently, this variable is referenced in two places (lines 118 and 148) but is never defined in the Makefile itself. Without it being set in the build environment, the RPMOUT_DIR path and the chown command will operate on incorrect paths, causing the build to fail or produce unintended results.
Consider either:
- Adding
WORKSPACE_TOP ?= $(shell pwd)or similar fallback definition to the Makefile, or - Documenting this as a required environment variable in the build instructions/README
🤖 Prompt for AI Agents
In @Makefile around lines 145 - 148, The Makefile references WORKSPACE_TOP in
the discovery-client-debs target (and earlier at line ~118) but never defines
it; add a safe default fallback such as setting WORKSPACE_TOP ?= $(shell pwd)
near the top of the Makefile (or explicitly document WORKSPACE_TOP as a required
env var in your README/build docs) so RPMOUT_DIR and the chown invocation in the
discovery-client-debs target operate on correct paths; update the Makefile to
use the fallback and/or add a brief comment explaining the env requirement.
Add another build target for deb packages to fix the package naming issue when we use alien to convert from rpm to deb.
Issue: LBM1-41980