Add compilation to NodeJS module #111
Conversation
|
I guess it was an accident that you checked in the build outputs? |
I did it to share the output with @pgherveou |
| branches: ["main"] | ||
| pull_request: | ||
| branches: ["main"] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Do we need the manual dispatch even though we build for every PR and master commit?
There was a problem hiding this comment.
it uses 1day retention period, I left it in case someone would like to recreate artefacts.
| # Install Emscripten | ||
| git clone https://github.com/emscripten-core/emsdk.git | ||
| cd emsdk | ||
| ./emsdk install ${{ env.EMSCRIPTEN_VERSION }} | ||
| ./emsdk activate ${{ env.EMSCRIPTEN_VERSION }} |
There was a problem hiding this comment.
We should check out a specific tag here. Otherwise we build against a different version every time.
There was a problem hiding this comment.
we are using the version defined in EMSCRIPTEN_VERSION env which is set now to the rev fd61bacaf40131f74987e649a135f1dd559aff60
| path: | | ||
| ${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc.js | ||
| ${{ env.REVIVE_WASM_INSTALL_DIR }}/resolc.wasm |
There was a problem hiding this comment.
Are those all the files that are needed? What about the hand written javascript files in /js? Or are you planning to do the full npm package release work flow as follow up?
There was a problem hiding this comment.
I will add npm package with its workflow in next PR. Now we need these 2 files only.
README.md
Outdated
|
|
||
| ### Cross-compilation to WASM | ||
|
|
||
| Cross-compiles the Revive recompiler to WASM to enable it to run in a Node.js or browser environment. |
There was a problem hiding this comment.
| Cross-compiles the Revive recompiler to WASM to enable it to run in a Node.js or browser environment. | |
| Cross-compiles the revive compiler to Wasm for running it in a Node.js or browser environment. |
|
|
||
| # Build LLVM, clang | ||
| LLVM_SRC_PREFIX=${PWD}/llvm-project | ||
| LLVM_SRC_DIR=${LLVM_SRC_PREFIX}/llvm | ||
| LLVM_BUILD_DIR=${PWD}/build/llvm |
There was a problem hiding this comment.
Why is this changed? Before the target build directory was outside the llvm-project source directory
There was a problem hiding this comment.
I have aligned it to this what i have in emscripten build:
LLVM_NATIVE=$LLVM_SRC/build-native
LLVM_WASM=$LLVM_SRC/build-wasm
We can reorganise it if you want
There was a problem hiding this comment.
Yes please I think it is a standard practice to keep the build outside the source, i.e. leave it as it is now.
| @@ -5,17 +5,12 @@ set -euo pipefail | |||
| INSTALL_DIR="${PWD}/llvm18.0" | |||
| mkdir -p ${INSTALL_DIR} | |||
|
|
|||
|
|
|||
| # Clone LLVM 18 (any revision after commit bd32aaa is supposed to work) | |||
| if [ ! -d "llvm-project" ]; then | |||
There was a problem hiding this comment.
Should clone into $LLVM_SRC_PREFIX instead of a hardcoded path (if it goes into a dedicated script it should be an argument instead)
| ) | ||
| })?; | ||
| Ok(output) | ||
| pub trait Process { |
There was a problem hiding this comment.
Trying to understand why is this a trait? Because everywhere else it seems we decide on the configured target. But the trait makes it theoretically possible to use the worker process in native and vice-versa?
There was a problem hiding this comment.
It doesn't need to be a trait now. I was considering introducing a Config trait with associated types for the compiler and process, but I haven't decided to add it yet. it should reduce the number of #cfgs.
There was a problem hiding this comment.
Ah okay, I think both would work but I think the set of targets we want to support is finite so the feature gates should do fine. No hard opinion on which solution, but can you please make it consistent everywhere?
There was a problem hiding this comment.
I don't see any issues with having a trait that provides a common interface for both native and worker processes. Would you like me to remove it? Similarly both SolcCompiler and SoljsonCompiler implement the Compiler trait, but in this case it is already used.
There was a problem hiding this comment.
I'm just trying to understand the use case for the trait? I.e. it seems like we can just have the Wasm version gated behind a feature.
This makes it consistent with the rest of the codebase, where we gate the emscripten specific code behind features.
I guess the trait would be useful if we need to add many more but I can't think of anything else?
If it works and you want to leave it, fine for me. I just noticed this is a bit inconsistent.
Co-authored-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Co-authored-by: Cyrill Leutwiler <bigcyrill@hotmail.com>
Cross-compile the
resolcbinary to WebAssembly (WASM) using Emscripten.