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

feat: Added Makefile for simpler build process #38

Merged
merged 5 commits into from
May 19, 2022
Merged

Conversation

ChaoticTempest
Copy link
Member

@ChaoticTempest ChaoticTempest commented May 18, 2022

Added the Makefile for a simpler build process for users trying to build the SDK.

@ailisp I never got the reason why we went with C, since I was thinking we could've reused the Rust near-sys crate to bind the host functions instead of rewriting it all in C. There's probably a reason behind it, but if we continue to go down the route of using C, we should have a proper build system such as cmake or meson, so people can build the SDK more readily instead of having to manually install dependencies and run into reproducibility issues

@ChaoticTempest ChaoticTempest requested review from volovyks and ailisp May 18, 2022 21:36
@ailisp
Copy link
Member

ailisp commented May 19, 2022

@ChaoticTempest jsvm.wasm is essentially a JavaScript runtime plus injected near host functions. The JavaScript runtime is modified QuickJS, which is written in C. To inject near host functions to QuickJS, it also need to be done in C

we could've reused the Rust near-sys crate to bind the host functions instead of rewriting it all in C

It's actually not rewriting. It's dealing with QuickJS's encoding/decoding of argument and pass it to near host functions. We don't even need to reuse near-sys crate: in Rust world, a *-sys crate is required as a foreign function interface. In C, simply extern to import them:

extern void read_register(uint64_t register_id, uint64_t ptr);

@ailisp
Copy link
Member

ailisp commented May 19, 2022

Looks good to me, I update it with @volovyk-s 's new project layout

Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @ChaoticTempest! Thanks for the update @ailisp .
It's already good for merging, but we will need to add the ARM (M1) build artifact to the res folder (I will need your help with that).

@volovyks
Copy link
Collaborator

@itegulov thanks for the help!

@volovyks volovyks merged commit 366e33b into master May 19, 2022
@ChaoticTempest ChaoticTempest deleted the feat/makefile branch May 19, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants