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

Support gzipped WASM inputs in utils::parse_wasm and utils::parse_wasm_file #55

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

aterga
Copy link
Contributor

@aterga aterga commented Mar 20, 2024

The goal of this PR is to support gzipped WASMs as inputs in the ic-wasm crate. This is aligned with the IC interface spec that says:

WebAssembly modules that are (slightly) larger than 2MB can still be installed on ICP by using gzip file compression before uploading; ICP will then decompress the file and install the contained WebAssembly module.

https://internetcomputer.org/docs/current/developer-docs/smart-contracts/deploy/larger-wasm/

@aterga aterga requested a review from a team as a code owner March 20, 2024 10:00
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Another related question, if the input is gzipped, do we expect the output to be gzipped as well?

src/utils.rs Outdated
} else if bytes.starts_with(GZIPPED_WASM_MAGIC_BYTES) {
Err(io::Error::new(
io::ErrorKind::InvalidInput,
"Please use parse_wasm_robust with gzipped inputs.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be a breaking change, why not just decompress here, instead of a new function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original public function parse_wasm's signature has bytes: &[u8], changing it to bytes: Vec<u8> would be a breaking change, right?

If we keep it &[u8], then the decompress function's result type (Result<Vec<u8>) is incompatible with it in one of the branches. So either we would need to inline decompress or have another function that takes Vec<u8> to begin with.

What's your preference? Am I missing a better possible implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep it &[u8], then the decompress function's result type (Result<Vec) is incompatible with it in one of the branches.

We can make both branches to return Cow::Borrowed and Cow::Owned, so that they end up with the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give it a try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to have worked right away. Thanks for the tip!

@aterga
Copy link
Contributor Author

aterga commented Mar 20, 2024

Another related question, if the input is gzipped, do we expect the output to be gzipped as well?

Good question. In my view, this should be controlled separately. I.e., we could add a feature to enable emitting gzipped WASMs, but it doesn't have to be part of this feature.

@aterga aterga changed the title Add parse_wasm_robust Support gzipped WASM inputs in utils::parse_wasm and utils::parse_wasm_file Mar 20, 2024
Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

LGTM

@aterga aterga merged commit b1c63d5 into dfinity:main Mar 20, 2024
4 checks passed
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.

None yet

2 participants