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.

Sign up for GitHub

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

Jump to bottom

fix: Upgrade Wasmer #2332

Closed
wants to merge 26 commits into from
Closed

fix: Upgrade Wasmer #2332

wants to merge 26 commits into from

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Mar 27, 2020 •
edited
Loading

Actually don't fixes traps. Wasmer still returns "unknown" errors on traps.

ref #2055

fixes #2316

@gitpod-io
Copy link

gitpod-io bot commented Mar 27, 2020 •
edited
Loading

@lexfrl lexfrl force-pushed the fckt/wasmer-16-update branch from 5b871c0 to ef572fa Compare March 30, 2020 15:12
@lexfrl lexfrl marked this pull request as ready for review March 30, 2020 15:58
@lexfrl lexfrl changed the title [WIP] fix: Upgrade Wasmer fix: Upgrade Wasmer Mar 30, 2020
@lexfrl lexfrl force-pushed the fckt/wasmer-16-update branch from af4e658 to 07f65c3 Compare April 1, 2020 00:14
mikhailOK
mikhailOK suggested changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@mikhailOK mikhailOK left a comment

Choose a reason for hiding this comment

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

near-vm-runner tests don't pass, looks like CI is broken right now

@bowenwang1996 bowenwang1996 removed their request for review April 3, 2020 22:31
@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 6, 2020 •
edited
Loading

Summary: this PR just updates Wasmer for now (let's wait for updates from Wasmer). Wasmer doesn't return ExceptionCode's, so this PR is not fixing #2055 .

please review @evgenykuzyakov @mikhailOK @nearmax

@lexfrl lexfrl requested a review from mikhailOK April 6, 2020 13:02
MaksymZavershynskyi
MaksymZavershynskyi approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

Thank you for looking into it! It is good enough for now as explained here: #2316 (comment)

@MaksymZavershynskyi
Copy link
Contributor

We prefer to not make significant alterations to the code after the reviews given

Absolutely. In other case, there is no point to even have reviews.

@nearmax @mikhailOK
If I understand correctly, the trouble here is that "msg" of the Trap is a part of protocol, right?

Correct. Until wasmerio/wasmer#1338 is fixed we should categorize all string-like errors as unknown

MaksymZavershynskyi
MaksymZavershynskyi reviewed Apr 13, 2020
View reviewed changes
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

See the comment

@lexfrl lexfrl assigned MaksymZavershynskyi and unassigned lexfrl Apr 14, 2020
@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 14, 2020

blocked by @evgenykuzyakov @mikhailOK

@lexfrl lexfrl requested a review from mikhailOK April 14, 2020 21:09
MaksymZavershynskyi
MaksymZavershynskyi suggested changes Apr 15, 2020
View reviewed changes
Copy link
Contributor

@MaksymZavershynskyi MaksymZavershynskyi left a comment

Choose a reason for hiding this comment

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

I would still suggest to try to cast to ExceptionCode and return unknown only when it is not possible to do so, e.g. in case of the string error, as mentioned in my previous comment. wasmerio/wasmer#1338

Also, you need only one approval either from me or from @evgenykuzyakov .

@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 15, 2020 •
edited
Loading

Also, you need only one approval either from me or from @evgenykuzyakov

@nearmax only if there are no change requests

@MaksymZavershynskyi
Copy link
Contributor

@fckt Did you have a chance to address my comment?

@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 17, 2020

@fckt Did you have a chance to address my comment?

I'd prefer to not use not working feature (as discussed)

@MaksymZavershynskyi
Copy link
Contributor

@fckt Did you have a chance to address my comment?

I'd prefer to not use not working feature (as discussed)

I don't remember agreeing on that. We are using not working feature anyway, where feature is the error handling in Wasmer. We need to use what we can process correctly (ExceptionCode) and not use unknown for everything else.

@lexfrl lexfrl mentioned this pull request Apr 20, 2020
Merged
@lexfrl
Copy link
Contributor Author

lexfrl commented Apr 21, 2020

#2505 supersedes this PR, so closing

@lexfrl lexfrl closed this Apr 21, 2020
@chefsale chefsale deleted the fckt/wasmer-16-update branch July 21, 2020 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaksymZavershynskyi MaksymZavershynskyi MaksymZavershynskyi requested changes

@mikhailOK mikhailOK mikhailOK approved these changes

@evgenykuzyakov evgenykuzyakov evgenykuzyakov approved these changes

@SkidanovAlex SkidanovAlex Awaiting requested review from SkidanovAlex

Assignees

@lexfrl lexfrl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade the downcasting of RuntimeError in Wasmer
5 participants
@lexfrl @MaksymZavershynskyi @evgenykuzyakov @mikhailOK @bowenwang1996