[bazel] Run buildifier on Bazel build files #2904

2026-04-16T03:33:29-05:00

adhoc-bobcat commented

2026-04-16 03:33:29 -05:00

(Migrated from github.com)

This is an automated change made with the following command:

$ buildifier --mode=fix --lint=fix **/BUILD.bazel \
      src/rp2_common/pico_lwip/lwip.BUILD \
      src/rp2_common/pico_mbedtls/mbedtls.BUILD \
      src/rp2_common/tinyusb/tinyusb.BUILD \
      src/rp2_common/pico_cyw43_driver/cyw43-driver.BUILD \
      src/rp2_common/pico_btstack/btstack.BUILD

The primary purpose was to get back Bazel 9 compatibility lost since the last time this was done, as doing this automatically adds the necessary rules_cc loads for its rules (cc_library, ...)

It also normalizes the formatting, in particular

Loads go at the top
Rule attributes are sorted
Deps lists are sorted
Trailing commas are added on multiline lists
Standardizes whitespace between declarations, and at the end of file

Verified by running:

$ bazelisk build //...  # Uses .bazelversion (8.1.0)
$ USE_BAZEL_VERSION=9.1.0.rc3 bazelisk build //...

This is an automated change made with the following command: ``` $ buildifier --mode=fix --lint=fix **/BUILD.bazel \ src/rp2_common/pico_lwip/lwip.BUILD \ src/rp2_common/pico_mbedtls/mbedtls.BUILD \ src/rp2_common/tinyusb/tinyusb.BUILD \ src/rp2_common/pico_cyw43_driver/cyw43-driver.BUILD \ src/rp2_common/pico_btstack/btstack.BUILD ``` The primary purpose was to get back Bazel 9 compatibility lost since the last time this was done, as doing this automatically adds the necessary rules_cc loads for its rules (cc_library, ...) It also normalizes the formatting, in particular - Loads go at the top - Rule attributes are sorted - Deps lists are sorted - Trailing commas are added on multiline lists - Standardizes whitespace between declarations, and at the end of file Verified by running: ``` $ bazelisk build //... # Uses .bazelversion (8.1.0) $ USE_BAZEL_VERSION=9.1.0.rc3 bazelisk build //... ```

adhoc-bobcat commented

2026-04-16 03:51:57 -05:00

(Migrated from github.com)

I reproduced the "bazel-build-check (ubuntu-latest)" failure, and confirmed it was my patch. I'll mark this as a draft PR until I figure what I did wrong (I'm guessing the full set of fixes was the wrong thing to do, and perhaps deps should NOT have been sorted).

will-v-pi commented

2026-04-16 13:09:35 -05:00

(Migrated from github.com)

I reproduced the "bazel-build-check (ubuntu-latest)" failure, and confirmed it was my patch. I'll mark this as a draft PR until I figure what I did wrong (I'm guessing the full set of fixes was the wrong thing to do, and perhaps deps should NOT have been sorted).

Yes, the dependency ordering for the linker scripts is important, as earlier dependencies override files in later ones.

If there's a way to run buildifier without doing the re-ordering of those dependencies, it might be nice to add that to the other Bazel CI checks (throwing an error and printing out the diff if there are any changes needed, for the PR author to copy into their PR) - that would ensure Bazel 9 compatibility doesn't get lost again

> I reproduced the "bazel-build-check (ubuntu-latest)" failure, and confirmed it was my patch. I'll mark this as a draft PR until I figure what I did wrong (I'm guessing the full set of fixes was the wrong thing to do, and perhaps deps should NOT have been sorted). Yes, the dependency ordering for the linker scripts is important, as earlier dependencies override files in later ones. If there's a way to run `buildifier` without doing the re-ordering of those dependencies, it might be nice to add that to the other Bazel CI checks (throwing an error and printing out the diff if there are any changes needed, for the PR author to copy into their PR) - that would ensure Bazel 9 compatibility doesn't get lost again

adhoc-bobcat commented

2026-04-16 13:22:42 -05:00

(Migrated from github.com)

I reproduced the "bazel-build-check (ubuntu-latest)" failure, and confirmed it was my patch. I'll mark this as a draft PR until I figure what I did wrong (I'm guessing the full set of fixes was the wrong thing to do, and perhaps deps should NOT have been sorted).

Yes, the dependency ordering for the linker scripts is important, as earlier dependencies override files in later ones.

If there's a way to run buildifier without doing the re-ordering of those dependencies, it might be nice to add that to the other Bazel CI checks (throwing an error and printing out the diff if there are any changes needed, for the PR author to copy into their PR) - that would ensure Bazel 9 compatibility doesn't get lost again

Hi Will!

