-
Notifications
You must be signed in to change notification settings - Fork 160
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
Don't build wasm-demo in build
target + ci job to run the wasm demo
#1393
Conversation
build
targets + add target & ci job to run the wasm demobuild
targets + add target & ci job to run the wasm demo
build
targets + add target & ci job to run the wasm demobuild
target + add target & ci job to run the wasm demo
Codecov Report
@@ Coverage Diff @@
## main #1393 +/- ##
=======================================
Coverage 96.96% 96.96%
=======================================
Files 94 94
Lines 38635 38635
=======================================
Hits 37463 37463
Misses 1172 1172 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Benchmark Results for unmodified programs 🚀
|
Makefile
Outdated
npm install live-server && \ | ||
npx live-server |
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.
These are new deps AFAICT. We should add them both to our deps targets/scripts and to the docs.
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.
These dependencies are only needed for the wasm-demo, which is not an integral part of the project, so I wouldn't force its dependencies for any user of the project (the main purpose of this PR is to separate it's build from the project's build). There is a separate README with instructions on how to build & run the demo, and its dependencies, so we could remove this target altogether
build
target + add target & ci job to run the wasm demobuild
target + ci job to run the wasm demo
@@ -6,6 +6,11 @@ members = [ | |||
"hint_accountant", | |||
"examples/wasm-demo", | |||
] | |||
default-members = [ |
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.
Just to check @fmoletta
Why are we adding this default-members?
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 problem we want to fix is that the build fails due to the wasm-demo needing the compiled cairo files in order to build.
Someone that wants to build the cairo-vm crate shouldn't have to compile a cairo file in order to build a demo for the vm crate to build.
Therefore, by setting the default members we can build the core crates (vm, cli & felt) with cargo/make build without building the wasm demo and running into compilation errors
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!!
Aims to fix #1392 .
Checklist