Implement better support for --silent flag in top-level Makefile #4292
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntroduces a SILENT variable and a print macro to conditionally emit messages in the Makefile; replaces direct echo/info outputs with $(call print, ...) for .DEFAULT, per-target defconfig, and build notices, suppressing these messages when make is run with -s. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as make
participant F as Makefile
participant P as print macro
participant C as Console
U->>M: invoke make (optionally -s)
M->>F: parse Makefile, MAKEFLAGS
F->>F: set SILENT := find 's' in MAKEFLAGS
F->>P: $(call print, "message")
alt SILENT set (silent mode)
P--xC: (no output)
else SILENT not set
P->>C: info "message"
end
F->>M: continue target execution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge Base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
✨ Finishing touches🧪 Generate unit tests
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: 3
♻️ Duplicate comments (1)
Makefile (1)
55-55: Propagate no-print-directory to all sub-makesExtend the earlier change to all $(MAKE) calls so silent runs are fully clean.
- $(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) + $(MAKE) $(NOPRINTDIR) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL)Also apply to:
- Line 42 (default target)
- Line 52 (%-config sub-make)
- Line 59 (buildroot-help)
- Line 38 (.DEFAULT) — per earlier comment
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
Makefile(3 hunks)
| SILENT := $(findstring s,$(word 1, $(MAKEFLAGS))) | ||
|
|
||
| define print | ||
| $(if $(SILENT),,$(info $1)) | ||
| endef | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make silent-mode detection robust; prep a reusable flag for sub-makes
Current detection misses grouped short options and may false-positive on variable text. Filter out long options first and scan all short-option groups. Also precompute a flag to suppress directory spam in sub-makes.
-SILENT := $(findstring s,$(word 1, $(MAKEFLAGS)))
+SILENT := $(findstring s,$(filter-out --%,$(MAKEFLAGS)))
+
+# Pass to sub-makes to suppress "Entering/Leaving directory" noise when silent
+NOPRINTDIR := $(if $(SILENT),--no-print-directory,)🤖 Prompt for AI Agents
In Makefile around lines 19-24, the current SILENT detection can be fooled by
long options and doesn't inspect grouped short options; change it to first strip
long options from MAKEFLAGS, then inspect remaining short-option groups for the
's' flag (e.g. compute a temp variable like MAKEFLAGS_SHORT := $(filter
%-%,$(MAKEFLAGS)) and then MAKEFLAGS_SHORT := $(filter-out
--%,$(MAKEFLAGS_SHORT)) and set SILENT := $(if $(findstring
s,$(MAKEFLAGS_SHORT)),1,) ), and then precompute and export a reusable flag for
sub-makes (e.g. SILENT_FLAG := $(if $(SILENT),-s,)) so sub-makes can use
$(SILENT_FLAG) to suppress directory spam.
| # fallback target when target undefined here is given | ||
| .DEFAULT: | ||
| @echo "$(COLOR_STEP)=== Falling back to Buildroot target '$@' ===$(TERM_RESET)" | ||
| $(call print,$(COLOR_STEP)=== Falling back to Buildroot target '$@' ===$(TERM_RESET)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also suppress “Entering/Leaving directory” noise from sub-makes
When piping output, those messages still appear even with -s. Pass --no-print-directory to the Buildroot sub-make (and replicate for other $(MAKE) calls below).
.DEFAULT:
- $(call print,$(COLOR_STEP)=== Falling back to Buildroot target '$@' ===$(TERM_RESET))
+ $(call print,$(COLOR_STEP)=== Falling back to Buildroot target '$@' ===$(TERM_RESET))
$(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) "$@"
+ # ^ add $(NOPRINTDIR)Apply:
- $(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) "$@"
+ $(MAKE) $(NOPRINTDIR) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) "$@"🤖 Prompt for AI Agents
In Makefile around line 37, the Buildroot sub-make invocation prints
"Entering/Leaving directory" noise; update the $(MAKE) invocation that produces
the line "$(call print,$(COLOR_STEP)=== Falling back to Buildroot target '$@'
===$(TERM_RESET))" to pass --no-print-directory to the sub-make (i.e., add
--no-print-directory to the command that calls $(MAKE) for the Buildroot target)
and apply the same --no-print-directory flag to any other $(MAKE) calls in this
Makefile below to suppress those messages when piping output.
| bash -c 'read -t 10 -p "Waiting 10s, press enter to continue or Ctrl-C to abort..."' || true; \ | ||
| fi | ||
| @echo "$(COLOR_STEP)=== Using $*_defconfig ===$(TERM_RESET)" | ||
| $(call print,$(COLOR_STEP)=== Using $*_defconfig ===$(TERM_RESET)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Send warnings to stderr and skip the 10s prompt in silent/non-tty runs
To keep stdout machine-parseable and avoid blocking CI/pipes, redirect warnings to stderr and don’t wait in silent mode or when not attached to a TTY.
$(TARGETS_CONFIG): %-config:
@if [ -f $(O)/.config ] && ! grep -q 'BR2_DEFCONFIG="$(DEFCONFIG_DIR)/$*_defconfig"' $(O)/.config; then \
- echo "$(COLOR_WARN)WARNING: Output directory '$(O)' already contains files for another target!$(TERM_RESET)"; \
- echo " Before running build for a different target, run 'make distclean' first."; \
- echo ""; \
- bash -c 'read -t 10 -p "Waiting 10s, press enter to continue or Ctrl-C to abort..."' || true; \
+ echo "$(COLOR_WARN)WARNING: Output directory '$(O)' already contains files for another target!$(TERM_RESET)" 1>&2; \
+ echo " Before running build for a different target, run 'make distclean' first." 1>&2; \
+ echo "" 1>&2; \
+ if [ -z "$(SILENT)" ] && [ -t 0 ]; then \
+ bash -c 'read -t 10 -p "Waiting 10s, press enter to continue or Ctrl-C to abort..."' || true; \
+ fi; \
fi
$(call print,$(COLOR_STEP)=== Using $*_defconfig ===$(TERM_RESET))
$(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) "$*_defconfig"Additionally pass $(NOPRINTDIR) to this sub-make (see next comment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $(call print,$(COLOR_STEP)=== Using $*_defconfig ===$(TERM_RESET)) | |
| $(TARGETS_CONFIG): %-config: | |
| @if [ -f $(O)/.config ] && ! grep -q 'BR2_DEFCONFIG="$(DEFCONFIG_DIR)/$*_defconfig"' $(O)/.config; then \ | |
| echo "$(COLOR_WARN)WARNING: Output directory '$(O)' already contains files for another target!$(TERM_RESET)" 1>&2; \ | |
| echo " Before running build for a different target, run 'make distclean' first." 1>&2; \ | |
| echo "" 1>&2; \ | |
| if [ -z "$(SILENT)" ] && [ -t 0 ]; then \ | |
| bash -c 'read -t 10 -p "Waiting 10s, press enter to continue or Ctrl-C to abort..."' || true; \ | |
| fi; \ | |
| fi | |
| $(call print,$(COLOR_STEP)=== Using $*_defconfig ===$(TERM_RESET)) | |
| $(MAKE) -C $(BUILDROOT) O=$(O) BR2_EXTERNAL=$(BUILDROOT_EXTERNAL) "$*_defconfig" |
🤖 Prompt for AI Agents
In Makefile around line 51, the current rule prints a step message to stdout and
may block CI by waiting for a 10s prompt; update the invocation so warnings/step
messages go to stderr (redirect the print/echo output to >&2) and ensure the
sub-make is invoked with silent/non-tty detection so the 10s prompt is skipped
(check for .SILENT or test -t 1 and conditionally avoid the sleep/prompt), and
also add passing $(NOPRINTDIR) into the sub-make invocation so the child make
inherits the no-print-dir behavior.
sairon
force-pushed
the
makefile-support-silent
branch
from
993be97 to
2aee67d
Compare
The extra information printed when using the top-level makefile can clutter the output when it needs to be further processed, e.g. when running `make show-info | jq`. Make it respect the --silent flag (which also suppresses messages about changing directories which would break parsing as well).
sairon
force-pushed
the
makefile-support-silent
branch
from
2aee67d to
c719f2c
Compare
The extra information printed when using the top-level makefile can clutter the output when it needs to be further processed, e.g. when running
make show-info | jq. Make it respect the --silent flag (which also suppresses messages about changing directories which would break parsing as well).Summary by CodeRabbit
New Features
Chores