8000 Boards: Add a build_scripts crate by bradjc · Pull Request #4172 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Boards: Add a build_scripts crate #4172

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

Merged
merged 7 commits into from
Oct 5, 2024
Merged

Boards: Add a build_scripts crate #4172

merged 7 commits into from
Oct 5, 2024

Conversation

bradjc
Copy link
Contributor
@bradjc bradjc commented Sep 12, 2024

Pull Request Overview

Writing a suitable build.rs file is subtle and repetitive. Historically, we have copied the same build.rs file into each board crate, and we recently moved to having one copy of the build.rs file in the boards/ directory and then pointing to it from each board's Cargo.toml. This is the next iteration by putting our build.rs logic in a proper crate which can be included by each board and called from the board's build.rs.

This has two key benefits:

  1. We can implement multiple "versions" of build.rs and allow individual boards to call whichever they need without having to copy and paste build.rs files into each board crate.
  2. Out-of-tree boards can specify this crate as a dependency and use it without having to either copy our build.rs (which might easily become out of date) or link to the main Tock kernel manually in addition to using the cargo dependency manager. They also get a copy of the linker script too.

This is a draft PR for now to see if people like how this looks before switching all boards.

Testing Strategy

Compiling the nrf52840dk.

TODO or Help Wanted

Thoughts? I don't really have a sense if this makes the linker script detection/inclusion more fragile, less fragile, or the same.

I also renamed the linker script to make it more clear this is for Tock.

Documentation Updated

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

Probably need to update the out-of-tree board guide if this is something we go with.

Formatting

  • Ran make prepush.

ppannuto
ppannuto previously approved these changes Sep 16, 2024
Copy link
Member
@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

This feels like an improvement over our current build.rs story to me.

For final PR, it might make sense to push this to crates.io and have the default template / example point there instead of the git version? (Or at least a comment suggesting such for out of tree use cases). I guess out of tree folks could also pin a hash, but that doesn't feel very ergonomic for them.

@bradjc
Copy link
Contributor Author
bradjc commented Sep 17, 2024

For final PR, it might make sense to push this to crates.io and have the default template / example point there instead of the git version? (Or at least a comment suggesting such for out of tree use cases). I guess out of tree folks could also pin a hash, but that doesn't feel very ergonomic for them.

Is there a reason to treat this Tock crate differently from all others? I imagine we would just add another line like the others in Cargo.toml:

image

@ppannuto
Copy link
Member

No, I suppose not.

I think this came up on the release issue already, but I do increasingly think it makes sense to have a process where we publish versions of each of our crates on releases. It's functionally the same thing, I understand, but I think the optics are more clearly 'this is a released artifact to use and build on'.

brghena
brghena previously approved these changes Sep 27, 2024
Copy link
Contributor
@brghena brghena left a comment

Choose a reason for hiding this comment

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

I like this. I think it's pretty clean and I like using crates for this.

I don't have a feel for what the alternatives are, but I don't really see downsides here.


```toml
# Cargo.toml
...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
...
# ...

This will get the markdown diff to render correctly without all the red

@bradjc bradjc dismissed stale reviews from brghena and ppannuto via d0c6be5 October 1, 2024 18:19
@github-actions github-actions bot added the WG-OpenTitan In the purview of the OpenTitan working group. label Oct 1, 2024
bradjc added 5 commits October 1, 2024 14:41
All boards can include this as a dev dependency to use the common
build.rs functions.
Include updates on using build_scripts, but also more generally the
document was out of date.
@bradjc
Copy link
Contributor Author
bradjc commented Oct 2, 2024

I made some progress on this, including fixing the lack of rerun for the linker script and using it for each board.

CI is failing because OT is copying linker scripts around. That was in part the motivation for this PR, as having a crate should make it easier for different boards that need custom support to implement that support without introducing additional complexity for boards that do not need it (as might happen when all we have is a single shared build.rs file).

But I'm not sure what to do at this point or why we need to copy around linker scripts.

@bradjc bradjc marked this pull request as ready for review October 2, 2024 16:48
@hudson-ayers
Copy link
Contributor

I pushed a fix for the Makefiles. Most of those layout copies were completely unnecessary and just a result of having been copied from the OT Makefile. The OT Makefile only needed the test_layout.ld copy to get the tests to work, not the other one that was breaking the build.

@ppannuto ppannuto added the last-call Final review period for a pull request. label Oct 4, 2024
@hudson-ayers hudson-ayers added this pull request to the merge queue Oct 5, 2024
Merged via the queue into master with commit 772ed33 Oct 5, 2024
18 checks passed
@hudson-ayers hudson-ayers deleted the buildrs-crate branch October 5, 2024 21:30
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. WG-OpenTitan In the purview of the OpenTitan working group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0