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

Convert from docopt to clap #55

Closed
sunfishcode opened this issue Feb 26, 2019 · 13 comments
Closed

Convert from docopt to clap #55

sunfishcode opened this issue Feb 26, 2019 · 13 comments

Comments

@sunfishcode
Copy link
Member

The wasmtime driver program (src/wasmtime.rs) currently uses docopt to parse command-line arguments. clif-util in Cranelift switched from docopt to clap because it's simpler and has better support for subcommands and seems to be more popular in the Rust ecosystem, so it'd make sense to convert wasmtime to use clap too.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 4500.0 DAI (4500.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Mar 11, 2019

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 2 months from now.
Please review their action plans below:

1) samparsky has been approved to start work.

I am quite familiar with Rust and have experience with Clap library. This should take between 3 - 5 days

Learn more on the Gitcoin Issue Details page.

2) maxmcd has applied to start work (Funders only: approve worker | reject worker).

I work with wasmtime regulary (i’ve contributed to it before) and I am familiar with the command line setup. Would take a couple days to swap out the necessary components.

Learn more on the Gitcoin Issue Details page.

3) davidbanu has applied to start work (Funders only: approve worker | reject worker).

Would love to convert this project from Docopt To Clap...

Learn more on the Gitcoin Issue Details page.

4) svartalf has applied to start work (Funders only: approve worker | reject worker).

I'm familiar with clap and had used it in my projects, so I would love to help you with that task

Learn more on the Gitcoin Issue Details page.

@iamonuwa
Copy link

Hi @sunfishcode, Gitcoin ambassador here 🖐️ 😄 . I think you need to jump in on this one. Please reach out to @ceresstation to select appropriate hunter for this task.

@spm32
Copy link

spm32 commented Mar 15, 2019

Hi @samparsky I just approved you since you were first, please make sure to submit WIP PRs :)

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 4500.0 DAI (4500.0 USD @ $1.0/DAI) has been submitted by:

  1. @samparsky

@ceresstation please take a look at the submitted work:


@svartalf
Copy link

@ceresstation, to be honest, this looks a bit weird, because Gitcoin issue states that approve is required before work can be started.

While nobody can force "workers" not to start earlier, this is unfair behavior to other applicants - you can do the job without approval awaiting and receive the expected bounty.
This is why I did this task too -- because otherwise this might be considered as cheating from the first applicant side: breaking contest rules and claiming bounty just because they was first to do the unasked task.

I understand that this issue is a wrong place to discuss this thing, but I feel like I need to accentuate the problem.

@samparsky
Copy link

samparsky commented Mar 15, 2019

@svartalf Yeah I understand your point and I have already been cautioned as regards this.
Also I didn't get approved cause I had already started the work its cos I was the first applicant and the timeline I suggested was approved.

@spm32
Copy link

spm32 commented Mar 15, 2019

Hey @svartalf I agree, we need a better system in place to ensure people only start once they've been accepted, but yes in this case @samparsky was just accepted because they were the first applicant on the bounty (not the first to start working on a solution).

That said if you also have a solution I'd be happy to give you a tip for it, and I think there may be some other bounties on these repos coming up which I'd love to have you working on if you're up for it :)

@svartalf
Copy link

@ceresstation this might lead to a case when automated or semi-automated applications to issues will be chosen more often, leading to selection based not on a merit or possible positive outcome (as in my #62 solution), but based only on the time of applying, which leads us to the reasons I described before.
Unfortunately, I can't suggest any properly functional solution to this problem right now, because it is hard to solve, I agree on that, and I wish you luck in resolving it.

@samparsky @sunfishcode I'd like you to consider re-using my implementation for reasons declared in #62; I especially want to emphasize that this suggestion is NOT based on possible expectation of payment for it, but just because I do not want to think that I've spent my time and skills for nothing :)

@spm32
Copy link

spm32 commented Mar 16, 2019

@svartalf I agree with you here, right now the only safeguard against these sorts of issues is to only start work once approved but hopefully we'll have better ones down the road. In this case I'll leave it to @sunfishcode to decide which implementation they prefer. :)

@lazaridiscom this was just to ensure that the task would be taken as seriously as possible since this is the first bounty with @sunfishcode

@gitcoinbot
Copy link

⚡️ A tip worth 1800.00000 DAI (1800.0 USD @ $1.0/DAI) has been granted to @samparsky for this issue from @ceresstation. ⚡️

Nice work @samparsky! Your tip has automatically been deposited in the ETH address we have on file.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This Bounty has been completed.

Additional Tips for this Bounty:

  • ceresstation tipped 1800.0000 DAI worth 1800.0 USD to samparsky.

@sunfishcode
Copy link
Member Author

As an update here: Using Gitcoin here was something of a trial run, and with what we know now, we won't be continuing it at this time. Thanks to the folks who participated here, and my apologies for things here being confusing.

