[cmake] add FindWasm.cmake and binaryen code into the repo #1412
Conversation
asiniscalchi
changed the title
There was a problem hiding this comment.
this will also conflict with #1452 which also introduces a FindBinaryen.cmake with additional need.
Depending on which one makes it through first this may need revision.
CMakeModules/wasm.cmake
Outdated
| # TODO: Check if compiler is able to generate wasm32 | ||
| if( NOT ("${WASM_CLANG}" STREQUAL "" OR "${WASM_LLC}" STREQUAL "" OR "${WASM_LLVM_LINK}" STREQUAL "" OR NOT BINARYEN_BIN) ) | ||
| set(WASM_TOOLCHAIN TRUE) | ||
| if(WASM_FOUND AND BINARYEN_FOUND) |
There was a problem hiding this comment.
Given that find_package has Wasm and Binaryen as required, is there any purpose to this conditional or the dependent one in the root CMakeLists.txt that handles "soft" failure of the wasm toolchain?
There was a problem hiding this comment.
It dependes on what you want: master branch already fails if toolchain is not found.
What do you prefer ?
| @@ -1,4 +1,4 @@ | |||
| if(WASM_TOOLCHAIN) | |||
| if(WASM_TOOLCHAIN_FOUND) | |||
There was a problem hiding this comment.
At this point, there is no "successful" cmake run where WASM_TOOLCHAIN_FOUND would be false. Most of these conditionals can be removed in favor of running the side of the branch that corresponds with success.
There was a problem hiding this comment.
yes. I was trying to mimic the master branch behavior
|
So in the end the question is: |
|
For a while, it was mandatory as the chain library needed compilation for the system contract. I suppose at this point it could return to optional but we have already leaned in to using WASM to exercise the chain in our integration/smoke tests. It is impossible to test some parts of the chain functionality without contracts working. TL;DR we should assume its mandatory and strip out anything thats now unnecessary as a result. Also, the OSX build needs love, even if its only changing the scripts to properly interface with your new modules |
|
Done. |
asiniscalchi
changed the title
|
I tried to run this through our build scripts which rely on (among otherthings) WASM_LLVM_CONFIG instead of WASM_ROOT please consider either maintaining WASM_LLVM_CONFIG as a single entry for configuring all endpoints or changing the build scripts to properly use WASM_ROOT |
|
#1479 has internalized binaryen to our repo to address issues across several PRs, I would still like to integrate your FindWasm changes but we need to address the mismatch with the build scripts. |
|
I didn't find any other WASM_LLVM_CONFIG in the repo |
it allows the cmake syntax:
find_package(Binaryen)
find_package(Wast)
and the cmake standard syntax
$ cmake .. -DWAST_ROOT=<path> -DBINARYEN_ROOT=<path>