build(wasmer/cache): opt 'sys' into default feature

[kwonoj]

Description

Context: this is originated from #2817 (comment) , where SWC (https://github.com/swc-project/swc) trying to setup its package dependencies orthogonally requires two distinct features of wasmer (js / sys), encounters feature selection getting merged by cargo. We have workaround it by manual opt in into each features, but the common dependency we have imports wasmer-cache, which ends up to enabling sys mandatory. It is also possible to do some workaround by having a feature with make dep as optional, but I feel it'd be better if cache allows to opt out from feature.

I am honestly not sure if this is acceptable or recommended approach to resolve issue. Please feel free to close if that's the case.

Review

• Add a short description of the change to the CHANGELOG.md file

[syrusakbary]

I thought about this further, and compiling wasmer-cache without the default features (that is without wasmer/sys) will make Rust compilation fail, as it depends on serialization/deserialization being present in the wasmer crate, however those methods are not present in wasmer/js.

To make this work when the default options are not present, the solution is simple: implement serialization/deserialization in wasmer/js, so wasmer-cache can be used also from JS. How?

• Each time a new module is being created with Module::new, we store the [u8] bytes inside the an attribute in the Module struct
• Module::deserialize (returning a wasmer::Module) takes as input an array of bytes and just calls Module::new on them.
• Module::serialize just returns the field on the struct with the module bytes contents
• Copy the SerializeError and DeserializeError to wasmer/js: https://github.com/wasmerio/wasmer/blob/master/lib/engine/src/error.rs#L10-L40

Also, we could make serialization a feature in wasmer/js as well, so we don't have an added overhead saving the bytes if the user doesn't need serialization present.
Thoughts?

We can do that as part of the PR and we should be good to merge! :)

[kwonoj]

Thanks for the guide, I'll peek if I can spin some time to shape PR to correctly include those.

[kwonoj]

I tried to add a few paths as much to follow suggestions, open to reflect fixes / suggestions.

[syrusakbary]

I don't think we should enforce sys or js by default. We can simplify this such as:

Suggested change

	default = ["sys"]
	sys = ["wasmer/sys-default"]
	js = ["wasmer/js-default", "wasmer/js-serializable-module"]
	default = ["wasmer/js-serializable-module"]

This should work without adding any feature dependency on wasmer (that is, that the wasmer crate will not automatically use js or sys, it will just use the wasmer/js-serializable-module feature when using js but do nothing on sys, which is good.

[kwonoj]

I was curious about this

wasmer crate will not automatically use js or sys

Wouldn't it make existing build as breaking, that upper deps having wasmer-cache or transitive deps need to manually opt in sys if they're using default? Or it'll be fine since any pkg having wasmer already may opt in js / sys?

[syrusakbary]

Oh, I think I was not super clear. I think the filesystem should be enabled by default as well (that way there are no breaking changes).
However, we should not enforce sys or js by default (wasmer-cache should remain agnostic).
In any case, I did applied a small suggestion and the PR looks good!

[syrusakbary]

Thanks @kwonoj, I added some feedback!

[kwonoj]

Thanks for the help, updated as suggested.

[syrusakbary]

bors r+

[bors]

Build failed:

• Code lint

[syrusakbary]

Linting is failing, but once is solved we should be good to merge.
You can lint locally with: cargo fmt --all @kwonoj

[kwonoj]

Thanks, applied fmt.

[syrusakbary]

bors r+

[bors]

Build succeeded:

• Audit
• Code lint
• Test cross-compile on linux
• Test cross-compile on macos
• Test on linux-x64
• Test on macos-x64
• Test on windows-x64