Use a Cargo workspace to make builds reproducible and speed up CI.

[gendx]

Pull Request Overview

This pull request adds a Cargo workspace 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:

• 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 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/.
• 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.
  • build-all-docs.sh Simplified, as mentioned above, now that all docs are built in the same folder.
  • post_size_changes_to_github.sh Updated, but it's currently broken for other reasons (see stop running size reporting script in travis #1722).
• 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 .*\*;'
  

• 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

• Updated the relevant files in /docs, or no updates are required. Now the arty-e21 chip and board appear in the generated docs :)

Formatting

• Ran make formatall.

[bradjc]

I think this is something we have wanted for a while.

[gendx]

I'm trying to understand what's wrong with Netlify. The current build-all-docs.sh tool is expecting a target/ folder in each board, whereas a top-level workspace will have all docs in the same top-level target/ folder, but I'm not sure about the implications of it (e.g. the kernel crate should appear only once because all boards depend on the same kernel).

---

I also hit the following warnings when running make alldoc.

1. Cargo now complains about unused #![no_main] attributes. This isn't new, but a known issue, see Tracking: Deny warnings for documentation builds #1334.

warning: unused attribute
 --> boards/imix/src/main.rs:7:1
  |
7 | #![no_main]
  | ^^^^^^^^^^^
  |
  = note: `#[warn(unused_attributes)]` on by default

warning: crate-level attribute should be in the root module
 --> boards/imix/src/main.rs:7:1
  |
7 | #![no_main]
  | ^^^^^^^^^^^

2. The arty-e21 board and the arty_e21 chip are colliding as far as documentation is concerned. We will need some renaming.

Documenting arty-e21
make[1]: Entering directory '/tock/boards/arty-e21'
warning: output filename collision.
The lib target `arty_e21` in package `arty_e21 v0.1.0 (/tock/chips/arty_e21)` has the same output filename as the bin target `arty-e21` in package `arty-e21 v0.1.0 (/tock/boards/arty-e21)`.
Colliding filename is: /tock/target/riscv32imac-unknown-none-elf/doc/arty_e21/index.html
The output filenames should be unique.
This is a known bug where multiple crates with the same name use
the same path; see <https://github.com/rust-lang/cargo/issues/6313>.
If this looks unexpected, it may be a bug in Cargo. Please file a bug report at
https://github.com/rust-lang/cargo/issues/ with as much information as you
can provide.
cargo 1.43.0-nightly (bda50510d 2020-03-02) running on `x86_64-unknown-linux-gnu` target `riscv32imac-unknown-none-elf`
First unit: Unit { pkg: Package { id: PackageId { name: "arty_e21", version: "0.1.0", source: "/tock/chips/arty_e21" }, ..: ".." }, target: Target { ..: lib_target("arty_e21", ["lib"], "/tock/chips/arty_e21/src/lib.rs", Edition2018) }, profile: Profile { name: "doc", debuginfo: Some(2), debug_assertions: true, overflow_checks: true, incremental: true, ..: default() }, kind: Target(CompileTarget { name: "riscv32imac-unknown-none-elf" }), mode: Doc { deps: true }, features: [] }
Second unit: Unit { pkg: Package { id: PackageId { name: "arty-e21", version: "0.1.0", source: "/tock/boards/arty-e21" }, ..: ".." }, target: Target { name: "arty-e21", doc: true, ..: with_path("/tock/boards/arty-e21/src/main.rs", Edition2018) }, profile: Profile { name: "doc", debuginfo: Some(2), debug_assertions: true, overflow_checks: true, incremental: true, ..: default() }, kind: Target(CompileTarget { name: "riscv32imac-unknown-none-elf" }), mode: Doc { deps: true }, features: [] }

3. A bunch of documentation wasn't formatted properly.

warning: `[0]` cannot be resolved, ignoring it.
  --> chips/nrf52/src/ppi.rs:17:40
   |
