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

feat(makefile) Simplify the prelude + build-capi includes all available compilers #2123

Merged

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Feb 15, 2021

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.

  1. adb7c59, 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.

  2. 659bdb0, This patch removes a possible duplication of the llvm value inside
    the compilers variable if both LLVM 10 and 11 are available.

    This patch also checks for LLVM 11 before LLVM 10.

  3. 7b38752, This patch moves all the assignments of compilers in the same place
    for the sake of clarity.

  4. 006c653, This patch renames the test_compilers_engines variable to
    compilers_engines. We can use this variable for test purposes, but more
    importantly, 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.

  5. 67a1dfe, This patch handles the (cranelift, *) pairs in a single place.

  6. f3c729d, This patch handles the (llvm, *) pairs in a single place.

  7. 1054e51, This patch handles the (singlepass, *) pairs in a single place.

  8. 07e48b4 , This patch adds the HAS_<compiler> variables, it makes the code
    easier to read.

  9. 06d543a, 660daba, 95dcb04, 6721769, and 62c7889 are small improvements.

  10. 0a0b7f8, This patch introduces ENABLE_<compiler> variables, that can be
    overriden by the user to force to enable or to disable a compiler. They replace
    the HAS_<compiler> variables.

Part 2, libwasmer includes all available compilers

  1. 387a753, This patch updates the build-capi rule to no longer uses Cranelift by default,
    but to include/enable all the available compilers.

  2. 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 to
    build 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 giving
    to the users! That's what test-capi-all does. Notice that the test-capi rule depends
    test-capi-all, so we don't need to update the CI.

Closes #2063

Review

  • Add a short description of the the change to the CHANGELOG.md file

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.
@Hywan Hywan changed the title feat(makefile) Simplify conditions with IS_* bool variables. feat(makefile) Simplify, improve, and clean (yup…) Feb 15, 2021
@Hywan Hywan force-pushed the feat-c-api-build-lib-with-all-compilers-and-engines branch from 07e48b4 to ba3fdf1 Compare February 15, 2021 12:57
@Hywan Hywan changed the title feat(makefile) Simplify, improve, and clean (yup…) feat(makefile) Simplify, improve, and clean (yup…) + build-capi includes all available compilers Feb 15, 2021
@Hywan Hywan self-assigned this Feb 15, 2021
@Hywan Hywan added 📚 documentation Do you like to read? 📦 lib-c-api About wasmer-c-api 🤖 bot Bip bip 🧪 tests I love tests labels Feb 15, 2021
@Hywan Hywan marked this pull request as ready for review February 15, 2021 14:05
@Hywan
Copy link
Contributor Author

Hywan commented Feb 15, 2021

bors try

bors bot added a commit that referenced this pull request Feb 15, 2021
@bors
Copy link
Contributor

bors bot commented Feb 15, 2021

try

Build failed:

… github.com:Hywan/wasmer into feat-c-api-build-lib-with-all-compilers-and-engines
@Hywan
Copy link
Contributor Author

Hywan commented Feb 16, 2021

@MarkMcCaskey With ed34e48, LLVM is always removed from the C API when built with make build-capi or make test-capi.

@wasmerio wasmerio deleted a comment from bors bot Feb 16, 2021
@wasmerio wasmerio deleted a comment from bors bot Feb 16, 2021
@Hywan
Copy link
Contributor Author

Hywan commented Feb 16, 2021

bors try

@bors
Copy link
Contributor

bors bot commented Feb 16, 2021

try

Already running a review

@Hywan
Copy link
Contributor Author

Hywan commented Feb 16, 2021

@Hywan
Copy link
Contributor Author

Hywan commented Feb 16, 2021

PR is green, modulo our sccache issue on Windows.

@nlewycky
Copy link
Contributor

Update to pick up fix to sccache issue.

@nlewycky
Copy link
Contributor

bors try

@bors
Copy link
Contributor

bors bot commented Feb 16, 2021

try

Already running a review

@syrusakbary
Copy link
Member

bors try

@bors
Copy link
Contributor

bors bot commented Feb 17, 2021

try

Already running a review

@syrusakbary
Copy link
Member

bors try-

@syrusakbary
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Feb 17, 2021
@syrusakbary
Copy link
Member

Not reviewing any of the specifics, just want to say again that I don't think we can ship libwasmer with LLVM until we fix the linking issues regarding LLVM. Otherwise users will need zlib, curses, and potentially other libraries even when not building against LLVM. If this is an unfixable problem, which I don't think anyone is saying it is, compiler-llvm (or maybe every compiler) should be shipped in its own static library.

@jubianchi can you check if that still the case (zlib/curses requirement when embedding LLVM in the wasmer c api)?
I thought we fixed by using our custom llvm builds, but that might not be the case

@bors
Copy link
Contributor

bors bot commented Feb 17, 2021

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2021

bors try

bors bot added a commit that referenced this pull request Feb 18, 2021
@bors
Copy link
Contributor

bors bot commented Feb 18, 2021

@Hywan
Copy link
Contributor Author

Hywan commented Feb 18, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 18, 2021

@bors bors bot merged commit 2d94189 into wasmerio:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 bot Bip bip 📚 documentation Do you like to read? 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants