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

bazel build: Fix compilation bugs for Pico-W support #1797

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

jaguilar
Copy link
Contributor

@jaguilar jaguilar commented Aug 11, 2024

Fixes various minor errors with the bazel build files in the pico_lwip and pico_cyw43_driver directories that were causing compilation errors.

Fixes #1796
Fixes #1798

@jaguilar jaguilar changed the title Fixes #1796: add proper @pico-sdk prefix to //bazel/config reference Fixes #1796 add proper @pico-sdk prefix to //bazel/config reference Aug 11, 2024
Copy link
Contributor

@armandomontanez armandomontanez left a comment

Choose a reason for hiding this comment

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

Thanks, I haven't had as much time to invest in Pico W so there's probably a few more rough edges that will need to be sorted out.

srcs = ["ports/freertos/sys_arch.c"],
includes = ["ports/freertos/include"],
srcs = ["contrib/ports/freertos/sys_arch.c"],
includes = ["contrib/ports/freertos/include"],
target_compatible_with = incompatible_with_config(
Copy link
Contributor

Choose a reason for hiding this comment

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

May also need hdrs = ["contrib/ports/freertos/include/arch/sys_arch.h"],

Copy link
Contributor

Choose a reason for hiding this comment

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

That header is probably getting picked up by the glob pattern in pico_lwip_core, so it might work without this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't working with or without, so I reverted this change for some more investigation.

Without this, we're trying to refer to a subpackage of
the lwip directory called bazel/config, which doesn't
exist. See similar references in this file.
@jaguilar
Copy link
Contributor Author

I'm not sure how it would be best to review the various changes needed to get LWIP working. I've set up this one pull request for this one tactical change, but there are a variety of other changes needed to the bazel build files to get to a working Bazel implementation of lwip, and especially lwip with freertos. Let me know if you'd like to see all the commits in one pull request, or a separate request for each tactical change.

@armandomontanez
Copy link
Contributor

I'm fine with either, but @kilograham is the one with the merge button.

@jaguilar
Copy link
Contributor Author

Do you happen to know the RPI people's typical preferences? They're extremely busy with the rp2350 launch right now so I'd love for this to be in the ideal condition for review whenever he gets around to it.

This fixes two general problems.

* pico_lwip_contrib_freertos misspelled several things
  (omitted contrib/ dir prefix, didn't have @pico-sdk in front of
  out references to pico-sdk targets)

  This is fixed simply by fixing the spellings.

* Circular dependency between pico_lwip_core and pico_lwip_contrib_freertos.
  In NO_SYS=0 mode, lwip wants to include sys_arch.h. But sys_arch.h
  is defined in pico_lwip_contrib_freertos. sys_arch.c in turn wants
  to include lwip's opt.h and arch.h, among other things. So it needs
  to depend on pico_lwip_core.

  This is fixed by extracting all the headers into a common rule which
  can be depended on by both targets, then depending on it in the
  relevant targets.

Additionally, for the LWIP+FreeRTOS build to work correctly, we need
to actually depend on the pico_lwip_contrib_freertos rule from
pico_lwip_core. This the purpose of the select in the deps of
pico_lwip_core.
@jaguilar jaguilar changed the title Fixes #1796 add proper @pico-sdk prefix to //bazel/config reference Add proper @pico-sdk prefix to //bazel/config reference Aug 14, 2024
@peterharperuk
Copy link
Contributor

Stick all related changes in one PR.

@jaguilar jaguilar changed the title Add proper @pico-sdk prefix to //bazel/config reference bazel build: Fix compilation bugs for Pico-W support Aug 16, 2024
This fixes issues with the cyw43 driver
build rules in Bazel:

* Before this, the btstack would always be included
  even if it could not be used. If the user did not
  specify a btstack config, this would cause a
  compilation error. Now, we condition the linking
  and building of the btstack on whether there is
  a config for it.
* Before, the btbus was not properly linked.
@jaguilar
Copy link
Contributor Author

@armandomontanez PTAL -- this now contains all the necessary changes to build the Pigweed + Pico-W thingy I showed y'all.

@kilograham
Copy link
Contributor

I'm fine with either, but @kilograham is the one with the merge button.

Yeah sorry, caught COVID at DEFCON

@jaguilar
Copy link
Contributor Author

Don't rush on my account. Take your time.

@armandomontanez
Copy link
Contributor

I'm fine with either, but @kilograham is the one with the merge button.

Yeah sorry, caught COVID at DEFCON

Get all the rest you need. :) No urgency here.

Copy link
Contributor

@armandomontanez armandomontanez left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good, a couple minor comments.

Please make sure to run ./tools/run_all_bazel_checks.py and make sure no new breakages pop up.

src/rp2_common/pico_cyw43_driver/BUILD.bazel Outdated Show resolved Hide resolved
src/rp2_common/pico_lwip/lwip.BUILD Outdated Show resolved Hide resolved
src/rp2_common/pico_lwip/lwip.BUILD Show resolved Hide resolved
Copy link
Contributor

@armandomontanez armandomontanez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR! 😁

@armandomontanez
Copy link
Contributor

@kilograham this is ready to merge whenever you're back.

@kilograham kilograham added this to the 2.0.1 milestone Aug 22, 2024
@kilograham kilograham merged commit 4cb5c1c into raspberrypi:develop Aug 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants