-
Notifications
You must be signed in to change notification settings - Fork 833
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
feat(makefile) Simplify the prelude + build-capi
includes all available compilers
#2123
feat(makefile) Simplify the prelude + build-capi
includes all available compilers
#2123
Conversation
This patch removes the `UNAME_S` and `ARCH` variables. It replaces them with `IS_WINDOWS, `IS_LINUX`, `IS_DARWIN, `IS_AMD64`, and `IS_AARCH64` variables. It makes the code easier to read and simpler to maintain. This patch also documents the large matrix of platform x architecture x compiler x engine x lib support.
…llvm` value inside `compilers`. This patch removes a possible duplication of the `llvm` value insode the `compilers` variable if both LLVM 10 and 11 are available. This patch also checks for LLVM 11 before LLVM 10.
IS_*
bool variables.07e48b4
to
ba3fdf1
Compare
build-capi
includes all available compilers
bors try |
tryBuild failed: |
… github.com:Hywan/wasmer into feat-c-api-build-lib-with-all-compilers-and-engines
@MarkMcCaskey With ed34e48, LLVM is always removed from the C API when built with |
bors try |
tryAlready running a review |
PR is green, modulo our sccache issue on Windows. |
Update to pick up fix to sccache issue. |
bors try |
tryAlready running a review |
bors try |
tryAlready running a review |
bors try- |
bors try |
@jubianchi can you check if that still the case (zlib/curses requirement when embedding LLVM in the wasmer c api)? |
tryBuild failed: |
… github.com:Hywan/wasmer into feat-c-api-build-lib-with-all-compilers-and-engines
bors try |
bors r+ |
Description
This PR contains 2 parts. Don't be afraid, it's very small changes, and it's easy to follow.
How to Review
I recommend to review the PR patch by patch, it's way simpler than reading the final
diff
.Note that the matrix (the table in the
Makefile
documentation) is updated patch after patch,so this part should ideally be reviewed in the final
diff
.The final
Makefile
is here, https://github.com/Hywan/wasmer/blob/feat-c-api-build-lib-with-all-compilers-and-engines/Makefile, and it's even easier to read now.Part 1, The Matrix
The set of patches writes a matrix: platform x architecture x compiler x engine x libc, and for each combination, says if it's supported or not.
This set of patches updates the “prelude” of the
Makefile
to make it clearler and easier to read, regarding the matrix.adb7c59, This patch removes the
UNAME_S
andARCH
variables. It replacesthem with
IS_WINDOWS
,IS_LINUX
,IS_DARWIN
,IS_AMD64
, andIS_AARCH64
variables. It makes the code easier to read and simplerto maintain.
This patch also documents the large matrix of platform x architecture
x compiler x engine x lib support.
659bdb0, This patch removes a possible duplication of the
llvm
value insidethe
compilers
variable if both LLVM 10 and 11 are available.This patch also checks for LLVM 11 before LLVM 10.
7b38752, This patch moves all the assignments of
compilers
in the same placefor the sake of clarity.
006c653, This patch renames the
test_compilers_engines
variable tocompilers_engines
. We can use this variable for test purposes, but moreimportantly, it defines all the pair (compiler, engine), make it that clear.
I've decided to separate this patch from the followings to facilitate the review.
67a1dfe, This patch handles the (cranelift, *) pairs in a single place.
f3c729d, This patch handles the (llvm, *) pairs in a single place.
1054e51, This patch handles the (singlepass, *) pairs in a single place.
07e48b4 , This patch adds the
HAS_<compiler>
variables, it makes the codeeasier to read.
06d543a, 660daba, 95dcb04, 6721769, and 62c7889 are small improvements.
0a0b7f8, This patch introduces
ENABLE_<compiler>
variables, that can beoverriden by the user to force to enable or to disable a compiler. They replace
the
HAS_<compiler>
variables.Part 2,
libwasmer
includes all available compilers387a753, This patch updates the
build-capi
rule to no longer uses Cranelift by default,but to include/enable all the available compilers.
f846a0d, This patch explains
test-capi
with a little bit more details.This patch also introduces the
test-capi-all
rule. Why?test-capi
will continue tobuild and to test the C API with a single compiler enabled, so that it's the default one
that will be used by the tests. But we also want to test the
libwasmer
we are givingto the users! That's what
test-capi-all
does. Notice that thetest-capi
rule dependstest-capi-all
, so we don't need to update the CI.Closes #2063
Review