There are "buildifier: leave-alone" and even a check for "do not sort" comments that can be added, which may not be quite what you want. I added them in a test branch (to not add noise to this PR) about an hour ago after tracking down the deps where the order mattered, and reverting that part of the CL:

Test branch: https://github.com/adhoc-bobcat/pico-sdk/tree/refs/heads/buildifier_test
Seems to have passed: https://github.com/adhoc-bobcat/pico-sdk/actions

I was about to update this PR with that commit. And while I didn't think about adding a buildifier check to the presubmit tests, I was going to look into adding a Bazel 9 check build, but I can look into that too.

> > I reproduced the "bazel-build-check (ubuntu-latest)" failure, and confirmed it was my patch. I'll mark this as a draft PR until I figure what I did wrong (I'm guessing the full set of fixes was the wrong thing to do, and perhaps deps should NOT have been sorted). > > Yes, the dependency ordering for the linker scripts is important, as earlier dependencies override files in later ones. > > If there's a way to run `buildifier` without doing the re-ordering of those dependencies, it might be nice to add that to the other Bazel CI checks (throwing an error and printing out the diff if there are any changes needed, for the PR author to copy into their PR) - that would ensure Bazel 9 compatibility doesn't get lost again Hi Will! There are "buildifier: leave-alone" and even a check for "do not sort" comments that can be added, which may not be quite what you want. I added them in a test branch (to not add noise to this PR) about an hour ago after tracking down the deps where the order mattered, and reverting that part of the CL: - Test branch: https://github.com/adhoc-bobcat/pico-sdk/tree/refs/heads/buildifier_test - Seems to have passed: https://github.com/adhoc-bobcat/pico-sdk/actions I was about to update this PR with that commit. And while I didn't think about adding a buildifier check to the presubmit tests, I was going to look into adding a Bazel 9 check build, but I can look into that too.

armandomontanez commented

2026-04-16 14:31:46 -05:00

(Migrated from github.com)

Buildifier will sort attributes with the following names:

github.com/bazelbuild/buildtools@05d2ebe49b/tables/tables.go (L110-L158)

Buildifier will sort attributes with the following names: https://github.com/bazelbuild/buildtools/blob/05d2ebe49b0f41aee2d480a72d5bcb0c20e26448/tables/tables.go#L110-L158

adhoc-bobcat commented

2026-04-16 15:04:30 -05:00

(Migrated from github.com)

I have what should be a Bazel 9 check (and a Bazel latest advisory check) as a local commit, but I'll need to upload it to my fork to see if it works as intended.

I also have a working buildifier check which does run locally, and which runs on more files than I initially cleaned up (WORKSPACE, *.bzl, REPO.bazel). I'm now working on getting those to be lint-clean, which requires some manual intervention to satisfy checks about missing docstrings et al.

After that, I'll do some testing on my fork that they catch regressions, but I think that won't take long. I think I'll have it working over the next few hours, but I've got some other things on my calendar today.

I was planning on adding them to this PR, since it makes sense for them to all go in together, but if needed I'm happy to set up a new PR.

I have what should be a Bazel 9 check (and a Bazel latest advisory check) as a local commit, but I'll need to upload it to my fork to see if it works as intended. I also have a working buildifier check which does run locally, and which runs on more files than I initially cleaned up (WORKSPACE, *.bzl, REPO.bazel). I'm now working on getting those to be lint-clean, which requires some manual intervention to satisfy checks about missing docstrings et al. After that, I'll do some testing on my fork that they catch regressions, but I think that won't take long. I think I'll have it working over the next few hours, but I've got some other things on my calendar today. I was planning on adding them to this PR, since it makes sense for them to all go in together, but if needed I'm happy to set up a new PR.

adhoc-bobcat commented

2026-04-16 16:43:54 -05:00

(Migrated from github.com)

I pulled out the latest Bazel ("last_rc") advisory check. It only checks a floating "9.x" version (latest released Bazel 9), which might be somewhat problematic on its own if a new version is released that requires changes. A "last_rc" check might be equally problematic, and if there is no release candidate, it duplicates the "9.x" run.

Otherwise it looks like the Bazel 9 check and the buildifier check both work as intended.

I pulled out the latest Bazel ("last_rc") advisory check. It only checks a floating "9.x" version (latest released Bazel 9), which might be somewhat problematic on its own if a new version is released that requires changes. A "last_rc" check might be equally problematic, and if there is no release candidate, it duplicates the "9.x" run. Otherwise it looks like the Bazel 9 check and the buildifier check both work as intended.

adhoc-bobcat commented

2026-04-16 16:50:44 -05:00

(Migrated from github.com)

Maybe it works too well. It flagged one picotool file because it found it, though that isn't really part of the code here. I'll create a PR there.

pico-sdk/lib/picotool/bazel/defs.bzl:1: module-docstring: The file has no module docstring.

