-
Notifications
You must be signed in to change notification settings - Fork 14
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: compile to WebAssembly to eliminate native dependencies #8
Conversation
563f4a7
to
5b95dfe
Compare
Some CI jobs are failing for various reasons currently:
|
For testing this PR locally, it's useful to use |
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 going to enable us to generate a single postject.wasm
file that would run on all platforms?
This change updates lief to lief-project/LIEF@b183666. This contains lief-project/LIEF#780, which is a requirement for #8. Signed-off-by: Darshan Sen <[email protected]>
patches/lief/0001-fix-change-variable-types-to-prevent-64-bit-to-32-bit-conversion.patch
Outdated
Show resolved
Hide resolved
This change updates lief to lief-project/LIEF@b183666. This contains lief-project/LIEF#780, which is a requirement for #8. Signed-off-by: Darshan Sen <[email protected]>
This change updates lief to lief-project/LIEF@b183666. This contains lief-project/LIEF#780, which is a requirement for #8. Signed-off-by: Darshan Sen <[email protected]>
See these lines in |
Okay, I was wondering because we were generating separate |
0ddb353
to
22d52cc
Compare
22d52cc
to
eb32deb
Compare
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.
did a light round of review on things that are unlikely to drastically change in further commits
cf76258
to
ae6e5c9
Compare
Ready for final review. A couple of things will be done in follow-up PRs:
|
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.
LGTM
@dsanders11 Can you respond to #8 (comment)?
This would be needed for #14, merging. Maybe my comment will get addressed while resolving that issue? |
@RaisinTen, I'm not sure what the proposed "unifying" would look like - the build jobs are still going to generate the artifacts regardless. I don't think it hurts to publish them for all build jobs, and if there's ever a discrepancy between them it would be easy to compare. Since the artifacts are the same for all platforms, we should just publish from whichever platform is most convenient. |
@dsanders11 yes, that's exactly what I'm doing in #30. I chose linux. :) |
Still WIP, but this is a major rewrite of the project to use Emscripten to compile to WebAssembly and ship as an npm package. Roughly I'd say the current state is 80% there.
Needs
emsdk
to be installed on the system to work. I considered adding it to thevendorpull
config, but that repo is over a gigabyte so for now leaving it as an external development dependency.The basics machinery works, although it has only been tested on macOS so far
, and the test is currently failing. I think there may be a LIEF issue causing that, as there are some codepaths which trying to detect the system and use that to determine page size for example, but further investigation needs to be done to determine root cause.(Root cause was in LIEF, patch created and PR opened upstream).There's a couple of high level goals driving the decisions in this PR:
zx
module instead)mocha
in this case) to allow for more detailed test coverageStill to do:
Closes #10, #15, #16, #17.