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

Migrate build to be entirely cargo-based, with make as integration tool #4075

Merged
merged 33 commits into from
Jul 26, 2024

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jul 9, 2024

Pull Request Overview

(Supplants #4054)

Adds support to all boards for building using plain-old-cargo with make as an instrumentation tool. Building in both ways are identical to building using make before this PR.

Without this change, in order to build with cargo , one needs a large number of RUSTFLAGS along with cargo-specific arguments. With this change, simply:

$ cd boards/[BOARD-OF-CHOICE]
$ cargo build --release

This is done by adding .cargo/config.toml files to each board which include config TOMLs from boards/cargo.

With this change, it is also easy to use tools such as cargo-binutils and probe-rs which rely on being able to run cargo build with normal arguments. For example, in the sma_q3 board, it is now possible to flash and attach the JLink RTT by simply installing probe-rs and running:

cargo embed --release

Testing Strategy

Continuous Integration

TODO or Help Wanted

  • Why do we still have CARGO_FLAGS in the makefile? I think those should be set in config.toml files now.
  • What do we do about the non-additive nature of setting RUSTFLAGS?
  • Can we use cargo config get to improve the make V=1 print out? This would help users not have to dig through unstable cargo docs to figure out how to view the current configuration.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@lschuermann
Copy link
Member

If recursive includes work in the Cargo config, I'm wondering whether it would perhaps make sense to create configs for both ARM and RISC-V, and have those in turn include the common Tock config. I think it'd be nice if we could have boards only include one config file.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 9, 2024

I think there is benefit to a pick-and-choose approach. Boards (specifically out-of-tree boards) can just pick what they need without worrying about nesting. Maybe no one needs that, though.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 9, 2024

If you want to see what we can remove from make with this change, my branch is here: https://github.com/tock/tock/compare/pr4054-bradjc2-makefile?expand=1

@bradjc
Copy link
Contributor Author

bradjc commented Jul 9, 2024

Why is this happening now:

cargo test
   Compiling core v0.0.0 (/Users/bradjc/.rustup/toolchains/nightly-2024-07-08-x86_64-apple-darwin/lib/rustlib/src/rust/library/core)
   Compiling rustc-std-workspace-core v1.99.0 (/Users/bradjc/.rustup/toolchains/nightly-2024-07-08-x86_64-apple-darwin/lib/rustlib/src/rust/library/rustc-std-workspace-core)
   Compiling compiler_builtins v0.1.109
   Compiling tock-tbf v0.1.0 (/Users/bradjc/git/tock/libraries/tock-tbf)
   Compiling tock-registers v0.9.0 (/Users/bradjc/git/tock/libraries/tock-register-interface)
   Compiling tock-cells v0.1.0 (/Users/bradjc/git/tock/libraries/tock-cells)
   Compiling enum_primitive v0.1.0 (/Users/bradjc/git/tock/libraries/enum_primitive)
   Compiling tickv v1.0.0 (/Users/bradjc/git/tock/libraries/tickv)
error[E0152]: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `sized`
  |
  = note: the lang item is first defined in crate `core` (which `tock_cells` depends on)
  = note: first definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-3c7bcd9e10a450b2.rmeta
  = note: second definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-e0f1196d964aa647.rmeta

For more information about this error, try `rustc --explain E0152`.
error[E0152]: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `sized`
  |
  = note: the lang item is first defined in crate `core` (which `enum_primitive` depends on)
  = note: first definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-3c7bcd9e10a450b2.rmeta
  = note: second definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-e0f1196d964aa647.rmeta

error[E0152]: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `sized`
  |
  = note: the lang item is first defined in crate `core` (which `tock_registers` depends on)
  = note: first definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-3c7bcd9e10a450b2.rmeta
  = note: second definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-e0f1196d964aa647.rmeta

error: could not compile `tock-cells` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error[E0152]: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `sized`
  |
  = note: the lang item is first defined in crate `core` (which `tock_tbf` depends on)
  = note: first definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-3c7bcd9e10a450b2.rmeta
  = note: second definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-e0f1196d964aa647.rmeta

error: could not compile `enum_primitive` (lib) due to 1 previous error
error[E0152]: duplicate lang item in crate `core` (which `rustc_std_workspace_core` depends on): `sized`
  |
  = note: the lang item is first defined in crate `core` (which `tickv` depends on)
  = note: first definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-3c7bcd9e10a450b2.rmeta
  = note: second definition in `core` loaded from /Users/bradjc/git/tock/target/thumbv7em-none-eabi/debug/deps/libcore-e0f1196d964aa647.rmeta

error: could not compile `tock-registers` (lib) due to 1 previous error
error: could not compile `tock-tbf` (lib) due to 1 previous error
error: could not compile `tickv` (lib) due to 1 previous error

Comment on lines 13 to 15
rustflags = [
"-Z", "virtual-function-elimination"
]

Copy link
Member

Choose a reason for hiding this comment

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

Okay, maybe I'm being obnoxious, but this is exactly what we don't want I think. Letting one-offs / "this magic line is necessary for this board to work" slip into the hidden config file. I feel like we should add a cargo/earlgrey-cw310.toml or similar somewhere and include it to pull this in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, need to look into this and what happened here. That line should have already been there (aka not be green).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

As explained here in the original PR, this is not "magically need it for this board to work," this is already in the Makefiles as a "magic" VFUNC variable that is only enabled for the earlgrey board.

This is a code size optimization that is still considered unstable and dangerous (because I guess it is not always correct) but saves a bunch of code size.

Copy link
Member

Choose a reason for hiding this comment

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

In particular, I'd argue this is significantly more hidden and obscure in the Makefiles where it is abstracted behind some custom variable name you have to chase down (basically via the Makefile equivalent of #ifdefs).

Copy link
Member

Choose a reason for hiding this comment

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

I define "hidden" as if I grep for virtual-function-elimination will it show up. Living in a hidden file / directory, the answer is no; living in a Makefile or anything not hidden, the answer is yes. The . of .cargo is a cancer.

@github-actions github-actions bot added the WG-OpenTitan In the purview of the OpenTitan working group. label Jul 10, 2024
Comment on lines +17 to +27
Example:

```toml
include = [
"../../cargo/tock_flags.toml",
"../../cargo/unstable_flags.toml",
]

[unstable]
config-include = true
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Every board also seems to have some build target.

[build]
target = "thumbv7em-none-eabi"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This guide is how to use the extra configs, not how to configure a board. Maybe that fits in the porting guide.

Comment on lines 5 to 9
[unstable]
# Build test crates with the `panic = "abort"` strategy. Tests use `unwind` by
# default which causes incompatability with `core` built via `-Zbuild-std`.
panic-abort-tests = true
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have a comment about when a board might want this, and when they wouldn't.

Copy link
Member

Choose a reason for hiding this comment

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

If and only if they have tests

Comment on lines +9 to +10
"../../../cargo/virtual_function_elimination.toml",
"../../../cargo/panic_abort_tests.toml",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to leave a comment about why these are needed for earlgrey but not other boards.

Copy link
Contributor

Choose a reason for hiding this comment

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

I fear the answer might be "I don't know. The Makefile already did that for some reason."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I appreciate that, but that should have been requested when first added.

Comment on lines 16 to 17
[target.'cfg(target_arch = "riscv32")']
runner = "./run.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, it would be great to comment why this exists here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, that is already there.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this is a super standard Cargo thing. We can comment, but it basically invokes the run.sh command in the earlgray crate directory when running cargo run (in turn, this is script basically runs the opentitan qemu).

ppannuto
ppannuto previously approved these changes Jul 10, 2024
@bradjc
Copy link
Contributor Author

bradjc commented Jul 10, 2024

Anyone willing to bite on why CI fails? I don't know what's different.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 11, 2024

Any cargo aficionados know how to replicate

# `cargo bloat` does not support `-Z build-std`, so we must remove it from cargo
# flags. See https://github.com/RazrFalcon/cargo-bloat/issues/62.
# As we aren't building the library we also remove the
# `-Z build-std-features="core/optimize_for_size"` argument.
CARGO_FLAGS_TOCK_NO_BUILD_STD := $(filter-out -Z build-std=core,$(CARGO_FLAGS_TOCK))
CARGO_FLAGS_TOCK_NO_BUILD_STD := $(filter-out -Z build-std-features="core/optimize_for_size",$(CARGO_FLAGS_TOCK))

in cargo-land?

cargo test fails if build-std = ["core", "compiler_builtins" ] is set.

Also this issue seems the same? rust-lang/cargo#13146

@lschuermann
Copy link
Member

Okay, ci-tests passes now with panic = "abort".

We still have some mutable references to mutable statics in tests. I looked into fixing this proper, as there's not really a good reason for all of these to be mutable statics in the first place. However, that is a much more invasive change and will take some time to get ready. This is orthogonal to this PR, and as such I don't think we should block this on that.

Another lesson learned is that we don't promote warnings to errors in ci-tests right now. We should do that, presumably as a follow-up to the above change.

@lschuermann
Copy link
Member

lschuermann commented Jul 12, 2024

Anyone know how we can suppress the following warning that shows up in our docs builds (and generally in builds, depending on your toolchain)?

11:29:42 AM: warning: unstable feature specified for `-Ctarget-feature`: `relax`
11:29:42 AM:   |
11:29:42 AM:   = note: this feature is not stably supported; its behavior can change in the future

FWIW, I've been seeing this locally for a while now, even before these changes.

Edit: @alevy working on this!

@alevy
Copy link
Member

alevy commented Jul 12, 2024

@bradjc

Any cargo aficionados know how to replicate

# `cargo bloat` does not support `-Z build-std`, so we must remove it from cargo
# flags. See https://github.com/RazrFalcon/cargo-bloat/issues/62.
# As we aren't building the library we also remove the
# `-Z build-std-features="core/optimize_for_size"` argument.
CARGO_FLAGS_TOCK_NO_BUILD_STD := $(filter-out -Z build-std=core,$(CARGO_FLAGS_TOCK))
CARGO_FLAGS_TOCK_NO_BUILD_STD := $(filter-out -Z build-std-features="core/optimize_for_size",$(CARGO_FLAGS_TOCK))

in cargo-land?

I don't think this is actually a problem. I can run cargo bloat --release from the boards I've tested and can run cargo test --no-run in any relevant crates (not boards)

Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

Looks good to me apart from these comments.

@@ -0,0 +1,14 @@
# Licensed under the Apache License, Version 2.0 or the MIT License.
Copy link
Member

Choose a reason for hiding this comment

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

I'm skittish about having these configs in directories other than the board crate itself. It would seem to me like this inconsistency could result in confusion down the line. Same applies to, e.g., the LiteX boards.

Copy link
Member

Choose a reason for hiding this comment

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

Ah... you're probably right. The small bit of savings isn't worth the inconsistency. Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linker file is shared, what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is appropriate to put the config in the parent directory. That is consistent with not using the .cargo files for customization beyond what is strictly necessary, and our other shared build system features (eg build.rs, linker files, cargo/ toml files, Makefile.common).

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel particularly strongly about this, but one thing that's different from our build.rs, linker files, cargo/*.toml files is that these are all shared resources that are explicitly referenced by something that's in a board directory.

On the other hand, .cargo/config.toml files in any parent directory are implicitly picked up by Cargo. Once consequence of relying on configs in parent directories then is that, were you to copy a Nordic board crate to somewhere else (e.g., use it as a template for a top-level board crate in boards/), it would silently no longer take this shared config into account. This is different from, e.g., our linker script, where you'd get an error because the relative path to it has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could get behind a restriction that .cargo dirs can only exist in a folder with a Cargo.toml in Tock.

Copy link
Member

Choose a reason for hiding this comment

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

I could get behind a restriction that .cargo dirs can only exist in a folder with a Cargo.toml in Tock.

I'd support that, with the additional restriction that that Cargo.toml must not be a workspace root Cargo.toml (i.e., only actual crates should have a .cargo/config.toml). The rest we'd solve with includes, which implies that their contents will not be hidden.

@alevy
Copy link
Member

alevy commented Jul 12, 2024

We have a remaining problem, which is that RUSTFLAGS= overrides build.rustflags config (see here):

There are four mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

  1. CARGO_ENCODED_RUSTFLAGS environment variable.
  2. RUSTFLAGS environment variable.
  3. All matching target..rustflags and target..rustflags config entries joined together.
  4. build.rustflags config value.

Which means that when it's set in the Makefiles at all, it replaces anything set in the config.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 12, 2024

We have a remaining problem, which is that RUSTFLAGS= overrides build.rustflags config (see here):

There are four mutually exclusive sources of extra flags. They are checked in order, with the first one being used:

  1. CARGO_ENCODED_RUSTFLAGS environment variable.
  2. RUSTFLAGS environment variable.
  3. All matching target..rustflags and target..rustflags config entries joined together.
  4. build.rustflags config value.

Which means that when it's set in the Makefiles at all, it replaces anything set in the config.

Why is it set in the makefile?

@alevy
Copy link
Member

alevy commented Jul 12, 2024

Well, in master everything that been moved to cargo configs. In this PR, basically nothing

@alevy
Copy link
Member

alevy commented Jul 21, 2024

I think I've addressed all the outstanding issues, including:

  • Restoring checking the special config flag in build.rs to fail the build if RUSTFLAGS is set (without explicitly also setting the magic flag)
  • Using cargo config get to show RUSTFLAGS in make V=1 (turns out this does work on stable, just need to add a -Zunstable-options flag, so works on hail)
  • Move CARGO_FLAGS out of Makefile.common and into the only place it's used, which is earlgrey (in order to choose cargo features based on an environment variable).

@bradjc
Copy link
Contributor Author

bradjc commented Jul 22, 2024

  • Using cargo config get to show RUSTFLAGS in make V=1 (turns out this does work on stable, just need to add a -Zunstable-options flag, so works on hail)
*******************************************************
TOCK KERNEL BUILD SYSTEM -- VERBOSE BUILD CONFIGURATION
*******************************************************
MAKEFILE_COMMON_PATH          = /Users/bradjc/git/tock/boards/
TOCK_ROOT_DIRECTORY           = /Users/bradjc/git/tock/
TARGET_DIRECTORY              = /Users/bradjc/git/tock/target/

PLATFORM                      = hail
TARGET                        = thumbv7em-none-eabi
TOCK_KERNEL_VERSION           = release-2.1-3228-gcb00dee1f
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
RUSTFLAGS                     = )
MAKEFLAGS                     =  -r -R
OBJDUMP_FLAGS                 = --disassemble-all --source --section-headers --demangle --arch-name=thumb
OBJCOPY_FLAGS                 = --strip-sections --strip-all --remove-section .apps
SIZE_FLAGS                    =

TOOLCHAIN                     = "/Users/bradjc/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/bin/llvm"
SIZE                          = "/Users/bradjc/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/bin/llvm"-size
OBJCOPY                       = "/Users/bradjc/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/bin/llvm"-objcopy
OBJDUMP                       = "/Users/bradjc/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/x86_64-apple-darwin/bin/llvm"-objdump
CARGO                         = cargo
RUSTUP                        = rustup
SHA256SUM                     = sha256sum

?

@alevy
Copy link
Member

alevy commented Jul 22, 2024

Hrm... It worked for me, sorry! Perhaps my environment is actually picking a different cargo than yours. Will investigate.

alevy added 2 commits July 22, 2024 07:35
Instead, just override the rules in earlgrey that are the only ones that
rely on it (to set features).
@alevy
Copy link
Member

alevy commented Jul 22, 2024

@bradjc fixed I think (turns out with Nix we're not using rustup directly, so the stable channel doesn't exactly get picked up for hails). Now on hail (or whenever stable is used) it should report "Listing Rust flags only accessible on nightly cargo".

Not ideal, but at least it's not an error. I don't have a concrete suggestion for what to do otherwise.

@bradjc
Copy link
Contributor Author

bradjc commented Jul 22, 2024

Not ideal, but at least it's not an error. I don't have a concrete suggestion for what to do otherwise.

That's what I was thinking we would have to do. Hopefully cargo stabilizes this soon-ish. It doesn't seem controversial to want to see the configuration.

Copy link
Contributor Author

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

I can't approve a PR I opened but I would give this the green check at this point if I could.

Also
image
is pretty cool.

@bradjc bradjc added the P-Significant This is a substancial change that requires review from all core developers. label Jul 22, 2024
@lschuermann lschuermann requested a review from a team July 22, 2024 16:59
@bradjc bradjc mentioned this pull request Jul 22, 2024
2 tasks
Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

Did not go through all files again, but these incremental changes all look good to me and I'm in favor of this overall direction.

@ppannuto ppannuto added the last-call Final review period for a pull request. label Jul 25, 2024
@lschuermann lschuermann added this pull request to the merge queue Jul 26, 2024
Merged via the queue into master with commit 144f5a9 Jul 26, 2024
18 checks passed
@lschuermann lschuermann deleted the pr4054-bradjc2 branch July 26, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
last-call Final review period for a pull request. P-Significant This is a substancial change that requires review from all core developers. WG-OpenTitan In the purview of the OpenTitan working group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants