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

fix(scripts) Fix publish script with wasmer-c-api #2

Closed
wants to merge 10 commits into from

Conversation

Hywan
Copy link
Owner

@Hywan Hywan commented Jan 29, 2021

Description

Depends on wasmerio#2053.

This patch is just 2 commits:

  • wasmerio@52daf9b adding the WASMER_PUBLISH_SCRIPT_IS_RUNNING env var when running cargo publish,
  • wasmerio@4f8bc8a to not run build.rs when the scripts/publish.py script is running.

Review

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

Hywan and others added 10 commits January 29, 2021 11:39
…l range.

We use `VecDeque::drain` to read the captured stream, zipped with the
given buffer. We could expect that only the yielded items from the
`drain` will be removed, but actually no. Reading [the
documentation](https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.drain):

> Note 1: The element `range` is removed even if the iterator is not
> consumed until the end.

So by using a range like `..` will drain the entire captured stream,
whatever we read from it. Said differently, if the given buffer length
is smaller than the captured stream, the first read will drain the
entire captured stream.

This patch fixes the problem by specifying a better range:
`..min(inner_buffer.len(), oc.buffer.len())`.

With this new range, it's actually useless to increment
`num_bytes_written`, we already know ahead of time the amount of bytes
we are going to read. Consequently, the patch simplifies this code a
little bit more.
@Hywan Hywan self-assigned this Jan 29, 2021
@Hywan Hywan closed this Jan 29, 2021
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.

3 participants