Stream: git-wasmtime

Topic: wasmtime / PR #6773 Remove cranelift-codegen-shared

Wasmtime GitHub notifications bot (Jul 25 2023 at 19:55):

bjorn3 opened PR #6773 from bjorn3:cleanup3 to bytecodealliance:main:

Based on https://github.com/bytecodealliance/wasmtime/pull/6772

Wasmtime GitHub notifications bot (Jul 25 2023 at 19:55):

bjorn3 requested wasmtime-compiler-reviewers for a review on PR #6773.

Wasmtime GitHub notifications bot (Jul 25 2023 at 19:55):

bjorn3 requested abrown for a review on PR #6773.

Wasmtime GitHub notifications bot (Jul 25 2023 at 19:55):

bjorn3 requested wasmtime-default-reviewers for a review on PR #6773.

Wasmtime GitHub notifications bot (Jul 25 2023 at 21:24):

elliottt requested jameysharp for a review on PR #6773.

Wasmtime GitHub notifications bot (Jul 26 2023 at 12:59):

bjorn3 updated PR #6773.

Wasmtime GitHub notifications bot (Jul 31 2023 at 19:10):

jameysharp submitted PR review:

I discussed this today with @cfallin and @fitzgen and we're in favor of these changes. Reducing the time it takes to build Cranelift is super helpful. I was a little uncomfortable with adding more strings we have to emit from codegen-meta but the change is well-motivated and you've done a great job making the result as easy to maintain as possible. Thanks!

I think one further change is necessary before we can merge this: don't we need to remove cranelift-codegen-shared from both lists in scripts/publish.rs? I think the verify-publish CI job would have caught this but it was skipped.

Thanks also to @elliottt for reminding me that I hadn't responded to this PR yet.

Wasmtime GitHub notifications bot (Jul 31 2023 at 19:10):

jameysharp submitted PR review:

I discussed this today with @cfallin and @fitzgen and we're in favor of these changes. Reducing the time it takes to build Cranelift is super helpful. I was a little uncomfortable with adding more strings we have to emit from codegen-meta but the change is well-motivated and you've done a great job making the result as easy to maintain as possible. Thanks!

I think one further change is necessary before we can merge this: don't we need to remove cranelift-codegen-shared from both lists in scripts/publish.rs? I think the verify-publish CI job would have caught this but it was skipped.

Thanks also to @elliottt for reminding me that I hadn't responded to this PR yet.

Wasmtime GitHub notifications bot (Jul 31 2023 at 19:10):

jameysharp created PR review comment:

I don't feel strongly about this, but I'd be tempted to use a macro here to ensure that the name of the constant is always the same in the generated source as it is in the meta crate. Something like constants!(fmt, LANE_BASE, REFERENCE_BASE, ...) I guess.

I'm happy to merge this as-is, but I thought I'd mention it since I think publish.rs needs to be fixed anyway. Let me know whether you want to change this too.

Last updated: Oct 23 2024 at 20:03 UTC