@aduros

I've played a bit but I've not tried the download feature because I've got alreadywasm-opt onPATH.
Overall it works and is pretty nifty but I've got few observations.

unimplemented! if Os is not supported

Can we usebinaryen.js if a binary is not available for the user machine instead of panicking?
Note that there are currently issues with M1 macs:WebAssembly/binaryen#4334

Too much code inxtask-build directory

There's probably too much logic insidextask/xtask-build.
Is it possible to move most of the logic in an external lib a leave there the bare minimum?

Readme

This branch is a bit behindmain:
I'd like to see how your changes interact with theReadme feature before merging this PR.

I think that is important that we get more feedback on this overhaul.
Maybe@claudiomattera could chime in on this one?

unimplemented! if Os is not supported

Can we usebinaryen.js if a binary is not available for the user machine instead of panicking? Note that there are currently issues with M1 macs:WebAssembly/binaryen#4334

It seems to me. There is. Another reason for. WASM-4 SDK.

Too much code inxtask-build directory

There's probably too much logic insidextask/xtask-build. Is it possible to move most of the logic in an external lib a leave there the bare minimum?

I had a thought about implementing cargo-wasm4 for my wasm4-rs repo. I could implement this thing inside of the cargo-wasm4 and expose it as a library. Optimization like--zero-filled-memory is runtime implementation dependent, so i don't think i could expand it to more general case. However i am worried@aduros won't approve usage of 3rd party crate there anyway.

Maybe@claudiomattera could chime in on this one?

First of all, let me remind everyone that I have little experience with WASM, so do not take my opinions too seriously :D

Anyway, it is nice to see someone working on streamlining WASM-4 games development with Rust.
I was quite confused when I started playing with this console and could not understand the reason for cartridges being so oversized.
We need a way to includewasm-opt in the build process transparently to the user, so that the final cartridge does not contain debug information.

I also have a few observations:

1. I do not like too much that it overhauls the build process by wrapping everything in a new command.
   Rust developers might expect a familiar workflow involvingcargo build --release and build upon that.

2. I also do not like the workspace structure, where crates contain other crates.
   I would probably make the top-levelCargo.toml only a workspace, and create three subdirectoriescartridge,xtask andxtask-build with the actual crates.

3. I am conflicted whether downloading Binaryen if it is missing is a good idea or not.
   First of all, depending onbinary-install brings quite a lot of build-time dependencies.
   I really liked that the default build of a Rust template has zero dependencies (one, if you enablebuddy-alloc).

   binary-install also depends on cURL and OpenSSL.
   I wonder how that would turn out on systems where those tools do not exist or are difficult to build, such as MUSL-based Linux systems or architectures different than x86[_64] and ARM.
   But realistically this might not be an actual issue for anyone.

   On top of that, cratebinary-install seems neither mature nor actively developed.
   Latest stable version 0.0.2 is from three years ago, latest unstable version 0.0.3.alpha1 is from over one year ago, and last activity on the repo is also quite old.
   Aside from the version number, its readme oncrates.io is basically empty.
   It makes it a bit difficult to figure out what it does and how it works.

   So I would at least make the download optional, perhaps hiding it behind a Cargo feature.
   That would removebinary-install and thin out the dependency tree.

   I would also verify that no unused features of other dependencies (cargo_metadata andlog) are enabled.
   That might remove a couple more dependencies.

4. Finally, I am not a big fan of strippingall debug information from the release cartridge.
   I usetwiggy quite a lot to investigate which functions take space in the cartridge, but it needs debug information, otherwise it will just tell me something like

    Shallow Bytes │ Shallow % │ Item───────────────┼───────────┼────────────────          7831 ┊    36.13% ┊ code[0]          4168 ┊    19.23% ┊ data[0]          2344 ┊    10.82% ┊ data[1]          1166 ┊     5.38% ┊ code[1]           782 ┊     3.61% ┊ code[2]

   which is quite useless.
   I would prefer to leave the unstripped cartridge around.

   But now that I read it again, I realize I can just callcargo build --release without using the wrapper, so it is not an actual issue.

I hope I did not sound too negative, though.
Extracting the path of build artefacts from Cargo's output was quite clever (I normally just assume they are intarget/wasm32-unknown-unknown/release, but this seems more robust).
Ultimately it does generate a properly stripped and optimized cartridge, perhaps it can be a good starting point for the Rust template.

In comparison, I took a much less ambitious approach in my projects.
I simply created aMakefile.toml (usingcargo-make), and defined few tasks to automate the build process.

cargo make build-release builds the cartridge leaving all debug information, andcargo make build-optimized also runswasm-opt on it.
I also have other useful tasks such ascargo make run-optimized-native, orcargo make bundle-into-cartridge, or evencargo make verify-cartridge-size (useful in pipelines).

I do not like too much that it overhauls the build process by wrapping everything in a new command.
Rust developers might expect a familiar workflow involving cargo build --release and build upon that.

