Fix Makefile errors that prevent builds on macOS #409
Merged
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
Back in January 2023, some changes were made to the Makefile to drop some flags that aren't available in macOS builds, that were causing those builds to fail. Unfortunately, these changes had a few small syntax issues that prevent the conditional checks for "Darwin" from ever being true. See commits 0e19696, d011eea, 0737d53. Specifically, a trailing comment after a variable assignment with spaces before the comment character actually has the effect of adding those trailing spaces to the variable being assigned (`detected_OS`). The subsequent line attempts to strip this spaces, but inadvertently adds them back with another trailing comment. Further, the quotes around these assignments added in commit 0737d53 also end up verbatim in the variable's value. The consequence of this the comparison actually ends up comparing `"Darwin" ` with `Darwin`, so the check never succeeds and the wrong flags are used. You can confirm this by adding echo .$(detected_OS). to the `all` target and running make. You'll see the quotes and extra spaces between the dots when the echo command is written to the console (the quotes are eaten by the shell when echo is executed). The fix is to remove the unnecessary quotes, the comments, and the now unnecessary call to `strip`. I went a bit further and removed `detected_OS` altogether since it's only used in one place. I tested these changes on macOS 14.4 and Ubuntu Linux 22.04.4 LTS
|
The quotes were added to facilitate WSL support, I'll give it a test on WSL and get back to you |
|
I see. Maybe I was overzealous in my quote removal. I can revert those and limit the changes to just the Darwin test.
|
|
Leave it as is for now and i will give it a test on wsl |
|
Sorry it took me so long to test this! I can confirm it still builds on my WSL instance |
|
Would you be able to tweak changelog.md to the PR title and link with the current date? Other than that it looks good to me :) |
|
Sure thing. Thanks! |
|
Thanks! |
alok
added a commit
to alok/Adv360-Pro-ZMK
that referenced
this pull request
Apr 8, 2024
…ro-ZMK * 'V3.0' of https://github.com/KinesisCorporation/Adv360-Pro-ZMK: Fix Makefile errors that prevent builds on macOS (KinesisCorporation#409) Fix version.dtsi is reset after local firmware build (KinesisCorporation#385) Update build workflows (KinesisCorporation#376)
fmgrotepass
added a commit
to fmgrotepass/Adv360-Pro-ZMK
that referenced
this pull request
Apr 9, 2024
Fix Makefile errors that prevent builds on macOS (KinesisCorporation#409)
ztomer
pushed a commit
to ztomer/Adv360-Pro-ZMK
that referenced
this pull request
Apr 16, 2024
ztomer
pushed a commit
to ztomer/Adv360-Pro-ZMK
that referenced
this pull request
Apr 16, 2024
gabrielfreiberg
added a commit
to gabrielfreiberg/Adv360-Pro-ZMK
that referenced
this pull request
May 5, 2024
* upstream_V3.0: Changelog update (KinesisCorporation#448) Minor docs update (KinesisCorporation#445) Document new layer colors and modifier color configuration option (KinesisCorporation#431) Zephyr 3.5 Update (KinesisCorporation#426) Fix Makefile errors that prevent builds on macOS (KinesisCorporation#409) Fix version.dtsi is reset after local firmware build (KinesisCorporation#385) Update build workflows (KinesisCorporation#376)
gabrielfreiberg
added a commit
to gabrielfreiberg/Adv360-Pro-ZMK
that referenced
this pull request
May 5, 2024
* main: Changelog update (KinesisCorporation#448) Minor docs update (KinesisCorporation#445) Document new layer colors and modifier color configuration option (KinesisCorporation#431) Zephyr 3.5 Update (KinesisCorporation#426) Fix Makefile errors that prevent builds on macOS (KinesisCorporation#409) Fix version.dtsi is reset after local firmware build (KinesisCorporation#385) Update build workflows (KinesisCorporation#376)
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.
What's changed:
Remove unnecessary quotes from variable assignments, errant comments, and an unnecessary call to
strip. I also inlined the variabledetected_OSsince it's only used in one place.Why has this change been implemented:
Back in January 2023, some changes were made to
Makefileto drop some flags that aren't available in macOS builds that were causing those builds to fail.Unfortunately, these changes had a few small syntax issues that prevent the conditional checks for "Darwin" from ever being true. See commits 0e19696, d011eea, 0737d53.
Specifically, a trailing comment after a variable assignment with spaces before the comment character actually has the effect of adding those trailing spaces to the variable being assigned (
detected_OS). The subsequent line attempts to strip these spaces, but inadvertently adds them back with another trailing comment. Further, the quotes around these assignments added in commit 0737d53 also end up verbatim in the variable's value. The consequence of this the comparison actually ends up comparing"Darwin"withDarwin, so the check never succeeds and the wrong flags are used. You can confirm this by addingto the
alltarget and running make. You'll see the quotes and extra spaces between the dots when the echo command is written to the console (the quotes are eaten by the shell when echo is executed).What (if any) actions must a user take after this change:
No user actions necessary. I tested these changes on macOS 14.4 and Ubuntu Linux 22.04.4 LTS