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,dvsim] remove gen-binaries.py's reliance on meson_init.sh script #12447

Closed
5 tasks
Tracked by #12449
timothytrippel opened this issue May 3, 2022 · 5 comments · Fixed by #12634
Closed
5 tasks
Tracked by #12449

[bazel,dvsim] remove gen-binaries.py's reliance on meson_init.sh script #12447

timothytrippel opened this issue May 3, 2022 · 5 comments · Fixed by #12634
Assignees
Labels

Comments

@timothytrippel
Copy link
Contributor

timothytrippel commented May 3, 2022

The OTBN gen-binaries.py script is used by the hw/ip/otbn/dv/uvm/otbn_sim_cfg.hjson dvsim.py config file to build the necessary OTBN images to run simulations. Specifically, the gen-binaries.pyscript invokes meson-generated ninja build rules under the hood to generate the binaries. Once we deprecate meson, this approach will no longer work. To migrate these tools to make use of bazel, what needs to happen is:

  • go through the gen-binaries.py script and determine if additional bazel rules must be added (in addition to the OTBN rules that already exist) to build the necessary OTBN binaries (I think the answer should be no?)
  • update the hw/ip/otbn/dv/uvm/otbn_sim_cfg.hjson config file to invoke bazel directly to build OTBN images that the gen-binaries.py script (and therefore meson) would normally build
  • remove gen-binaries.py script
  • replace the run-some.py script with a set of opentitan_functests
  • remove the run-some.py script

PLEASE DISREGARD THE ABOVE TASKS IN FAVOR OF THE ONES LISTED BELOW.
(the solution to this issue has evolved)

@arunthomas arunthomas added the Priority:P0 Priority: critical label May 3, 2022
@arunthomas arunthomas assigned arunthomas and unassigned drewmacrae May 4, 2022
@timothytrippel timothytrippel changed the title [bazel] bazelify OTBN gen-binaries.py script [bazel] remove / replace gen-binaries.py and run-some.py scripts with bazel rules May 5, 2022
@timothytrippel timothytrippel changed the title [bazel] remove / replace gen-binaries.py and run-some.py scripts with bazel rules [bazel,dvsim] remove / replace gen-binaries.py and run-some.py scripts with bazel rules May 5, 2022
@rswarbrick
Copy link
Contributor

I thought P0 meant "people are blocked and no-one can do any work". I can't believe that applies here. Anyway...

I think this is slightly backwards: the gen-binaries script generates one or more randomised binaries using the random instruction generator. There's no real interaction with Bazel, except that we've historically used the meson toolchain file to figure out what the underlying riscv toolchain should be. As far as I can tell, all we really need is to get Bazel to tell us how to run an assembler and linker.

This is not otherwise part of the software build system, so I don't see any need for replacing it with Bazel rules.

@timothytrippel
Copy link
Contributor Author

hmm, after digging into the script more, its seems the gen-binaries.py script:

  1. first generates a ninja build file with targets that:
    1. invoke the otbn-rig tool to generate a file with a series of random (OTBN) instructions
    2. invoke the RV32 + OTBN assembler
    3. invoke the RV32 + OTBN linker
  2. invoke ninja to build the targets described in the generated ninja build file
    Then, it seems, dvsim.py is configured to run ./meson_init.sh as the "pre build" command and the gen-binaries.py script as a "pre run" command.

When meson is removed, ninja will be removed too (essentially meson just generates ninja build files). To convert this setup to use bazel, we have a couple options:

  1. change the gen-binaries.py script to
    1. generate a bazel build file with rules that accomplish the same steps as the ninja build rules (this will also require a custom bazel rule to invoke the RIG tool.)
    2. invoke bazel to build the targets
  2. Write a bazel macro to replace the gen-binaries.py script that, generates all the required OTBN assembly files with the RIG (this will require a custom bazel rule that invokes the RIG, same as the above option),
    1. then feeds the generated assembly files to otbn_binary rules (defined in rules/otbn.bzl),
    2. which will invoke the RV32 + OTBN assembler / linker correctly (using the toolchains managed by bazel)

