-
Notifications
You must be signed in to change notification settings - Fork 937
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
Introduce initial Bazel build #1705
Introduce initial Bazel build #1705
Conversation
Introduces the initial foundations of a Bazel build, including a toolchain, critical generated headers, platform patterns, and enough BUILD files to build boot_stage2.
Uses archive_override where applicable to allow transitive bzlmod deps to propagate.
Makes an objcopy alias that redirects to the objcopy tool for the current exec platform, which allows boot_stage2 to build on Linux, macOS, and Windows.
Adds initial set of generated Bazel build files. Note that these do not yet build, as dependency cycles are present.
Fixes many dependency cycles, some were unintentionally created by the build file generator, others are true dependency cycles that require manual workarounds.
Silences a stray warning in the Bazel build.
This makes `bazel build //...` succeed, and also prevents the fetching of toolchains that aren't compatible with the current execution environment (i.e. Windows computers will no longer try to download macOS toolchains).
Finishes out the remainder of the work required to successfully compile a working blinky example.
Fixes some dependencies around pico_stdlib so that pico_stdlib links properly and UART stdio works.
Adds external an external registry for resolving Bazel module dependencies.
Provides the appropriate defines for host builds to support the picotool build.
The -ffreestanding toolchain flag is quite strict, so remove it from the Bazel toolchain.
Cuts out most of the Bazel toolchain flags and only specifies the bare-minimum set of flags. Also, adds wrapper linker flags for functions the SDK wraps.
Adds initial TinyUSB support and enough integration to get USB serial working.
Removes comments that indicates BUILD.bazel files are generated. This was used during initial bringup to indicate hand-crafted vs automatically generated BUILD.bazel files.
Prevents USB libraries from being built unless the build is properly configured to use them.
Moves toolchain configuration to use the new rules in rules_cc.
@kilograham I've put together an initial PR for adding a Bazel build, I have a few minor bits of cleanup before this is fully ready to merge, but I wanted to raise it to you now so you can start sharing any feedback/questions/concerns you may have now. @tpudlik has offered to review this from the Bazel side of things. |
Cleans up trailing whitespace and runs the black formatter on parse_version.py.
Consolidates the class/chip constraint settings to be a single constraint_setting with a config_setting that represents the rp2 class.
Includes a necessary fix for the target_compatible_with expression in the cc_toolchain to work as intended.
Moves toolchain definitions from pico.bzl to BUILD.bazel to make them easier to find and read.
Fix trivial formatting issues by running buildifier on all BUILD.bazel files.
Makes a simple objcopy rule to remove direct references to the ARM toolchains.
Critical flags were not being applied to link steps. This applies -mcpu and -mthumb to the link steps to make the produced binaries work again.
986aa77
to
ba3c006
Compare
build --@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains | ||
``` | ||
|
||
### Ready to build! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the README mention pico_config_extra_headers
and pico_config_platform_headers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added instructions for these. They'll get totally reworked shortly, but it's good to document what's there!
@@ -0,0 +1,43 @@ | |||
package(default_visibility = ["//visibility:public"]) | |||
|
|||
constraint_setting( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constraint_setting
s should really have comments explaining what they mean!
Eventually, will each header in src/boards/include/boards
have a corresponding sdk_target
constraint_value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we'll have a constraint for each board. I've added a comment to clarify. After this first PR is merged, I'll be focusing on doing a comprehensive pass over the SDK configuration and then provide better foundations for configuration options. I have some ideas here, but I'd rather build those out as a separate PR so we can review/discuss them in isolation.
bazel/parse_version.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worries me a little. It seems brittle and complex... Ideally the version information would be exposed in a file with a simpler syntax. And as an intermediate solution, maybe some duplication (a handwritten header that needs to match the CMake) would be better. https://xkcd.com/1171/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the MODULE.bazel we'll need to explicitly list a version anyway. I've reworked this script to just use that version. It's way simpler now.
"include/pico/async_context_threadsafe_background.h", | ||
], | ||
includes = ["include"], | ||
# Missing deps for: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will all of these come from FreeRTOS? Another reason to upstream our FreeRTOS build file (https://pwbug.dev/326625641)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these are all provided by FreeRTOS. Interestingly I'm not seeing CMake configuration options for turning this on/off despite it seeming to be an optional feature. I'll mark this on my TODO list.
alwayslink = True, # Ensures the wrapped symbols are linked in. | ||
) | ||
|
||
# TODO: Support printf_none.S. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand the TODO to mention why that's nontrivial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here and elsewhere. TL;DR: we need to set up configuration patterns since it's mutually exclusive.
* pico_bit_ops was incomplete. * pico_double and pico_float were trying to link in the "none" implementation.
Improves documentation and comments across the Bazel build.
Switches genrules to use skylib rules to simplify things. Reworks version header generation to use the Bazel module version rather than parsing CMake.
Moves `includes` to be enumerated on the correct library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for another pass! :)
@@ -0,0 +1,43 @@ | |||
package(default_visibility = ["//visibility:public"]) | |||
|
|||
constraint_setting( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't think we'll have a constraint for each board. I've added a comment to clarify. After this first PR is merged, I'll be focusing on doing a comprehensive pass over the SDK configuration and then provide better foundations for configuration options. I have some ideas here, but I'd rather build those out as a separate PR so we can review/discuss them in isolation.
bazel/parse_version.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the MODULE.bazel we'll need to explicitly list a version anyway. I've reworked this script to just use that version. It's way simpler now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to design everything so that the host works out-of-the-box without having to specify --platforms
or --host_platform
. I have a follow-up branch with work that gets the host working with just a bazel build //...
.
name = "rp2040", | ||
constraint_values = [ | ||
"@pico-sdk//bazel/constraint:rp2040", | ||
"@platforms//cpu:armv6-m", # This is just FYI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a well-known constraint though. Since there's a correct value, I feel we should just configure it so people aren't left confused as to why it's unset.
bazel/toolchain/BUILD.bazel
Outdated
overrides = "@rules_cc//cc/toolchains/features:dbg", | ||
) | ||
|
||
# TODO: This is required for now, but hopefully will eventually go away. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed bazelbuild/rules_cc#224
build --@rules_cc//cc/toolchains:experimental_enable_rule_based_toolchains | ||
``` | ||
|
||
### Ready to build! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added instructions for these. They'll get totally reworked shortly, but it's good to document what's there!
"include/pico/async_context_threadsafe_background.h", | ||
], | ||
includes = ["include"], | ||
# Missing deps for: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these are all provided by FreeRTOS. Interestingly I'm not seeing CMake configuration options for turning this on/off despite it seeming to be an optional feature. I'll mark this on my TODO list.
WORKSPACE Bazel projects don't support querying module version, so add a fallback of '0.0.1-WORKSPACE' so the build can succeed.
Prevents Bazel from ever trying to link malloc into the boot_stage2 binary.
A dedicated boot_stage2 platform introduces a lot of complexity that needs to be more thought-through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust that this has stabilized enough for a baseline build... since it is entirely orthogonal to the CMake build, it can evolve independently.
Introduces the beginnings of a Bazel build for the Raspberry Pi Pico. The vast majority of preexisting build system configuration options are missing, but the build produces working binaries when used from an external project.