Maybe it works too well. It flagged one picotool file because it found it, though that isn't really part of the code here. I'll create a PR there. ``` pico-sdk/lib/picotool/bazel/defs.bzl:1: module-docstring: The file has no module docstring. ```

adhoc-bobcat commented

2026-04-22 17:22:18 -05:00

(Migrated from github.com)

To make it clear, my last change altered the buildifier checks so that only files that are checked in are checked, which naturally excludes the picotool files. The presubmit checks now pass.

I think this PR is ready, and will be happy to make any adjustments necessary.

To make it clear, my last change altered the buildifier checks so that only files that are checked in are checked, which naturally excludes the picotool files. The presubmit checks now pass. I think this PR is ready, and will be happy to make any adjustments necessary.

lurch (Migrated from github.com) reviewed

2026-04-23 05:20:17 -05:00

src/rp2350/pico_platform/BUILD.bazel

					
				@ -86,6 +85,9 @@ cc_library(

				    visibility = [

				        "//src/rp2_common/pico_standard_link:__pkg__",

I'm afraid I know nothing about Bazel or linker-scripts, but should src/rp2350/pico_platform/BUILD.bazel have the same

    # The order of the linker scripts below is important.
    # Prevent buildifier from reordering them.
    # buildifier: leave-alone

comments that src/rp2040/pico_platform/BUILD.bazel does?

I'm afraid I know nothing about Bazel or linker-scripts, but should `src/rp2350/pico_platform/BUILD.bazel` have the same ``` # The order of the linker scripts below is important. # Prevent buildifier from reordering them. # buildifier: leave-alone ``` comments that `src/rp2040/pico_platform/BUILD.bazel` does?

will-v-pi (Migrated from github.com) reviewed

2026-04-23 05:40:15 -05:00

src/rp2350/pico_platform/BUILD.bazel

					
				@ -86,6 +85,9 @@ cc_library(

				    visibility = [

				        "//src/rp2_common/pico_standard_link:__pkg__",

Yes, it should - it probably isn't breaking things because RP2350 doesn't override any of the default files

adhoc-bobcat (Migrated from github.com) reviewed

2026-04-23 12:20:29 -05:00

src/rp2350/pico_platform/BUILD.bazel

					
				@ -86,6 +85,9 @@ cc_library(

				    visibility = [

				        "//src/rp2_common/pico_standard_link:__pkg__",

Sorry I missed those. I'll add the leave-alone, and revert any order changes.

Sorry I missed those. I'll add the `leave-alone`, and revert any order changes.

adhoc-bobcat (Migrated from github.com) reviewed

2026-04-23 13:46:42 -05:00

src/rp2350/pico_platform/BUILD.bazel

					
				@ -86,6 +85,9 @@ cc_library(

				    visibility = [

				        "//src/rp2_common/pico_standard_link:__pkg__",

I fixed up src/rp2350/pico_platform/BUILD.bazel.

I additionally did a grep for (regex) linker.script, memmap_ and _script.

Out of caution, I added a "leave-alone" comment to the list for the non-bare metal case deps list in pico_standard_link in src/rp2_common/pico_standard_link/BUILD.bazel, though that list appears to be sorted already.

I also saw a small number of cases like rp2350_linker_scripts in src/rp2350/pico_platform/script_include/BUILD.bazel where there was a deps list with just a single value. I could see that changing at some point, but I decided to leave it alone.

I fixed up `src/rp2350/pico_platform/BUILD.bazel`. I additionally did a grep for (regex) `linker.script`, `memmap_` and `_script`. Out of caution, I added a "leave-alone" comment to the list for the non-bare metal case deps list in `pico_standard_link` in `src/rp2_common/pico_standard_link/BUILD.bazel`, though that list appears to be sorted already. I also saw a small number of cases like `rp2350_linker_scripts` in `src/rp2350/pico_platform/script_include/BUILD.bazel` where there was a deps list with just a single value. I could see that changing at some point, but I decided to leave it alone.

armandomontanez (Migrated from github.com) reviewed

2026-04-24 13:37:52 -05:00

BUILD.bazel

Firstly: do we need this? I'm not seeing it referenced. If we want to add this, maybe tools/bazel_buildifier_lint_check.py should use this.

Second:

If you want a top-level buildifier shortcut for convenience, we still need to be careful about how we add it since this is a dev_dependency:

When you're just adding a dep, this is less sensitive. The instant you do a load from a dev-dependency, anyone who lacks that dependency fails to parse the BUILD file completely. Not an issue yet, but it immediately prevents the root package of this module from being parsable by a downstream user in the future.

The simplest solution is to just make this a private alias:

alias(
    name = "buildifier",
    actual = "@buildifier_prebuilt//:buildifier",
    visibility = ["//visibility:private"],
)

Firstly: do we need this? I'm not seeing it referenced. If we want to add this, maybe `tools/bazel_buildifier_lint_check.py` should use this. Second: If you want a top-level `buildifier` shortcut for convenience, we still need to be careful about how we add it since this is a `dev_dependency`: When you're just adding a `dep`, this is less sensitive. The instant you do a `load` from a dev-dependency, anyone who lacks that dependency fails to parse the `BUILD` file completely. Not an issue yet, but it immediately prevents the root package of this module from being parsable by a downstream user in the future. The simplest solution is to just make this a private alias: ``` alias( name = "buildifier", actual = "@buildifier_prebuilt//:buildifier", visibility = ["//visibility:private"], ) ```

bazel/util/multiple_choice_flag.bzl

					
				@ -1,5 +1,8 @@

				def declare_flag_choices(flag, choices):

				    """Declares a `config_setting` for each known choice for the provided flag.

				"""Utilities for creating multiple choice flags."""

name is unused here. You can disable the Buildifier lint that is complaining about needing a name. Normally, I prefer ensuring there's a name and that it does something, but it doesn't really make sense for this macro.

Also, using a signature like this prevents passing the args positionally:

def declare_flag_choices(*, flag, choices):

`name` is unused here. You can disable the Buildifier lint that is complaining about needing a name. Normally, I prefer ensuring there's a `name` and that it does something, but it doesn't really make sense for this macro. Also, using a signature like this prevents passing the args positionally: ```python def declare_flag_choices(*, flag, choices): ```

adhoc-bobcat (Migrated from github.com) reviewed

2026-04-28 01:29:08 -05:00

bazel/util/multiple_choice_flag.bzl

					
				@ -1,5 +1,8 @@

				def declare_flag_choices(flag, choices):

				    """Declares a `config_setting` for each known choice for the provided flag.

				"""Utilities for creating multiple choice flags."""

Ok, I've disabled the lint warning (I was on the fence about doing it that way), and added the *.

Ok, I've disabled the lint warning (I was on the fence about doing it that way), and added the `*`.

adhoc-bobcat (Migrated from github.com) reviewed

2026-04-28 01:29:18 -05:00

BUILD.bazel

Firstly: do we need this? I'm not seeing it referenced. If we want to add this, maybe tools/bazel_buildifier_lint_check.py should use this.

Well then, I should have reviewed this change a bit more closely.

I started off adding buildifier by following the official instructions at https://github.com/bazelbuild/buildtools/tree/main/buildifier#setup-and-usage-via-bazel, adding the few lines suggested there to the root BUILD.bazel (and creating one).

I then got busy with the other changes, and when I got around to putting together tools/bazel_buildifier_lint_check.py, I just directly ran "@buildifier_presubmit//:buildifier", since I wasn't just blindly following instructions.

I've removed this BUILD.bazel file as unneeded.

Second:

If you want a top-level buildifier shortcut for convenience, we still need to be careful about how we add it since this is a dev_dependency:

When you're just adding a dep, this is less sensitive. The instant you do a load from a dev-dependency, anyone who lacks that dependency fails to parse the BUILD file completely. Not an issue yet, but it immediately prevents the root package of this module from being parsable by a downstream user in the future.

The simplest solution is to just make this a private alias:
alias(
    name = "buildifier",
    actual = "@buildifier_prebuilt//:buildifier",
    visibility = ["//visibility:private"],
)

Ack, again I was just blindly following the suggested instructions, and wasn't questioning them too much.

> Firstly: do we need this? I'm not seeing it referenced. If we want to add this, maybe `tools/bazel_buildifier_lint_check.py` should use this. Well then, I should have reviewed this change a bit more closely. I started off adding buildifier by following the official instructions at https://github.com/bazelbuild/buildtools/tree/main/buildifier#setup-and-usage-via-bazel, adding the few lines suggested there to the root BUILD.bazel (and creating one). I then got busy with the other changes, and when I got around to putting together tools/bazel_buildifier_lint_check.py, I just directly ran "@buildifier_presubmit//:buildifier", since I wasn't just blindly following instructions. I've removed this BUILD.bazel file as unneeded. > > Second: > > If you want a top-level `buildifier` shortcut for convenience, we still need to be careful about how we add it since this is a `dev_dependency`: > > When you're just adding a `dep`, this is less sensitive. The instant you do a `load` from a dev-dependency, anyone who lacks that dependency fails to parse the `BUILD` file completely. Not an issue yet, but it immediately prevents the root package of this module from being parsable by a downstream user in the future. > > The simplest solution is to just make this a private alias: > > ``` > alias( > name = "buildifier", > actual = "@buildifier_prebuilt//:buildifier", > visibility = ["//visibility:private"], > ) > ``` Ack, again I was just blindly following the suggested instructions, and wasn't questioning them too much.