Build in debug mode by default #15
Conversation
maximebedard
force-pushed
the
use-profile-env
branch
4 times, most recently
from
ba6117a to
97b2a69
Compare
They are not, but perhaps a custom script ( EDIT: Sorry, I misunderstood what you meant by release build, I thought you were referring to releasing (distributing) the CLI, not the release profile for compiling. |
crates/cli/build.rs
Outdated
| let mut engine_path = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap()); | ||
| engine_path.pop(); | ||
| engine_path.pop(); | ||
| let engine_path = engine_path.join("target/wasm32-wasi/debug/javy_core.wasm"); |
There was a problem hiding this comment.
I believe the debug mode shouldn't be hardcoded here? Instead, it should be parametrized? Having the ability to specify release makes sense for benchmarking mostly (even if we are not concerned about actually releasing)
There was a problem hiding this comment.
This will be helpful to locally install the cli too via cargo install --path crates/cli for local testing
There was a problem hiding this comment.
It should probably be configurable though and not necessarily rely on the build profile of a different crate. We could potentially just export a different env variable that points to the engine we want. WDYT?
There was a problem hiding this comment.
That works too, I believe that as long as we can build release and debug profiles without too much trouble we can go with whatever approach for now. If you feel that the env var works let's do that 👍
On a more personal note, I believe that artifact dependencies are a thing and should be the norm, in the same way that you depend on .a or .so files generated by other crates that are statically linked by a final executable for example. I hope that this rfc gets implemented soon which will make all this process easier and standard.
There was a problem hiding this comment.
The point that I'm trying to make is, in which case would it make sense to have core be built in debug mode and the cli built in release mode or vice-versa? Even though core is not listed in the CLI's Cargo.toml as a dependency, it is still a dependency like any other, it's just that there's no standard way to express it. At least not until the RFC linked above is finalized. But again, we can use an env var if you feel that it's better.
There was a problem hiding this comment.
Yes, with that RFC implemented in rust stable it would have made our lives much, much easier. For the meantime, I can still use the PROFILE env variable and hope that the crate has build this artifact properly first.
| @@ -2,28 +2,32 @@ | |||
| .DEFAULT_GOAL := cli | |||
|
|
|||
| cli: core | |||
| cd crates/cli && cargo build --release | |||
| cd crates/cli && cargo build && cd - | |||
There was a problem hiding this comment.
I wonder if we should put back the release option here? Maybe it's not as critical, I don't feel strongly but sometimes there's a need to check the release profile for debugging. Alternatively we can just manually run the commands, I believe that this is less critical than my comment above (about allowing to copy the release binary)
There was a problem hiding this comment.
I would try to build everything in debug mode for now, it just makes local development so much faster since we don't have to go through the many optimization layers.
maximebedard
force-pushed
the
use-profile-env
branch
from
97b2a69 to
038c81a
Compare
|
@saulecabrera all good if I merge this one as is right now? I think it's pretty much a noop right now, except I'm running the optimizations just in case at build time. It mainly just makes the build script a bit simpler at this point... |
maximebedard
force-pushed
the
use-profile-env
branch
from
302e50d to
d887868
Compare
Instead of always compiling in release mode and enabling all the optimizations, I'm compiling everything in debug instead. However, when compiling in debug mode, there's an assertion in wizer that gets tripped due to a bug in LLVM (see bytecodealliance/wizer#27). As a work-around, I'm using wasm-strip and wasm-opt directly on the engine during the build step.
We would have to figure out how we want to handle release builds, but I don't think they are necessary right now. This should make compile times much faster since we make better use of incremental builds.