Allow macOS build #556
Merged
Allow macOS build #556
Conversation
This file contains 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
Changelist by BitoThis pull request implements the following key changes. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
jserv
force-pushed
the
allow-macos-build
branch
from
3849ae8 to
9ea34d5
Compare
jserv
force-pushed
the
allow-macos-build
branch
from
9ea34d5 to
357add2
Compare
Code Review Agent Run #ad92c8Actionable Suggestions - 0Review Details
AI Code Review powered by |
jserv
commented
Feb 6, 2025
jserv
force-pushed
the
allow-macos-build
branch
from
357add2 to
80678a5
Compare
Code Review Agent Run #a698c2Actionable Suggestions - 0Review Details
AI Code Review powered by |
jserv
force-pushed
the
allow-macos-build
branch
from
80678a5 to
d738bfc
Compare
Code Review Agent Run #6df34bActionable Suggestions - 0Review Details
AI Code Review powered by |
jserv
force-pushed
the
allow-macos-build
branch
from
d738bfc to
8da8f0e
Compare
Code Review Agent Run #805c88Actionable Suggestions - 1
Review Details
AI Code Review powered by |
ChinYikMing
reviewed
Feb 11, 2025
Makefile
Outdated
| $(PRINTF) "Running $(3) ... "; \ | ||
| OUTPUT_FILE="$$(mktemp)"; \ | ||
| if (LC_ALL=C $(BIN) $(1) $(2) > "$$OUTPUT_FILE") && \ | ||
| [ "$$(cat "$$OUTPUT_FILE" | $(LOG_FILTER) | $(4))" = "$(strip $(5))" ]; then \ |
There was a problem hiding this comment.
No need $(strip)? Since it truncates the whitespace in the sha1sum output of the misalign test when running uaes.elf, causing the test to fail."
There was a problem hiding this comment.
If $(strip) is removed, the last argument of line 380, 383, 387, 391, 394 should be refined by removing the whitespace after the comma to pass the test.
There was a problem hiding this comment.
diff --git a/Makefile b/Makefile
index d2ab6f2..a0224cd 100644
--- a/Makefile
+++ b/Makefile
@@ -367,7 +367,7 @@ $(Q)true; \
$(PRINTF) "Running $(3) ... "; \
OUTPUT_FILE="$$(mktemp)"; \
if (LC_ALL=C $(BIN) $(1) $(2) > "$$OUTPUT_FILE") && \
- [ "$$(cat "$$OUTPUT_FILE" | $(LOG_FILTER) | $(4))" = "$(strip $(5))" ]; then \
+ [ "$$(cat "$$OUTPUT_FILE" | $(LOG_FILTER) | $(4))" = "$(5)" ]; then \
$(call notice, [OK]); \
else \
$(PRINTF) "Failed.\n"; \
@@ -377,22 +377,22 @@ $(RM) "$$OUTPUT_FILE"
endef
check-hello: $(BIN)
- $(call check-test, , $(OUT)/hello.elf, hello.elf, uniq, $(EXPECTED_hello))
+ $(call check-test, , $(OUT)/hello.elf, hello.elf, uniq,$(EXPECTED_hello))
check: $(BIN) check-hello artifact
- $(Q)$(foreach e, $(CHECK_ELF_FILES), $(call check-test, , $(OUT)/riscv32/$(e), $(e), uniq, $(EXPECTED_$(e))))
+ $(Q)$(foreach e, $(CHECK_ELF_FILES), $(call check-test, , $(OUT)/riscv32/$(e), $(e), uniq,$(EXPECTED_$(e))))
EXPECTED_aes_sha1 = 89169ec034bec1c6bb2c556b26728a736d350ca3 -
misalign: $(BIN) artifact
- $(call check-test, -m, $(OUT)/riscv32/uaes, uaes.elf, $(SHA1SUM), $(EXPECTED_aes_sha1))
+ $(call check-test, -m, $(OUT)/riscv32/uaes, uaes.elf, $(SHA1SUM),$(EXPECTED_aes_sha1))
EXPECTED_misalign = MISALIGNED INSTRUCTION FETCH TEST PASSED!
misalign-in-blk-emu: $(BIN)
- $(call check-test, , tests/system/alignment/misalign.elf, misalign.elf, tail -n 1, $(EXPECTED_misalign))
+ $(call check-test, , tests/system/alignment/misalign.elf, misalign.elf, tail -n 1,$(EXPECTED_misalign))
EXPECTED_mmu = STORE PAGE FAULT TEST PASSED!
mmu-test: $(BIN)
- $(call check-test, , tests/system/mmu/vm.elf, vm.elf, tail -n 1, $(EXPECTED_mmu))
+ $(call check-test, , tests/system/mmu/vm.elf, vm.elf, tail -n 1,$(EXPECTED_mmu))
# Non-trivial demonstration programs
ifeq ($(call has, SDL), 1)
There was a problem hiding this comment.
After applying the proposed changes to allow build on macOS and the above diff of Makefile, the default tests passed on both host-x64 and macOS-arm64, check CI. Note that the macOS CI currently excludes the gdbstub-test and arch-test because the RISC-V toolchain cannot be installed using the existing scripts (will try to fix it).
ChinYikMing
reviewed
Feb 11, 2025
| @@ -10,6 +10,8 @@ ifeq ($(UNAME_M),x86_64) | |||
| HOST_PLATFORM := x86 | |||
| else ifeq ($(UNAME_M),aarch64) | |||
| HOST_PLATFORM := aarch64 | |||
| else ifeq ($(UNAME_M),arm64) # macOS | |||
| HOST_PLATFORM := arm64 | |||
There was a problem hiding this comment.
There is no Sail model with suffix arm64 currently (Check sail model in rv32emu-prebuilt). I would defer the integration of arch-test on masOS CI.
Commit 2ab8a68 failed to detect macOS/Arm64 properly, and commit 2e87a75 relied on the .SHELLSTATUS feature introduced in GNU make 4.2 (released on 2016-05-22). However, macOS ships with GNU make 3.81 by default. This commit fixes the macOS/Arm64 detection issue and runs everything in a single shell invocation, capturing both the exit code and program output without relying on '.SHELLSTATUS'. Co-authored-by: ChinYikMing <[email protected]>
jserv
force-pushed
the
allow-macos-build
branch
from
8da8f0e to
b23860d
Compare
Code Review Agent Run #8c8988Actionable Suggestions - 0Review Details
AI Code Review powered by |
|
Thank @ChinYikMing for contributing! |
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.
Commit 2ab8a68 failed to detect macOS/Arm64 properly, and commit 2e87a75 relied on the
.SHELLSTATUSfeature introduced in GNU make 4.2 (released on 2016-05-22). However, macOS ships with GNU make 3.81 by default, lacking of.SHELLSTATUS.This commit fixes the macOS/Arm64 detection issue and reworks the build commands to run everything in a single shell invocation, capturing both the exit code and program output without relying on
.SHELLSTATUS.Summary by Bito
This PR enhances macOS build compatibility through two main changes: implementing ARM64 architecture detection and refactoring Makefile command execution for better compatibility with macOS GNU make 3.81. The changes focus on maintaining existing functionality while improving cross-platform support.Unit tests added: False
Estimated effort to review (1-5, lower is better): 1