Stream: git-wasmtime

Topic: wasmtime / PR #3616 Cranelift ISLE build script: make man...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 17 2021 at 19:09):

cfallin opened PR #3616 from check-isle-feature to main:

Some use-cases are very sensitive to the number of crates pulled in,
both for audit-related reasons and for build-time reasons.

Our manifest-based ISLE-up-to-date approach in #3534 was meant to help
with this while still avoiding the footgun of a completely opt-in source
rebuild: by including generated source in the checked-in tree, and just
checking that source is up to date, we allow for fast builds without the
whole ISLE compiler meta-step, but still catch attempted use of stale
generated source (and allow the developer to opt-in to regenerating the
in-tree source).

Unfortunately this still requires the hash algorithm itself (sha2)
which turns out to pull in a number of other small crates. In cases
where we know the source won't be stale -- for example, depending on the
main branch in git, or a published crate version of
cranelift-codegen -- the checks are actually not needed at all.

This PR thus introduces a feature check-isle that controls whether
build.rs does the checks. It is on by default, so developer safety
remains: if someone checks out the source, modifies some ISLE, and then
does a cargo build, they will get an error that helps them with the
proper steps to regenerate the source. But, dependencies that know what
they are doing can turn it off with default-features = false.

I've verified that we have the expected small dependency tree now:

$ cargo tree --depth 1 -p cranelift-codegen --no-default-features --features "std unwind all-arch"
cranelift-codegen v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen)
├── cranelift-bforest v0.79.0 (/home/cfallin/work/wasmtime/cranelift/bforest)
├── cranelift-codegen-shared v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen/shared)
├── cranelift-entity v0.79.0 (/home/cfallin/work/wasmtime/cranelift/entity)
├── gimli v0.26.1
├── log v0.4.14
├── regalloc v0.0.33
├── smallvec v1.6.1
└── target-lexicon v0.12.0
[build-dependencies]
├── cranelift-codegen-meta v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen/meta)
└── sha2 v0.9.8
[dev-dependencies]
└── criterion v0.3.5

Fixes #3609.

The diff looks wonky here but all I did was put the ISLE-related
logic inside a mod isle { ... } with a conditional-on-feature directive
and then alter the toplevel a bit.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 17 2021 at 19:09):

cfallin requested alexcrichton for a review on PR #3616.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 17 2021 at 19:16):

cfallin updated PR #3616 from check-isle-feature to main.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 17 2021 at 19:17):

cfallin edited PR #3616 from check-isle-feature to main:

Some use-cases are very sensitive to the number of crates pulled in,
both for audit-related reasons and for build-time reasons.

Our manifest-based ISLE-up-to-date approach in #3534 was meant to help
with this while still avoiding the footgun of a completely opt-in source
rebuild: by including generated source in the checked-in tree, and just
checking that source is up to date, we allow for fast builds without the
whole ISLE compiler meta-step, but still catch attempted use of stale
generated source (and allow the developer to opt-in to regenerating the
in-tree source).

Unfortunately this still requires the hash algorithm itself (sha2)
which turns out to pull in a number of other small crates. In cases
where we know the source won't be stale -- for example, depending on the
main branch in git, or a published crate version of
cranelift-codegen -- the checks are actually not needed at all.

This PR thus introduces a feature check-isle that controls whether
build.rs does the checks. It is on by default, so developer safety
remains: if someone checks out the source, modifies some ISLE, and then
does a cargo build, they will get an error that helps them with the
proper steps to regenerate the source. But, dependencies that know what
they are doing can turn it off with default-features = false.

I've verified that we have the expected small dependency tree now:

$ cargo tree --depth 1 -p cranelift-codegen --no-default-features --features "std unwind all-arch"
cranelift-codegen v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen)
├── cranelift-bforest v0.79.0 (/home/cfallin/work/wasmtime/cranelift/bforest)
├── cranelift-codegen-shared v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen/shared)
├── cranelift-entity v0.79.0 (/home/cfallin/work/wasmtime/cranelift/entity)
├── gimli v0.26.1
├── log v0.4.14
├── regalloc v0.0.33
├── smallvec v1.6.1
└── target-lexicon v0.12.0
[build-dependencies]
└── cranelift-codegen-meta v0.79.0 (/home/cfallin/work/wasmtime/cranelift/codegen/meta)
[dev-dependencies]
└── criterion v0.3.5

Fixes #3609.

The diff looks wonky here but all I did was put the ISLE-related
logic inside a mod isle { ... } with a conditional-on-feature directive
and then alter the toplevel a bit.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Dec 17 2021 at 20:02):

cfallin closed without merge PR #3616.

Last updated: Dec 23 2024 at 12:05 UTC