Stream: git-wasmtime

Topic: wasmtime / PR #5318 cranelift-isle: Rewrite error reporting


view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:30):

jameysharp opened PR #5318 from isle-errors to main:

There were several issues with ISLE's existing error reporting implementation.

This commit:

I tried to make this a series of smaller commits but I couldn't find any good split points; everything was too entangled with everything else.

I still need to test this with input that actually has errors in it, but I wanted to get early review at this point since it at least builds cleanly both with and without the fancy-errors feature.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:30):

jameysharp requested fitzgen for a review on PR #5318.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:45):

jameysharp updated PR #5318 from isle-errors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:46):

jameysharp updated PR #5318 from isle-errors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:49):

jameysharp updated PR #5318 from isle-errors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 00:49):

jameysharp has marked PR #5318 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin created PR review comment:

Likewise here, for deploy -> run.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin created PR review comment:

Can you say more about this exemption update? As far as I understood it, we decided we wouldn't add any more exemptions past the initial set that we had when we adopted cargo-vet, so this might need a true audit rather than an expanded exemption. (But I don't claim to fully understand our policies and cargo-vet subtleties here, so others please feel free to chime in too...)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:12):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:12):

cfallin created PR review comment:

Ah, actually, reading more, it seems that safe-to-run is a subset of safe-to-deploy (a weaker claim), so this is a downgrade and should be automatically fine; does that match your understanding? Was this change due to some indication that we didn't need the wider claim anymore?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:13):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:13):

jameysharp created PR review comment:

I didn't think about it too hard because this is something cargo vet did automatically, and I figured it knew what it was doing. But I believe this is weakening the exemption. If I understand correctly, safe-to-deploy implies safe-to-run, but not the other way around, so with this change we're no longer saying we trust these exempted crates as much as we did before.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:19):

jameysharp created PR review comment:

Jinx :laughing:

All the changes to config.toml are because Miette had a bunch of dependencies while codespan-reporting only depends on a couple of crates, which I guess we were already using elsewhere. termcolor is used by clap, cranelift-tools, and others, while unicode-width is used by clap, wast, and others.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:19):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 23 2022 at 22:20):

jameysharp merged PR #5318.


Last updated: Dec 23 2024 at 12:05 UTC