-
Notifications
You must be signed in to change notification settings - Fork 687
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
[Tracking] Reproducible builds #1666
Comments
Wow. Looks like diff --git a/boards/Makefile.common b/boards/Makefile.common
index 0d4eae0f2..3625f467d 100644
--- a/boards/Makefile.common
+++ b/boards/Makefile.common
@@ -16,7 +16,8 @@ RUSTUP ?= rustup
# lld the actual page size so it doesn't have to be conservative.
RUSTFLAGS_FOR_CARGO_LINKING ?= -C link-arg=-Tlayout.ld -C linker=rust-lld \
-C linker-flavor=ld.lld -C relocation-model=dynamic-no-pic \
--C link-arg=-zmax-page-size=512
+-C link-arg=-zmax-page-size=512 \
+--remap-path-prefix=/Users/bradjc/git=
# Disallow warnings for continuous integration builds. Disallowing them here
# ensures that warnings during testing won't prevent compilation from succeeding. |
Nice find @bradjc! We still need to extract the main Makefile's directory instead of hard-coding a given user's build directory. |
Updated the list to refer to #1657, as the Makefile doesn't seem idempotent. |
#1657 was merged, has this been addressed at this point? |
I can check the box, although the reason is still unclear, so it might come back when we update to another nightly. |
I did a quick test, building the imix board in two build folders ( Using # in /path/to/tock
rustc --crate-name tock_cells ... \
-C metadata=a72da9d1edb1895f \
-C extra-filename=-a72da9d1edb1895f \
...
# in /path/to/tock2
rustc --crate-name tock_cells ... \
-C metadata=e0d01c51ad799de2 \
-C extra-filename=-e0d01c51ad799de2 \
... I think that this metadata parameter is used as a seed for non-deterministic elements in the compiler (e.g. ordering of elements inside HashMaps, maybe also allocation of registers, or choice of equivalent instructions), so ultimately the code is different inside the binary. Relatedly, rust-lang/cargo#6966 removed |
Following rust-secure-code/wg#28 (comment), I also tried to use I also tried to remap the sysroot (rust-lang/rust#63505), but again didn't observe any effect. I also found reproducibility issues related to the linker: as well as debug symbols (we use
Tweaking these bits of the configuration may make Tock builds more reproducible. |
I did a few more tests.
The invocation for crates internal to Tock looks like the following.
On the other hand, the board invocation looks like the following.
What strikes me is the absolute path for local dependencies ( This is consistent with the results I observed in my tests. Update After a few more tests, it seems that defining a Cargo workspace encapsulating all the Tock crates would make the build reproducible (at least on a given machine), because then Cargo identifies all the crates by a relative path to the workspace root. |
With #1714 (currently in review), we're getting close to having reproducible builds of Tock! One remaining discrepancy is due to the |
1714: Use a Cargo workspace to make builds reproducible and speed up CI. r=gendx a=gendx ### Pull Request Overview This pull request adds a [Cargo workspace](https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html) at the top-level directory to make build reproducible (see the analysis in #1666 (comment)). With a workspace, Cargo now only writes to a single `target/` directory in the root folder, instead of having a separate `target/` in each board. This means that some changes have to be made in the Makefiles and tools. Another benefit of a single workspace is that shared dependencies can be built once (per CPU architecture) and cached across boards. For example, the kernel or the capsules only need to be compiled once per CPU architecture, instead of once per board, which can significantly speed up commands like `make allboards`. ### Testing Strategy This pull request was tested by: - Duplicating the Tock directory (two identical copies in `/path/to/tock` and `/path/to/tock2`), and running `make allboards`, checking that the SHA-256 checksum match. - Running `make flash` in the nRF52840-DK board's folder, and checking that flashing the kernel worked properly. - Travis-CI. ### TODO or Help Wanted This pull request still needs: - [x] Fixing netlify. Having a single `target/` folder allows to greatly simplify the `tools/build-all-docs.sh` script. I checked the output on a few files and it looks consistent, but some pages might break. On the other hand, some crates such as the [arty_e21](https://docs.tockos.org/arty_e21/index.html) board are currently not indexed on the navigation bar, and the `arty_e21` chip was not even documented so far on https://docs.tockos.org/. - [x] A bit of cleanup throughout Makefiles. ~~For now I made just enough changes in `boards/Makefile.common` for `make ci-travis` to work, but other make rules should be checked.~~ - [ ] Checking that the `tools/` still work properly with a single target folder now at the top-level. - [x] `build-all-docs.sh` Simplified, as mentioned above, now that all docs are built in the same folder. - [x] `post_size_changes_to_github.sh` Updated, but it's currently broken for other reasons (see #1722). - [x] Now fixed with a minimum search depth of 2. ~~I had to remove the `tools/check_wildcard_imports.sh` in `make ci-travis` because otherwise it reported the following error. The tool would need some refactoring.~~ ``` Wildcard import(s) found in .. Tock style rules prohibit this use of wildcard imports. The following wildcard imports were found: libraries/tock-register-interface/src/macros.rs: use super::super::*; tools/check_wildcard_imports.sh: if $(git grep -q 'use .*\*;' -- ':!src/macros.rs'); then tools/check_wildcard_imports.sh: git grep 'use .*\*;' ``` - [x] Cargo complained that the `[profile.dev]` and `[profile.release]` rules must be in the top-level workspace. I put them there given that all boards had the same profiles, but it's something to keep in mind if we want to use custom workspace for specific boards. I'm also not sure what's the impact of these profiles on the other libraries in the workspace. This didn't seem to cause any issue though. - [Optional] Should `tools/` also be part of the workspace? Have their own workspace? ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. Now the arty-e21 chip and board appear in the generated docs :) ### Formatting - [x] Ran `make formatall`. Co-authored-by: Guillaume Endignoux <guillaumee@google.com> Co-authored-by: gendx <gendx@users.noreply.github.com>
With #1714 merged, we've reached a big milestone in terms of reproducibility. I now have the same hashes when building from master on various folders on the same machine, as well as on various machines. The only remaining part is the
|
I might argue that Travis has both "pr" and "push" builds:
So I think what might make the most sense would be enforcing the reproducibility check on the |
That's a good point indeed!
One thing to note is that a commit's hash includes both the state of the repository (as a filesystem), but also the commit message, commit author, and timestamp. So Travis' merge commit will generally be different than what one can obtain locally by creating their own merge commit. For example, the latest commit on master contains the following git object.
The commit is f920947, which can then be retrieved from the command line with
Regarding reproducibility, I think that a more interesting description of the state of the repository would be the "tree" (first line in the pretty-printed commit), which is a hash of all the files checked into the repository.
Enforcing the Requiring to always rebase pull requests on the current master would avoid the merge commit problem (and I think GitHub can be configured to require rebased pull requests), at the expense of more burden for developers of pull requests (especially when there are unrelated pull requests in parallel, i.e. without any merge conflicts, in which case the current model works smoothly). |
Hmm, this is a very good point. We're partially running into a usability vs. correctness challenge here I think. While we could dig into the git internals, the intent of including a hash in the panic trace is that it's easy for developers to investigate reported errors (i.e. can reproduce by simply
One idea I thought about briefly would be modifying the label to be Stepping back a bit, I think we should ask what we would like the reproducibility mechanism to look like (this became very stream-of-conscious, but hopefully useful for discussion):
|
Definitely agree with the concerns. That would be a lot of annoyance to keep in sync on every commit.
Similarly, the overhead on developers would be quite high.
I quite like this idea (providing transparency) - some action builds the boards on each commit (or each master branch commit), and publishes the hashes. Tags may add quite some extra overhead, but I don't think they would be necessary. The UX could be similar to the size reporting tool (#1701). Then anyone can reproduce this on their machine and compare the hashes. The question that remains is how to enforce reproducibility after each commit? Ideally, there should be some CI workflow checking that builds remain reproducible. For example, if some commit removes the Line 32 in b134ba0
then the builds are not reproducible anymore across directories, but there is currently no tool in Tock's CI to prevent that.
|
While working on google/OpenSK#94, I noticed that although builds seem reproducible across Linux machines (my local development machine vs. GitHub workflows), the binaries obtained on MacOS are currently not the same. |
I added a reference to tock/elf2tab#16 to track reproducibility of |
Does anyone know of any github/rust projects that do something similar in terms of using CI to verify builds are reproducible and publishing SHAs? |
Context
When working on google/OpenSK#67, we realized that building some boards was failing to build on my desktop, but not on Travis-CI. The reason was that the paths on the filesystem are stored in the binary. These are for example included by Rust to indicate the line where some code panicked. You can check how many of these are in your binary by running
strings
on it.Because the stored paths are absolute, the size they take depends on which directory is used for building Tock, and therefore the resulting binary size will increase or decrease accordingly (can be a few KB).
But because we use custom linker scripts with a fixed layout such as:
tock/boards/nordic/nrf52840dk/layout.ld
Lines 1 to 10 in 76d1abf
it's possible that someone hits the limit of the
rom
section's size, whereas Travis-CI and/or other developers don't hit this threshold.Reproducible builds
Reproducible builds have multiple advantages.
Making builds reproducible isn't trivial, as it requires the Rust compiler (and LLVM) to give us reproducible results. But some things (such as
rust-toolchain
) are in Tock's control, and therefore I think it makes sense to track what's currently allowing and preventing reproducible builds.rust-toolchain
file in the root directory pins a given version of (nightly) Rust, so that everyone uses the same compiler.This would require support in the Rust compiler.This could be done with the--remap-path-prefix
parameter of the Rust compiler.make ci-travis
twice triggers "unused attribute" errors. #1657)Idempotent make rulesBuggy nightly compiler. On some nightly version(s) (observed onnightly-2020-02-27
), runningmake ci-travis
twice in a row on the same code triggers different warnings the second time.make ci-travis
on two distinct build folders on the same machine (e.g./path/to/tock
and/path/to/tock2
) and compare the checksums. If they don't match, investigate why and add item(s) to this list.elf2tab
. Sources of non-determinism inelf2tab
are filesystem metadata in the TAR layer (timestamp, permissions, user ID), and a build timestamp.TOCK_KERNEL_VERSION
value. Currently, this value (embedded in some panic error messages) depends on the latest git commit. Because Travis-CI adds a merge commit to check each pull-request, theTOCK_KERNEL_VERSION
will be different than on a local developing branch. There could be a rule or option in the Makefiles to build with a fixed or providedTOCK_KERNEL_VERSION
value instead.tock/boards/Makefile.common
Lines 55 to 58 in bdc5df5
make ci-travis
locally and compare the checksums with those of Travis-CI. If they don't match, investigate why and add item(s) to this list.--remap-path-prefix
parameter, making builds not reproducible.The text was updated successfully, but these errors were encountered: