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] Add scripts to validate pregeneration tools #7690

Open
wants to merge 7 commits into
base: 2027
Choose a base branch
from

Conversation

pjreiniger
Copy link
Contributor

This might feel like a lot just to duplicate the .github/workflows/pregenerate.yml action, but it shows a powerful thing that bazel can do, and is eating up a lot of diff space between wpilibsuite and my mega-fork.

This adds bazel'ability to the pregeneration (generate_numbers.py, generate_nanopb.py, etc but not upstream_utils). Each individual script could now be run with bazel run //wpimath:generate_numbers, etc. But the really powerful thing is that they can be automatically at build time, and the generation results can be compared against the source tree in a unit test. Bazel's cache is respected across builds, so this autogeneration will almost never result in a cahche miss and have to be re-run. All of the pregen stuff can be run at once by running bazel run //:write_all.

When the output is a folder instead of a single file, all of the contents of the folder must be update'able at once, which forced making a new script for subprojects that run multiple things, i.e. wpimath running generate_numbers + generate_nanopb + generate_quickbuf.

In addition, this unlocked the ability to build the examples. You can't really read the examples.json file during the analysis phase to create targets, but you can add a task to generate file which can define targets. Eventually the examples will need to be build against shared libraries so they can function correctly, but this at least makes sure they are always compile'able in bazel. A common breaking point I've had is intellisense grabbing code across boundries, like ProjectB.Drivetrain trying to import ProjectA.Constants. It seems like gradle allows that happen, but bazel will not.

@pjreiniger pjreiniger requested review from PeterJohnson and a team as code owners January 15, 2025 07:25
@github-actions github-actions bot added component: ntcore NetworkTables library component: wpiutil WPI utility library component: wpilibj WPILib Java component: wpilibc WPILib C++ component: hal Hardware Abstraction Layer component: command-based WPILib Command Based Library component: wpimath Math library component: examples component: wpiunits Java units library labels Jan 15, 2025
Fork Sync: Update from parent repository
@PeterJohnson
Copy link
Member

The reason we checked in generated code and don't generate at build time is the substantial additional requirements for doing so (particularly protobuf and the quickbuf plugin for it). Are these requirements still optional for the build with this change?

@pjreiniger
Copy link
Contributor Author

They are not optional, but are all downloaded / compiled hermetically and sandboxed from the rest of your machine. If you notice, the CI job was not updated but there is no additional need to do a pip install or an apt-get, but the unit tests still work on Windows, Mac and Linux

  • The pip deps are hermetically installed with rules_python
  • The appropriate quickbuf is downloaded for the host build machine
  • The in-tree nanopb generator is used
  • protoc is built from source for the host machine with the help of rules_proto

Since everything is downloaded at build time, the results become more deterministic (don't need to worry if you pip install'd a different version of jinja, or even have to pip install at all), and you don't need to worry about finding the appropriate version of something like the quickbuf compiler and adding it to your path. You just one-click-build with bazel test //... and should get the same results as anyone running the build on any computer

@PeterJohnson
Copy link
Member

  • The appropriate quickbuf is downloaded for the host build machine

The downside is that this constrains the build to those specific host build machines, correct? What about arm64 hosts for example?

@pjreiniger
Copy link
Contributor Author

So if there is a host architecture that doesn't have pre-built dependency for itself (quickbuf, if one of the pip install's needs native code, etc) that would probably be a problem.

I can probably add a pregeneration tag to the targets, which should be able to be skipped if the user adds --build_tag_filters=-pregeneration to their user.rc or command line. I can verify that later

@pjreiniger pjreiniger changed the base branch from main to 2027 January 18, 2025 01:55
@github-actions github-actions bot added the 2027 2027 target label Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2027 2027 target component: command-based WPILib Command Based Library component: examples component: hal Hardware Abstraction Layer component: ntcore NetworkTables library component: wpilibc WPILib C++ component: wpilibj WPILib Java component: wpimath Math library component: wpiunits Java units library component: wpiutil WPI utility library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants