cfallin opened PR #3534 from isle-generated-code-manifest
to main
:
Currently, the
build.rs
script that generates Rust source from the
ISLE DSL will only do this generation if therebuild-isle
Cargo
feature is specified. By default, it is not. This is based on the
principle that we (the build script) do not modify the source tree as
managed by git; git-managed files are strictly a human-managed and
human-edited resource. By adding the opt-in Cargo feature, a developer
is requesting the build script to perform an explicit action. (In my
understanding at least, this principle comes from the general philosophy
of hermetic builds: the output should be a pure function of the input,
and part of this is that the input is read-only. If we modify the source
tree, then all bets are off.)Unfortunately, requiring the opt-in feature also creates a footgun that
is easy to hit: if a developer modifies the ISLE DSL source, but forgets
to specify the Cargo feature, then the compiler will silently be built
successfully with stale source, and will silently exclude any changes
that were made.The generated source is checked into git for a good reason: we want DSL
compiler to not affect build times for the overwhelmingly common case
that Cranelift is used as a dependency but the backends are not being
actively developed. (This overhead comes mainly from buildingislec
itself.)So, what to do? This PR implements a middle ground first described in
this conversation, in which we:
Generate a hash (SHA-512) of the ISLE DSL source and produce a
"manifest" of ISLE inputs alongside the generated source; andAlways read the ISLE DSL source, and see if the manifest is still
valid, on builds that do not have the opt-in "rebuild" feature.This allows us to know whether the ISLE compiler output would have been
the same (modulo changes to the DSL compiler itself, which are
out-of-scope here), without actually building the ISLE compiler and
running it.If the compiler-backend developer modifies an ISLE source file and then
tries to buildcranelift-codegen
without adding therebuild-isle
Cargo feature, they get the following output:Error: the ISLE source files that resulted in the generated Rust source * src/isa/x64/lower/isle/generated_code.rs have changed but the generated source was not rebuilt! These ISLE source files are: * src/clif.isle * src/prelude.isle * src/isa/x64/inst.isle * src/isa/x64/lower.isle Please add `--features rebuild-isle` to your `cargo build` command if you wish to rebuild the generated source, then include these changes in any git commits you make that include the changes to the ISLE. For example: $ cargo build -p cranelift-codegen --features rebuild-isle (This build script cannot do this for you by default because we cannot modify checked-into-git source without your explicit opt-in.)
which will tell them exactly what they need to do to fix the problem!
Note that I am leaving the "Rebuild ISLE" CI job alone for now, because
otherwise, we are trusting whomever submits a PR to generate the correct
generated source. In other words, the manifest is a communication from
the checked-in tree to the developer, but we still need to verify that
the checked-in generated Rust source and the manifest are correct with
respect to the checked-in ISLE source.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
- [ ] This has been discussed in issue #..., or if not, please tell us why
here.- [ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.- [ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin requested fitzgen for a review on PR #3534.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
md5 or sha1 should be enough for this purpose, right? That is probably faster.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
FWIW I see that this was done already but
std::process::abort
is a somewhat violent method of taking down the process (I think it may core dump?) andstd::process::exit
I think is a reasonable alternative.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Speed is not a concern here so few files are hashed.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Yep, I was following the existing
abort()
but I've made itexit(1)
now.
alexcrichton submitted PR review.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
alexcrichton submitted PR review.
cfallin updated PR #3534 from isle-generated-code-manifest
to main
.
cfallin merged PR #3534.
Last updated: Dec 23 2024 at 12:05 UTC