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 unwrap error when given a no export functions wasm #2386

Merged
merged 2 commits into from
Jun 5, 2021

Conversation

chenyukang
Copy link
Contributor

Description

When the wasm file contains no export functions, an unwrap crash will trigger:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', lib/cli/src/commands/run.rs:344:52

This fix will give a more detailed output.

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Great catch! Merging!

@syrusakbary
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Jun 4, 2021
2386: fix unwrap error when given a no export functions wasm r=syrusakbary a=chenyukang


# Description
When the wasm file contains no export functions, an unwrap crash will trigger:

```
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', lib/cli/src/commands/run.rs:344:52
```

This fix will give a more detailed output.




Co-authored-by: chenyukang <[email protected]>
@chenyukang
Copy link
Contributor Author

chenyukang commented Jun 4, 2021

I found another issue when adding testcase, my code is not executed when I run:

 cargo test --package wasmer-integration-tests-cli --test run -- run_no_start_wasm_report_error --exact --nocapture

This is because WASMER_PATH only uses the release path, but I run it with debug version.

pub const WASMER_PATH: &str = concat!(
    env!("CARGO_MANIFEST_DIR"),
    "/../../../target/release/wasmer"
);

To keep it consistent, we should fix this:

#[cfg(feature = "debug")]
pub const WASMER_PATH: &str = concat!(
    env!("CARGO_MANIFEST_DIR"),
    "/../../../target/debug/wasmer"
);

#[cfg(not(feature = "debug"))]
pub const WASMER_PATH: &str = concat!(
    env!("CARGO_MANIFEST_DIR"),
    "/../../../target/release/wasmer"
);

@syrusakbary How do you think about it?

@bors
Copy link
Contributor

bors bot commented Jun 4, 2021

Canceled.

@syrusakbary
Copy link
Member

I think that’s a good change/fix! Happy to merge it on this PR as well

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkMcCaskey
Copy link
Contributor

The one thing I'd say, is that the debug feature is not set automatically when doing a debug build... So we may want to have a function which checks for both a debug or release build and uses the newest one? Or a more proper fix, maybe the make test-integration makefile command should depend on build-wasmer so it's always there.

I don't think the debug feature exists in the the integration tests crate, so I think the not(feature = "debug") will be triggered unconditionally

@syrusakbary
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 5, 2021

@bors bors bot merged commit cae21af into wasmerio:master Jun 5, 2021
@Hywan
Copy link
Contributor

Hywan commented Jun 7, 2021

👍 thanks for the PR!

@Hywan Hywan added bug Something isn't working 📦 lib-cli About wasmer-cli labels Jun 7, 2021
Hywan added a commit to Hywan/wasmer that referenced this pull request Jun 7, 2021
@Hywan Hywan mentioned this pull request Jun 7, 2021
1 task
syrusakbary added a commit that referenced this pull request Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-cli About wasmer-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants