Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: ensure LLVM_CMAKE_FLAGS additions are kept #337

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

abrown
Copy link
Collaborator

@abrown abrown commented Jul 27, 2023

In #321, some OSX-specific Makefile additions to LLVM_CMAKE_FLAGS were skipped unless make is run on a Darwin OS. This allowed building wasi-sdk for aarch64. But, as reported in #336, this also broke arm64/x86_64 universal binaries that are built during CI.

The reason for this is that CI's main.yml overrides LLVM_CMAKE_FLAGS to add caching but make will not append to a variable set on the command line. This changes uses the override keyword to append to such a variable, as suggested here.

In WebAssembly#321, some OSX-specific `Makefile` additions to `LLVM_CMAKE_FLAGS`
were skipped unless `make` is run on a Darwin OS. This allowed building
wasi-sdk for aarch64. But, as reported in WebAssembly#336, this also broke
arm64/x86_64 universal binaries that are built during CI.

The reason for this is that CI's `main.yml` overrides `LLVM_CMAKE_FLAGS`
to add caching but `make` will not append to a variable set on the
command line. This changes uses the `override` keyword to append to
such a variable, as suggested [here].

[here]: https://www.gnu.org/software/make/manual/html_node/Override-Directive.html
Copy link
Contributor

@TerrorJack TerrorJack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

til about the override keyword in makefiles, thanks for the fix! macos build log shows -arch arm64 -arch x86_64 in the llvm build flags so i believe this is a valid fix.

@abrown
Copy link
Collaborator Author

abrown commented Jul 27, 2023

Credit to @sunfishcode for recommending override!

@abrown abrown marked this pull request as ready for review July 28, 2023 15:42
@abrown
Copy link
Collaborator Author

abrown commented Jul 28, 2023

Ok, looking at the artifacts for this PR's run, dist-macos-latest now adds up to 118MB prior to a previous run's 65.1MB. That seems to be a good sign that this has fixed #336; @TerrorJack, let me know if doesn't!

@abrown abrown merged commit f3b43c7 into WebAssembly:main Jul 28, 2023
5 checks passed
@abrown abrown deleted the fix-336 branch July 28, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants