Fix build system issues #128
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This addresses the following critical build system issues:
1. Kconfig syntax error
- Replace unsupported $(success,...) with Kconfiglib's $(shell,...)
- Use 'def_bool $(shell,pkg-config --exists ... && echo y)' for
dependency detection
2. Mixed target check logic
- Change from "any goal matches" to "all goals match" logic
- Use filter-out to verify all goals are known before including
dependencies
- Prevents build failures with mixed known/unknown targets
3. Multi-target invocation
- Invert logic to only skip mk/common.mk for pure config/defconfig
- Support mixed invocations like 'make defconfig all'
- Handle empty goals (default to 'all')
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 3 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="configs/Kconfig">
<violation number="1" location="configs/Kconfig:9">
If pkg-config fails, $(shell,…) expands to an empty string, so this def_bool line becomes syntactically invalid and breaks Kconfig parsing. Please emit an explicit "n" on failure to keep the default well-formed.</violation>
<violation number="2" location="configs/Kconfig:12">
The $(shell,…) call returns nothing when pixman-1 is unavailable, leaving def_bool without a value and triggering a parse error. Please ensure the command echoes "n" on failure.</violation>
<violation number="3" location="configs/Kconfig:15">
When libpng is not installed, this $(shell,…) expansion is empty, leaving def_bool malformed and causing Kconfig parsing to fail. Echo an explicit "n" on failure to restore correct behaviour.</violation>
<violation number="4" location="configs/Kconfig:18">
On dependency failure this $(shell,…) call expands to nothing, leaving def_bool without a value and causing a parse error. Echo "n" on failure to keep the default valid.</violation>
</file>
<file name="mk/common.mk">
<violation number="1" location="mk/common.mk:688">
This condition now skips dependency includes whenever a clean goal is present, so `make clean all` loses its dependency files. Please only treat clean-only invocations as the skip case.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| config HAVE_LIBJPEG | ||
| def_bool $(success,pkg-config --exists libjpeg) | ||
| def_bool $(shell,pkg-config --exists libjpeg && echo y) |
There was a problem hiding this comment.
On dependency failure this $(shell,…) call expands to nothing, leaving def_bool without a value and causing a parse error. Echo "n" on failure to keep the default valid.
Prompt for AI agents
Address the following comment on configs/Kconfig at line 18:
<comment>On dependency failure this $(shell,…) call expands to nothing, leaving def_bool without a value and causing a parse error. Echo "n" on failure to keep the default valid.</comment>
<file context>
@@ -4,18 +4,18 @@ config CONFIGURED
config HAVE_LIBJPEG
- def_bool $(success,pkg-config --exists libjpeg)
+ def_bool $(shell,pkg-config --exists libjpeg && echo y)
choice
</file context>
Suggested change
| def_bool $(shell,pkg-config --exists libjpeg && echo y) | |
| def_bool $(shell,pkg-config --exists libjpeg && echo y || echo n) |
Contributor
Author
There was a problem hiding this comment.
The above is not necessary since our Kconfiglib ( https://github.com/sysprog21/Kconfiglib ) can deal properly.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
|
|
||
| config HAVE_LIBPNG | ||
| def_bool $(success,pkg-config --exists libpng) | ||
| def_bool $(shell,pkg-config --exists libpng && echo y) |
There was a problem hiding this comment.
When libpng is not installed, this $(shell,…) expansion is empty, leaving def_bool malformed and causing Kconfig parsing to fail. Echo an explicit "n" on failure to restore correct behaviour.
Prompt for AI agents
Address the following comment on configs/Kconfig at line 15:
<comment>When libpng is not installed, this $(shell,…) expansion is empty, leaving def_bool malformed and causing Kconfig parsing to fail. Echo an explicit "n" on failure to restore correct behaviour.</comment>
<file context>
@@ -4,18 +4,18 @@ config CONFIGURED
config HAVE_LIBPNG
- def_bool $(success,pkg-config --exists libpng)
+ def_bool $(shell,pkg-config --exists libpng && echo y)
config HAVE_LIBJPEG
</file context>
Suggested change
| def_bool $(shell,pkg-config --exists libpng && echo y) | |
| def_bool $(shell,pkg-config --exists libpng && echo y || echo n) |
|
|
||
| config HAVE_PIXMAN | ||
| def_bool $(success,pkg-config --exists pixman-1) | ||
| def_bool $(shell,pkg-config --exists pixman-1 && echo y) |
There was a problem hiding this comment.
The $(shell,…) call returns nothing when pixman-1 is unavailable, leaving def_bool without a value and triggering a parse error. Please ensure the command echoes "n" on failure.
Prompt for AI agents
Address the following comment on configs/Kconfig at line 12:
<comment>The $(shell,…) call returns nothing when pixman-1 is unavailable, leaving def_bool without a value and triggering a parse error. Please ensure the command echoes "n" on failure.</comment>
<file context>
@@ -4,18 +4,18 @@ config CONFIGURED
config HAVE_PIXMAN
- def_bool $(success,pkg-config --exists pixman-1)
+ def_bool $(shell,pkg-config --exists pixman-1 && echo y)
config HAVE_LIBPNG
</file context>
Suggested change
| def_bool $(shell,pkg-config --exists pixman-1 && echo y) | |
| def_bool $(shell,pkg-config --exists pixman-1 && echo y || echo n) |
| # Dependency detection using Kconfiglib shell function | ||
| config HAVE_SDL2 | ||
| def_bool $(success,pkg-config --exists sdl2) | ||
| def_bool $(shell,pkg-config --exists sdl2 && echo y) |
There was a problem hiding this comment.
If pkg-config fails, $(shell,…) expands to an empty string, so this def_bool line becomes syntactically invalid and breaks Kconfig parsing. Please emit an explicit "n" on failure to keep the default well-formed.
Prompt for AI agents
Address the following comment on configs/Kconfig at line 9:
<comment>If pkg-config fails, $(shell,…) expands to an empty string, so this def_bool line becomes syntactically invalid and breaks Kconfig parsing. Please emit an explicit "n" on failure to keep the default well-formed.</comment>
<file context>
@@ -4,18 +4,18 @@ config CONFIGURED
+# Dependency detection using Kconfiglib shell function
config HAVE_SDL2
- def_bool $(success,pkg-config --exists sdl2)
+ def_bool $(shell,pkg-config --exists sdl2 && echo y)
config HAVE_PIXMAN
</file context>
Suggested change
| def_bool $(shell,pkg-config --exists sdl2 && echo y) | |
| def_bool $(shell,pkg-config --exists sdl2 && echo y || echo n) |
| # (i.e., no unknown goals remain after filtering out known ones) | ||
| ifeq ($(filter-out $(build-goals),$(MAKECMDGOALS)),) | ||
| # All goals are known, include dependencies (except for clean-only builds) | ||
| ifeq ($(filter clean,$(MAKECMDGOALS)),) |
There was a problem hiding this comment.
This condition now skips dependency includes whenever a clean goal is present, so make clean all loses its dependency files. Please only treat clean-only invocations as the skip case.
Prompt for AI agents
Address the following comment on mk/common.mk at line 688:
<comment>This condition now skips dependency includes whenever a clean goal is present, so `make clean all` loses its dependency files. Please only treat clean-only invocations as the skip case.</comment>
<file context>
@@ -681,10 +681,11 @@ __FORCE:
+ # (i.e., no unknown goals remain after filtering out known ones)
+ ifeq ($(filter-out $(build-goals),$(MAKECMDGOALS)),)
+ # All goals are known, include dependencies (except for clean-only builds)
+ ifeq ($(filter clean,$(MAKECMDGOALS)),)
-include $(target-depends)
endif
</file context>
Suggested change
| ifeq ($(filter clean,$(MAKECMDGOALS)),) | |
| ifneq ($(filter-out clean,$(MAKECMDGOALS)),) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the following critical build system issues:
$(success,...)with Kconfiglib's$(shell,...)def_bool $(shell,pkg-config --exists ... && echo y)for dependency detectionSummary by cubic
Fixes critical build system issues by correcting Kconfig dependency detection and tightening target checks. Mixed invocations like "make defconfig all" now work, and builds won’t fail on unknown targets.