There'srust-lang/cargo#545, but it is not resolved.

I also do not like the workspace structure, where crates contain other crates.
I would probably make the top-level Cargo.toml only a workspace, and create three subdirectories cartridge, xtask and xtask-build with the actual crates.

There's nothing wrong with it. This PR is modelled afterhttps://github.com/matklad/cargo-xtask/ repo.

3. binary-install iswasm-pack's dependency and is underrustwasm org. So it's not a random crate and it has some support. I should investigate openssl and curl support tho.

I would also verify that no unused features of other dependencies (cargo_metadata and log) are enabled.
That might remove a couple more dependencies.

I remember i have done that already. There's alreadydefault-features = false onenv_logger for example.

4. We could add a custom flag.

(5.) I have consideredcargo-make, but there wascargo-xtask which works out of the box. Also there's nocargo_metadata forcargo-make.

EDIT: i've eased my language here a bit. Sorry for being rude to you@claudiomattera and thank you for your review. I have spent a lot of time on user friendliness, so i was stressed there a bit.

I am no longer wanna do anything before there's any consensus on a solution. I had idea forcargo-wasm4 described above, but i am not gonna do it, for it to be disapproved right after.

I invite you@claudiomattera to go ahead and solverust-lang/cargo#545

That feature looks nice, it would open up a few possibilities of automation without relying on external tools.
Hopefully its design will be completed sometime, and we will get the dual ofbuild.rs scripts.

There's nothing wrong with it outside of your preference.

Correct.

binary-install iswasm-pack's dependency and is underrustwasm org. So it's not some random crate.

It does not really rebut any of my points, though.

It carries lots of dependencies, including system ones (cURL and OpenSSL).
It has not seen any stable release in 3 years.
It has not even hit 0.1.0 "milestone", sign that the authors might consider significantly changing its interface.
It is poorly documented: the readme is almost empty, the rustdoc is only slightly better (e.g. it does not state where files are downloaded).

I would personally try to avoid that kind of direct dependency.

Why are you assuming i haven't done that already? There's alreadydefault-features = false onenv_logger for example.

That was more of a reminder than anything else.

I have been auditing some of my own projects lately, and I always found some unused features in my dependencies.
Good to know that you already took care of that.

We could add a flag so whatever.

Yes, that would a good idea.
My main objection to this approach is depending onbinary-install, which is only used for downloading wasm-opt if missing.
If that was tied to a Cargo feature, users could easily disable it (I would personally keep it opt-in, though, and not opt-out).

(5.) I have consideredcargo-make, but there wascargo-xtask which works out of the box. Also there's nocargo_metadata forcargo-make.

Sure, I just mentioned it as a simpler, less powerful alternative.
Mycargo-make tasks are all manually written, while your approach seems more automated.

By the way, I was wondering if this step is something that could be embedded into the linker and Cargo's--strip option.
As I said, I am not too experienced with WASM, and I am not sure how the linker works for that target, but after all we are stripping the final object code.
Is there a way to make Cargo call wasm-opt in that way?

By the way, I was wondering if this step is something that could be embedded into the linker and Cargo's --strip option.
As I said, I am not too experienced with WASM, and I am not sure how the linker works for that target, but after all we are stripping the final object code.
Is there a way to make Cargo call wasm-opt in that way?

Well there'sstrip = true cargo option which is coming to 1.59.0. But that wouldn't solve everything tho. Allocator uses statically allocated heap, which results in a buch of zeroes in the final binary (~13KiB was for me i think).wasm-opt -Oz --zero-filled-memory solves this.

With regards to thebinary-install. I think it's not an ideal dependency, but i am afraid we cannot surpass it's quality by our own. I only guessWASM4_SDK would help with that, by shipping wasm-opt in it.

Again i would like to apologise for my rude behaviour@claudiomattera. Your thoughts on the subject are valuable here, do not doubt that.

Well there'sstrip = true cargo option which is coming to 1.59.0. But that wouldn't solve everything tho. Allocator uses statically allocated heap, which results in a buch of zeroes in the final binary (~13KiB was for me i think).wasm-opt -Oz --zero-filled-memory solves this.

Yeah.
In my previous pull request#138 I replaced thosestatic muts with raw pointes, but@aduros preferred to keep them that way and strip the executable withwasm-opt instead (it would have only solved the problem for the allocator, anyway, and not for other uses of statically-allocated heap, so it definitely made sense).

With regards to thebinary-install. I think it's not an ideal dependency, but i am afraid we cannot surpass it's quality by our own. I only guessWASM4_SDK would help with that, by shipping wasm-opt in it.

Fair point.
That crate might be young and unstable, but reimplementing it from scratch would not solve anything.

Again i would like to apologise for my rude behaviour@claudiomattera. Your thoughts on the subject are valuable here, do not doubt that.

No problem, and of course your work on this topic is also appreciated :)