17 | //! * 20        TIMER0->EVENTS_COMPARE[0]       RADIO->TASKS_TXEN
   |                                        ^ cannot be resolved, ignoring
   |
   = note: `#[warn(intra_doc_link_resolution_failure)]` on by default
   = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`

[hudson-ayers]

this breaks the memory/flash reporting tool as well, I can try to push a fix for that by tomorrow

[gendx]

this breaks the memory/flash reporting tool as well, I can try to push a fix for that by tomorrow

I managed to find a workaround. However, the Travis-CI logs show the following "bad credentials" errors, and indeed I don't see any reports here.

+/home/travis/build/tock/tock/tools/diff_memory_usage.py previous-benchmark-imix current-benchmark-imix size-diffs-imix.txt imix
+'[' -s size-diffs-imix.txt ']'
++grep -hs '^' size-diffs-imix.txt
+RES='flash use -200 bytes (-0.12%), RAM use +0 bytes(+0.00%)'
+curl -X POST -H 'Content-Type: application/json' --header 'Authorization: token ' --data '{"state": "success", "context": "imix", "description": "flash use -200 bytes (-0.12%), RAM use +0 bytes(+0.00%)"}' https://api.github.com/repos/tock/tock/statuses/487760598ce3a707f3ed8baa9328b12150856585
{
  "message": "Bad credentials",
  "documentation_url": "https://developer.github.com/v3"
}

[gendx]

Another advantage of the Cargo workspace is that there can be a single build cache across boards, e.g. for common libraries such as the kernel, capsules, etc. This means that the kernel can be compiled just once (per CPU architecture) when running make allboards, instead of one time for each board (when each board has its own target/ directory).

However, because Cargo looks at all the compilation flags (including those passed in RUSTFLAGS) to determine whether a build can be cached, and because we are passing each board's linker script as a flag in RUSTFLAGS, such caching of shared libraries (such as the kernel) isn't happening automatically.

I refactored (0c0f4df) the Makefile to use cargo rustc and pass the linker script folder as a flag there, because contrary to RUSTFLAGS, parameters passed to cargo rustc aren't propagated to the dependencies.

Following are some benchmarks showing the latency in running make ci in various scenarios. It is run twice for each scenario, in order to also show how much caching happens after a build compared to a clean state of the repository.

---

Before this pull request:

$ time make ci
real	6m41.864s
user	14m20.636s
sys	1m15.273s

$ time make ci
real	3m13.732s
user	5m17.785s
sys	0m45.885s

With a workspace, but before isolating per-board linker flags in cargo rustc:

$ time make ci
real	5m23.477s
user	11m8.481s
sys	0m53.869s

$ time make ci
real	3m42.967s
user	7m27.588s
sys	0m37.029s

After:

$ time make ci
real	3m58.004s
user	6m35.670s
sys	0m44.669s

$ time make ci
real	0m56.839s
user	0m39.727s
sys	0m23.313s

[bradjc]

What happens if someone creates a new crate but doesn't update the root Cargo.toml? Will something tell them they forgot?

[gendx]

What happens if someone creates a new crate but doesn't update the root Cargo.toml? Will something tell them they forgot?

Yes :)

For example, if I remove boards/imix from the workspace's members and try to build the board, Cargo will immediately complain with the following error message:

error: current package believes it's in a workspace when it's not:
current:   /tock/boards/imix/Cargo.toml
workspace: /tock/Cargo.toml

this may be fixable by adding `boards/imix` to the `workspace.members` array of the manifest located at: /tock/Cargo.toml
Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.

[gendx]

I've solved the merge conflicts.

A few notes for the last-call:

• I've renamed the arty_e21 chip to arty_e21_chip, and the arty-e21 board to arty_e21. Things should be consistent, and this now allows both to be documented.
• I've updated a bunch of paths in the .jlink files. I tried to be as thorough as possible, but haven't tested them on the actual hardware. On the other hand, a few paths seemed to be outdated already.

Regarding Clippy, I noticed that running the script as ./tools/run_clippy.sh only checks a subset of the crates in the workspace. One has to go in each board and call cargo clippy from there to get many more warnings.

[bradjc]

* I've renamed the `arty_e21` chip to `arty_e21_chip`, and the `arty-e21` board to `arty_e21`. Things should be consistent, and this now allows both to be documented.

That sounds good to me. Hopefully we can remove the arty_e21 crates soon once more risc-v hardware is available.

Regarding Clippy, I noticed that running the script as ./tools/run_clippy.sh only checks a subset of the crates in the workspace. One has to go in each board and call cargo clippy from there to get many more warnings.

Yeah, fixing clippy lints is a lot of work so I only tried on some crates. That is definitely a work in progress / experiment to see if it is useful.

[gendx]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push