-
Notifications
You must be signed in to change notification settings - Fork 5
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
Revive nodejs version #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM!
However I'd like to have a build job on the CI for the emscripten target and at least a very basic test case executed.
crates/lld-sys/build.rs
Outdated
@@ -22,6 +39,90 @@ fn set_rustc_link_flags() { | |||
"LLVMTargetParser", | |||
"LLVMBinaryFormat", | |||
"LLVMDemangle", | |||
// Required by `llvm-sys` | |||
"LLVMWindowsManifest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can hardly be that all of those are required?
crates/solidity/src/lib.rs
Outdated
#[path = "solc/compiler.rs"] | ||
pub(crate) mod compiler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed? I feel like there should be a module solc
containing a module compiler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rust compiler raises a warning with the combination solc::solc
README.md
Outdated
```bash | ||
export EMSDK_ROOT=<PATH_TO_EMSCRIPTEN_SDK> | ||
bash emscripten-build-llvm.sh | ||
source $EMSDK_ROOT/emsdk_env.sh | ||
export LLVM_LINK_PREFIX=${PWD}/llvm18.0-emscripten | ||
make install-wasm | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not complete yet, I tried to build this branch locally on mac OS however the builtins
and stdlib
crates build.rs
fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build.rs
scripts of several crates rely on the LLVM (bin) install dir being added to $PATH
, as it relies on those tools (e.g. llvm-config
the llvm-as
assembler and so on).
If you leave them in $PATH
it'll accidentially work :) I suggest either pointing out that the native LLVM build is required anyways (see Installation
), or adding bash build-llvm.sh && export PATH=${PWD}/llvm18.0/bin:$PATH
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the standard path :
bash build-llvm.sh && export PATH=${PWD}/llvm18.0/bin:$PATH
make install-bin
work for you?
On my pc it does not compile and requires full llvm release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make install-bin
isn't necessary, we just need the LLVM build. I just tested it, works on my machine (TM). Might be a good idea to reproduce it on CI though :)
A simple test is in the second PR. I think we could add the compilation with emscripten target to this PR. WDYT? |
Fine with me to add any tests later but I'd appreciate a basic CI job ensuring that at least the build process of the compiler itself works with this PR :) |
Are the emscripten LLVM build artifacts to be found anywhere? If not we should release them on our own terms, can't justify building them with each run. |
crates/solidity/Cargo.toml
Outdated
default = ["parallel"] | ||
parallel = ["rayon"] | ||
|
||
#TODO: Use pthreads -C target-feature=+atomics,+bulk-memory -Clink-arg=-pthread in compilation and enable `parallel` feature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to resolve this TODO in this PR? If not and it is blocked by some larger issue can you please point out why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't plan to do it now. I spent a couple of hours on it and still had issues where some libraries were compiled without the -pthread
flag.
This PR introduces cross-compilation of revive to WebAssembly (WASM) using Emscripten.
The enhancements made allow the compiler to be executed in a Node.js environment, facilitating broader application and integration possibilities.