alexcrichton added a commit that referenced this issue Nov 2, 2022
…n `Trap` (#5149)

* Return `anyhow::Error` from host functions instead of `Trap`

This commit refactors how errors are modeled when returned from host
functions and additionally refactors how custom errors work with `Trap`.
At a high level functions in Wasmtime that previously worked with
`Result<T, Trap>` now work with `Result<T>` instead where the error is
`anyhow::Error`. This includes functions such as:

* Host-defined functions in a `Linker<T>`
* `TypedFunc::call`
* Host-related callbacks like call hooks

Errors are now modeled primarily as `anyhow::Error` throughout Wasmtime.
This subsequently removes the need for `Trap` to have the ability to
represent all host-defined errors as it previously did. Consequently the
`From` implementations for any error into a `Trap` have been removed
here and the only embedder-defined way to create a `Trap` is to use
`Trap::new` with a custom string.

After this commit the distinction between a `Trap` and a host error is
the wasm backtrace that it contains. Previously all errors in host
functions would flow through a `Trap` and get a wasm backtrace attached
to them, but now this only happens if a `Trap` itself is created meaning
that arbitrary host-defined errors flowing from a host import to the
other side won't get backtraces attached. Some internals of Wasmtime
itself were updated or preserved to use `Trap::new` to capture a
backtrace where it seemed useful, such as when fuel runs out.

The main motivation for this commit is that it now enables hosts to
thread a concrete error type from a host function all the way through to
where a wasm function was invoked. Previously this could not be done
since the host error was wrapped in a `Trap` that didn't provide the
ability to get at the internals.

A consequence of this commit is that when a host error is returned that
isn't a `Trap` we'll capture a backtrace and then won't have a `Trap` to
attach it to. To avoid losing the contextual information this commit
uses the `Error::context` method to attach the backtrace as contextual
information to ensure that the backtrace is itself not lost.

This is a breaking change for likely all users of Wasmtime, but it's
hoped to be a relatively minor change to workaround. Most use cases can
likely change `-> Result<T, Trap>` to `-> Result<T>` and otherwise
explicit creation of a `Trap` is largely no longer necessary.

* Fix some doc links

* add some tests and make a backtrace type public (#55)

* Trap: avoid a trailing newline in the Display impl

which in turn ends up with three newlines between the end of the
backtrace and the `Caused by` in the anyhow Debug impl

* make BacktraceContext pub, and add tests showing downcasting behavior of anyhow::Error to traps or backtraces

* Remove now-unnecesary `Trap` downcasts in `Linker::module`

* Fix test output expectations

* Remove `Trap::i32_exit`

This commit removes special-handling in the `wasmtime::Trap` type for
the i32 exit code required by WASI. This is now instead modeled as a
specific `I32Exit` error type in the `wasmtime-wasi` crate which is
returned by the `proc_exit` hostcall. Embedders which previously tested
for i32 exits now downcast to the `I32Exit` value.

* Remove the `Trap::new` constructor

This commit removes the ability to create a trap with an arbitrary error
message. The purpose of this commit is to continue the prior trend of
leaning into the `anyhow::Error` type instead of trying to recreate it
with `Trap`. A subsequent simplification to `Trap` after this commit is
that `Trap` will simply be an `enum` of trap codes with no extra
information. This commit is doubly-motivated by the desire to always use
the new `BacktraceContext` type instead of sometimes using that and
sometimes using `Trap`.

Most of the changes here were around updating `Trap::new` calls to
`bail!` calls instead. Tests which assert particular error messages
additionally often needed to use the `:?` formatter instead of the `{}`
formatter because the prior formats the whole `anyhow::Error` and the
latter only formats the top-most error, which now contains the
backtrace.

* Merge `Trap` and `TrapCode`

With prior refactorings there's no more need for `Trap` to be opaque or
otherwise contain a backtrace. This commit parse down `Trap` to simply
an `enum` which was the old `TrapCode`. All various tests and such were
updated to handle this.

The main consequence of this commit is that all errors have a
`BacktraceContext` context attached to them. This unfortunately means
that the backtrace is printed first before the error message or trap
code, but given all the prior simplifications that seems worth it at
this time.

* Rename `BacktraceContext` to `WasmBacktrace`

This feels like a better name given how this has turned out, and
additionally this commit removes having both `WasmBacktrace` and
`BacktraceContext`.

* Soup up documentation for errors and traps

* Fix build of the C API

Co-authored-by: Pat Hickey <pat@moreproductive.org>
pchickey added a commit that referenced this issue Nov 3, 2022
* Trap: avoid a trailing newline in the Display impl

which in turn ends up with three newlines between the end of the
backtrace and the `Caused by` in the anyhow Debug impl

* make BacktraceContext pub, and add tests showing downcasting behavior of anyhow::Error to traps or backtraces
frank-emrich pushed a commit to frank-emrich/wasmtime that referenced this issue Aug 18, 2023
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 a pull request may close this issue.

6 participants