Unfortunately with option 1, since the gen-binaries.py script is a python script, we will need to write a bazel py_binary rule for it, so that it can be invoked using the hermetic python toolchain bazel manages (important for airgapped environments). At which point, we will have a rather unconventional setup where dvsim.py uses bazel to run the gen-binaries.py py_binary which itself, under the hood, generates more bazel build rules on the fly, and then invokes bazel again through the shell (essentially bazel will invoke itself). Additionally, having a python script that generates bazel build rules re-implements the same functionality that bazel macros already provides.

With option 2, dvsim.py will still invoke bazel, but bazel won't invoke itself under the hood, which would be the more appropriate way of doing things. Does that make sense?

Basically, option 2, will look pretty much like what I have described above, with the exception that the first task will require additional bazel rules / macros be developed.

@timothytrippel timothytrippel added Priority:P1 Priority: high and removed Priority:P0 Priority: critical labels May 6, 2022
@rswarbrick
Copy link
Contributor

To be clear, we're running ninja because it's an easy way to build stuff in parallel. I could have done it with Make or any other similar system. This has absolutely nothing to do with the Meson vs. Bazel change, except possibly a missing build dependency. If you're feeling absolutely allergic to keeping ninja installed, please port the script to use Make. (But I would view this as rather pointless busy-work, so I'd suggest not doing so).

Looking at your two options, (1) is kind of what I'm talking about above, but pulling in the juggernaut that is Bazel when all we want to do is run some commands in parallel. This seems a bit nuts to me. (2) is fine, I guess. But the existing script isn't completely trivial and it ties yet more of the non-SW ecosystem into having to run arcane Bazel commands. I'm not happy about this, but I guess I've lost the argument.

My preferred option: Treat ninja as a tool that we expect to be installed (and track it with whatever runes are required to get the hermetic properties that you want). Leave gen-binaries.py unchanged (I guess you'll have to rename it to gen_binaries.py because of Bazel being ridiculous, but eh, whatever). Then dvsim can run it as before (with the -j1), but now under Bazel: that should work fine. The verilator-based run-some.py script then doesn't need touching at all.

@rswarbrick
Copy link
Contributor

Oh, sorry, I missed something: I'd expect the Bazel-driven wrapper for gen_binaries.py to pass the risc-v assembler and linker as command line arguments or in the environment, to preserve the hermeticity property that you need.

@timothytrippel
Copy link
Contributor Author

timothytrippel commented May 6, 2022

After our offline discussion, I came to the understanding that the meson_init.sh script that is invoked in otbn_sim_cfg.hjson is not needed, and deleting it will not be an issue, so we can still use ninja under the hood as a parallel job runner. Additionally, bazel will just be needed to pass the gen-binaries.py script the paths to the right tools to use. Therefore, as per our discussion offline, we have come up with an even easier/simpler solution (so the options / task lists above may be disregarded now):

  • update gen-binaries.py to use the bazel provided tools (either by expecting them to be at a certain path, or providing them as env. vars, or input args) and also allow running the gen-binaries.py without bazel
  • update the otbn_sim_cfg.hjson to remove the call to meson_init.sh before invoking the gen-binaries.py script

Thanks for clearing this up @rswarbrick !

@timothytrippel timothytrippel changed the title [bazel,dvsim] remove / replace gen-binaries.py and run-some.py scripts with bazel rules [bazel,dvsim] removegen-binaries.py's reliance on meson_init.sh script May 12, 2022
@timothytrippel timothytrippel changed the title [bazel,dvsim] removegen-binaries.py's reliance on meson_init.sh script [bazel,dvsim] remove gen-binaries.py's reliance on meson_init.sh script May 12, 2022
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 12, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (lowRISC#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes lowRISC#12447.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 12, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (lowRISC#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes lowRISC#12447.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 13, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (lowRISC#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes lowRISC#12447.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 13, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (lowRISC#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes lowRISC#12447.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 13, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (lowRISC#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes lowRISC#12447.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit that referenced this issue May 13, 2022
Meson will soon be removed from our project and replaced entirely with
bazel (#12449). This updates the the OTBN `gen-binaries.py` script to
not rely on the presence of the `.env` file that was produced by the
`meson_init.sh` script to provide locations to the RV32 toolchain.

Instead, the `gen-binaries.py` script now uses environment variables to
get the locations of the RV32 toolchain tools, which are populated via
queries to bazel.

This fixes #12447.

Signed-off-by: Timothy Trippel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants