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

Makefile checks for LLVM 10, but documentation recommends LLVM 11. #1981

Closed
TylerNowicki opened this issue Jan 5, 2021 · 3 comments · Fixed by wasmerio/old-docs.wasmer.io#94 or #2008
Assignees
Labels
bug Something isn't working 📚 documentation Do you like to read?

Comments

@TylerNowicki
Copy link

The makefile performs this check at line 22

ifneq (, $(findstring 10,$(LLVM_VERSION)))

But if we use LLVM 11 this will fail. Yet, documentation recommends LLVM 11

https://docs.wasmer.io/ecosystem/wasmer/building-from-source#llvm-compiler

The correct way to do this is to parse the major and minor versions and compare them separately.

See: https://stackoverflow.com/questions/3728372/version-number-comparison-inside-makefile

@TylerNowicki TylerNowicki added the bug Something isn't working label Jan 5, 2021
@syrusakbary
Copy link
Member

Your comment is completely on point, @TylerNowicki!

We did the comparison that way because we initially only supported one version of LLVM.
Would you like to help us with a PR improving the Makefile?

@Hywan
Copy link
Contributor

Hywan commented Jan 5, 2021

Actually, wasmer-compiler-llvm needs LLVM 10. We don't support LLVM 11 for the moment. It seems to be an error from the documentation, can you confirm @jubianchi @nlewycky please?

@Hywan Hywan self-assigned this Jan 5, 2021
@Hywan Hywan added the 📚 documentation Do you like to read? label Jan 5, 2021
@TylerNowicki
Copy link
Author

TylerNowicki commented Jan 6, 2021

I noticed in the docs here

https://github.com/wasmerio/wasmer/tree/master/lib/compiler-llvm

under Requirements "It currently requires LLVM 10."

bors bot added a commit that referenced this issue Jan 12, 2021
2008: Improved Apple Silicon & LLVM 11 support r=syrusakbary a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description
This PR:
* [x] Adds support for LLVM 11
* [x] Adds support for llvm-native in Apple Silicon
* [x] Sign the `wasmer` and `wapm` binaries in macOS (so they can run smoothly upon installation)

This PR fix #1984 and fix #1981.

<!-- 
Provide details regarding the change including motivation,
links to related issues, and the context of the PR.
-->

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Syrus Akbary <[email protected]>
@bors bors bot closed this as completed in 3328277 Jan 12, 2021
@bors bors bot closed this as completed in #2008 Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📚 documentation Do you like to read?
Projects
None yet
3 participants