Update build system + minor .gitignore changes#6
Conversation
📝 WalkthroughWalkthroughThe build system is refactored from hardcoded toolchain invocations into a config-driven architecture. A new config.mk centralizes toolchain commands, directories, and flags. The Makefile is restructured to auto-generate per-C-file compilation rules, use variable-based linking and assembly patterns, and relies on config.mk for all paths and commands. ChangesBuild System Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 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.
🧹 Nitpick comments (3)
config.mk (1)
17-19: ⚡ Quick winConsider using patsubst for more robust ASM object naming.
The current approach using
$(subst .,_,...)replaces all dots with underscores, which works for simple filenames but could produce unexpected results if assembly files contain dots in their base names (e.g.,my.boot.asmbecomesmy_boot_asm.o).A more precise alternative:
ASM_OBJS := $(patsubst $(SRC_DIR)/%.asm,$(OBJ_DIR)/%_asm.o,$(ASM_SRC))This transforms only the
.asmextension while preserving any dots in the filename itself.🤖 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 `@config.mk` around lines 17 - 19, The ASM object naming currently uses ASM_OBJS := $(addsuffix .o,$(subst .,_,$(subst $(SRC_DIR),$(OBJ_DIR),$(ASM_SRC)))) which replaces all dots in paths; replace this with a pattern substitution that only strips the .asm extension by mapping $(ASM_SRC) from $(SRC_DIR)/%.asm to $(OBJ_DIR)/%_asm.o (use patsubst) so filenames with dots (e.g., my.boot.asm) keep their dots and only the extension is transformed; update the ASM_OBJS assignment and keep references to ASM_SRC, SRC_DIR and OBJ_DIR.Makefile (2)
5-7: ⚡ Quick winConsider using variables for build paths.
The paths
build/boot/os.binandbuildare hardcoded. For consistency with the config-driven approach, consider defining these inconfig.mk:# In config.mk GRUB_DIR := build GRUB_BOOT_DIR := $(GRUB_DIR)/boot # In Makefile $(OS_ISO): $(OS_BIN) cp $< $(GRUB_BOOT_DIR)/os.bin grub-mkrescue --output=$@ $(GRUB_DIR)This improves maintainability and follows the established pattern.
🤖 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 5 - 7, Define configurable build path variables (e.g., GRUB_DIR and GRUB_BOOT_DIR) in config.mk and use them in the Makefile instead of hardcoded paths; update the $(OS_ISO) rule that currently references build/boot/os.bin and build so that the cp command copies $< to $(GRUB_BOOT_DIR)/os.bin and the grub-mkrescue invocation uses $(GRUB_DIR), keeping the existing targets $(OS_ISO) and $(OS_BIN) unchanged and ensuring the Makefile reads the variables from config.mk.
32-40: ⚡ Quick winAdd .PHONY declarations for non-file targets.
The targets
compile,clean, andrundon't produce files with those names and should be declared as.PHONYto prevent conflicts if files namedcompile,clean, orrunare accidentally created.📝 Proposed addition
Add this line near the top of the Makefile (after the
includestatement):include config.mk +.PHONY: compile clean run + compile: $(OS_ISO)🤖 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 32 - 40, Declare the non-file Makefile targets as phony to avoid name conflicts: add a .PHONY declaration listing compile, clean, and run (for example ".PHONY: compile clean run") near the top of the Makefile (immediately after the include statement) so make treats those targets as always out-of-date rather than as files.
🤖 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.
Nitpick comments:
In `@config.mk`:
- Around line 17-19: The ASM object naming currently uses ASM_OBJS :=
$(addsuffix .o,$(subst .,_,$(subst $(SRC_DIR),$(OBJ_DIR),$(ASM_SRC)))) which
replaces all dots in paths; replace this with a pattern substitution that only
strips the .asm extension by mapping $(ASM_SRC) from $(SRC_DIR)/%.asm to
$(OBJ_DIR)/%_asm.o (use patsubst) so filenames with dots (e.g., my.boot.asm)
keep their dots and only the extension is transformed; update the ASM_OBJS
assignment and keep references to ASM_SRC, SRC_DIR and OBJ_DIR.
In `@Makefile`:
- Around line 5-7: Define configurable build path variables (e.g., GRUB_DIR and
GRUB_BOOT_DIR) in config.mk and use them in the Makefile instead of hardcoded
paths; update the $(OS_ISO) rule that currently references build/boot/os.bin and
build so that the cp command copies $< to $(GRUB_BOOT_DIR)/os.bin and the
grub-mkrescue invocation uses $(GRUB_DIR), keeping the existing targets
$(OS_ISO) and $(OS_BIN) unchanged and ensuring the Makefile reads the variables
from config.mk.
- Around line 32-40: Declare the non-file Makefile targets as phony to avoid
name conflicts: add a .PHONY declaration listing compile, clean, and run (for
example ".PHONY: compile clean run") near the top of the Makefile (immediately
after the include statement) so make treats those targets as always out-of-date
rather than as files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a141e15-0e98-41d4-9419-efe0a36aed2f
📒 Files selected for processing (6)
.GITIGNORE.gitignoreMakefileconfig.mkobj/.gitignoreout/.gitignore
💤 Files with no reviewable changes (1)
- .GITIGNORE
1 participant
Implemented advanced build system.
Removed previous static rules and replaced them with dynamic rule generation.
This system makes sure that only the files dependent on the changes are actually recompiled.
Instead of recreating the directories for objects at each compilation, we keep them empty with .gitignore files.
Summary by CodeRabbit
Release Notes