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

[bazel] Improve build determinism #25842

Merged
merged 3 commits into from
Jan 10, 2025
Merged

Conversation

AlexJones0
Copy link
Contributor

This PR includes three commits, which, in addition to the crate updates from e7868c7 in #25795, should help improve the determinism of our builds and as such should improve the cache hit rate for Bazel builds, reducing build times. See more details in the commit messages.

@AlexJones0 AlexJones0 requested a review from cfrantz as a code owner January 10, 2025 11:41
@AlexJones0 AlexJones0 requested review from jwnrt and pamaury January 10, 2025 11:44
Make OTP image stamping (including a comment "Generated on [DATE] by
gen_otp_img.py ...") conditional on the presence of Bazel's standard
stamp argument, such that by default generated files are not stamped,
but the original behaviour can be retained when running Bazel with the
`--stamp` flag.

This will make OTP image building deterministic, and as such should
help improve cache hit rates and thus reduce build times (if Bazel
thinks it needs to re-build the OTP but nothing has changed, making the
OTP image deterministic will allow it to re-use results that use the
identical OTP images).

Signed-off-by: Alex Jones <[email protected]>
Sorts the set of word annotations for a MEM line in the OTP image, such
that the MEM line being read by Vivado is the same, but the word
annotations always appear in the same order, improving the determinism
of the builds.

Previously, usage of Python's sets meant that for MEM lines with multiple
word attributes, they could appear in different orders between builds,
causing slight differences in the output images (and thus potentially
requiring later actions to be re-run instead of cache hits due to
differences in the generated OTP image).

Signed-off-by: Alex Jones <[email protected]>
Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me, nice work.

Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find @AlexJones0

Move rules_foreign_cc from the root MODULE.bazel to its own
`third_party/foreign_cc` directory so that we can patch it to fix an
issue that has not yet been supported upstream yet, in the same way as
we do for e.g. `rules_rust`.

When `rules_foreign_cc` builds using tools such as e.g. Configure+Make,
output files Configure.log and BootstrapGNUMake.log are generated and
emitted which contain references to the absolute path. When building
inside Bazel's sandbox, this produces a non-determinstic output which
changes between each run, as a result, rebuilding OpenOCD will then
change the inputs of any Bazel actions that use OpenOCD, resulting in
a reduced number of cache hits and increasing build times. By making
this determinstic, we improve reproducibility and potentially reduce
build times.

To fix this, this PR introduces a simple patch to completely remove
all the content of this logging output, as it is not needed by any later
steps as part of the build.

Signed-off-by: Alex Jones <[email protected]>
@jwnrt jwnrt merged commit 0e617f4 into lowRISC:master Jan 10, 2025
38 checks passed
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 this pull request may close these issues.

4 participants