Cleanup makefile and add docs - #39
Conversation
Cleanup the Makefile with unused targets and added a new doc.
📝 WalkthroughWalkthroughMakefile build system refactored to use COPR group identity instead of user-specific hardcoding, with chroot scope limited to rawhide. Dynamic FAS_USERNAME resolution and new clean target replace obsolete targets. Documentation updated across README and new makefile.md to reflect streamlined build flow. ChangesCOPR group migration and build system refactoring
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
23-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove stale reference to deleted
make spectarget.The setup instructions reference
make spec(lines 29-31), but this target was removed from the Makefile in this PR. Users following these instructions will encounter a "No rule to make target 'spec'" error.📝 Proposed fix
## Setup Before executing a new build, run the initial setup first: ```bash # Create a new copr repository make create-copr-repo - -# Will create a specfile based on project settings. -# This is only needed in case the goose.spec is missing. -make specAlternatively, if a spec file generation workflow still exists outside the Makefile, update the comment to reflect the new process. </details> As per coding guidelines, documentation must not contain stale references to old file names or structures. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 23 - 32, Remove the stale reference to the deleted
make target by deleting the lines that mention and call "make spec" and the
accompanying comment; keep the "make create-copr-repo" instruction intact
(referencing the existing target create-copr-repo) or replace the removed "make
spec" text with an updated note describing the new specfile generation workflow
if one exists outside the Makefile so README.md no longer references the
non-existent "make spec" target.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (2)
Makefile (2)
3-3: ⚡ Quick winRemove unused TOOL2RPM variable.
The
TOOL2RPMvariable is defined but never referenced in any target. This appears to be leftover from the removedspectarget.🧹 Proposed fix
NAME := goose PKG_NAME := $(NAME) -TOOL2RPM := rust2rpm FAS_USERNAME := $(shell copr whoami)🤖 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 3, Remove the unused TOOL2RPM variable definition from the Makefile: delete the line that defines TOOL2RPM := rust2rpm (ensure no other targets or variables reference TOOL2RPM first), leaving the Makefile without this dead variable.
6-38: 💤 Low valueConsider adding inline comments for target documentation.
While the targets follow conventions, adding brief inline comments (especially for less obvious targets like
freeze) would improve maintainability. The coding guidelines emphasize that targets should be well-documented.Example:
# Freeze Python dependency versions for goose extensions .PHONY: freeze freeze: ./scripts/freeze.py --python-versions 3.14As per coding guidelines, Makefile targets should be well-documented and follow conventions.
🤖 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 6 - 38, Add short inline comments above each Makefile target to document purpose and any non-obvious flags: e.g. annotate create-copr-repo, sources, srpm, build (note chroot and timeout), logs (mention fuzzytail prerequisite and usage of $(FAS_USERNAME)/$(NAME)), clean, and especially freeze (explain it runs ./scripts/freeze.py to lock Python dependency versions for goose extensions with --python-versions 3.14). Keep comments brief, use the target names create-copr-repo, sources, srpm, build, logs, clean, and freeze to locate where to insert the comments, and ensure .PHONY targets remain unchanged.🤖 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`: - Line 4: The Makefile currently sets FAS_USERNAME at parse time with FAS_USERNAME := $(shell copr whoami) which can error and lead to confusing failures when running the logs target; update the assignment to silence copr stderr (redirect copr whoami stderr to /dev/null) so it doesn't print noise during parsing, then add a new phony target check-copr-auth that fails fast if FAS_USERNAME is empty (print a clear error and exit non-zero), and make the logs target depend on check-copr-auth so logs will abort immediately with a readable message instead of invoking fuzzytail with an empty username; reference the FAS_USERNAME variable, the logs target, and the new check-copr-auth target when making these changes. --- Outside diff comments: In `@README.md`: - Around line 23-32: Remove the stale reference to the deleted make target by deleting the lines that mention and call "make spec" and the accompanying comment; keep the "make create-copr-repo" instruction intact (referencing the existing target create-copr-repo) or replace the removed "make spec" text with an updated note describing the new specfile generation workflow if one exists outside the Makefile so README.md no longer references the non-existent "make spec" target. --- Nitpick comments: In `@Makefile`: - Line 3: Remove the unused TOOL2RPM variable definition from the Makefile: delete the line that defines TOOL2RPM := rust2rpm (ensure no other targets or variables reference TOOL2RPM first), leaving the Makefile without this dead variable. - Around line 6-38: Add short inline comments above each Makefile target to document purpose and any non-obvious flags: e.g. annotate create-copr-repo, sources, srpm, build (note chroot and timeout), logs (mention fuzzytail prerequisite and usage of $(FAS_USERNAME)/$(NAME)), clean, and especially freeze (explain it runs ./scripts/freeze.py to lock Python dependency versions for goose extensions with --python-versions 3.14). Keep comments brief, use the target names create-copr-repo, sources, srpm, build, logs, clean, and freeze to locate where to insert the comments, and ensure .PHONY targets remain unchanged.🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID:
c3d3153b-c77d-47cd-b1b1-beab772791dd📒 Files selected for processing (3)
MakefileREADME.mddocs/makefile.md
| ifneq ("$(wildcard rust2rpm.toml)","") | ||
| FILES_TO_SYNC += rust2rpm.toml | ||
| endif | ||
| FAS_USERNAME := $(shell copr whoami) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the Makefile gracefully handles copr whoami failures
# Test what happens if copr whoami fails
(unset COPR_CONFIG_DIR; copr whoami 2>&1) || echo "copr whoami can fail"
# Check if FAS_USERNAME is used in targets that would fail silently
rg -n 'FAS_USERNAME' MakefileRepository: rhel-lightspeed/goose
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the Makefile with line numbers around the referenced lines
nl -ba Makefile | sed -n '1,80p'Repository: rhel-lightspeed/goose
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Makefile with line numbers (using cat -n since nl isn't available)
cat -n Makefile | sed -n '1,120p'
# Also show the specific region around the earlier hit lines (4 and 29)
echo "---- around line 1-40 ----"
cat -n Makefile | sed -n '1,50p'
echo "---- around line 20-40 ----"
cat -n Makefile | sed -n '20,45p'Repository: rhel-lightspeed/goose
Length of output: 2678
Add validation/fail-fast for FAS_USERNAME before running logs.
FAS_USERNAME := $(shell copr whoami) is evaluated at Makefile load time; if copr whoami errors or returns empty, logs will run fuzzytail watch $(FAS_USERNAME)/$(NAME) and produce unclear failures. Add stderr-silencing plus an explicit guard that fails fast when logs is invoked.
FAS_USERNAME := $(shell copr whoami)
🛡️ Proposed defensive implementation
-FAS_USERNAME := $(shell copr whoami)
+FAS_USERNAME := $(shell copr whoami 2>/dev/null || echo "")
+
+.PHONY: check-copr-auth
+check-copr-auth:
+ `@test` -n "$(FAS_USERNAME)" || { echo >&2 "Error: copr whoami failed. Please authenticate with 'copr whoami'"; exit 1; }Then make logs depend on check-copr-auth:
.PHONY: logs
-logs:
+logs: check-copr-auth
`@command` -v fuzzytail > /dev/null || { echo >&2 "fuzzytail is not installed. Install with pip install fuzzytail"; }
fuzzytail watch $(FAS_USERNAME)/$(NAME)🤖 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 4, The Makefile currently sets FAS_USERNAME at parse time
with FAS_USERNAME := $(shell copr whoami) which can error and lead to confusing
failures when running the logs target; update the assignment to silence copr
stderr (redirect copr whoami stderr to /dev/null) so it doesn't print noise
during parsing, then add a new phony target check-copr-auth that fails fast if
FAS_USERNAME is empty (print a clear error and exit non-zero), and make the logs
target depend on check-copr-auth so logs will abort immediately with a readable
message instead of invoking fuzzytail with an empty username; reference the
FAS_USERNAME variable, the logs target, and the new check-copr-auth target when
making these changes.
2 participants
Cleanup the Makefile with unused targets and added a new doc.
Summary by CodeRabbit
Documentation
Chores