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] lowRISC/misc-linters does not cache Python wheels #12538

Closed
4 of 5 tasks
timothytrippel opened this issue May 7, 2022 · 4 comments
Closed
4 of 5 tasks

[bazel] lowRISC/misc-linters does not cache Python wheels #12538

timothytrippel opened this issue May 7, 2022 · 4 comments
Assignees
Labels
Priority:P1 Priority: high Priority:P2 Priority: medium SW:Build System

Comments

@timothytrippel
Copy link
Contributor

timothytrippel commented May 7, 2022

The lowRISC/misc-linters repository currently makes use of the default rules_python repository to register a hermetic Python toolchain and packages. Like #12091, the Python wheels in this dependency must be cached so that this (opentitan) repo can be built in an air-gapped environment. To accomplish this:

Addressed in: lowRISC/misc-linters:#28

Addressed in: #12562

  • the commit and sha of the lowRISC/misc-linters dependency (in this Bazel workspace) must be updated to include the above updates

Addressed in: TBD

  • determine what is causing the lint rules to be a dep of the chip-level test (opentitan_functest) targets
@timothytrippel timothytrippel self-assigned this May 7, 2022
@timothytrippel timothytrippel changed the title [bazel] lowRISC/misc-linters uses does not cache Python wheels [bazel] lowRISC/misc-linters does not cache Python wheels May 7, 2022
@rswarbrick
Copy link
Contributor

As a meta-question: do we care? It seems a bit bizarre to be hot on running a licence check linting flow in an air-gapped environment! Can't we just not bother running it?

@timothytrippel
Copy link
Contributor Author

timothytrippel commented May 10, 2022

That's a great question, and one I asked myself before I endeavored down this path, and the answer I still believe is that we shouldn't need to! However, reality set in, and I had to side line my idealist approach in the interim.

Specifically when I run a bazel build --distdir=$BAZEL_DISTDIR --repository_cache=$BAZEL_CACHE //sw/device/tests:aes_smoketest on the air-gapped machine, it hangs, trying to fetch the @lowrisc_linter repository, see below screen shot:

Screen Shot 2022-05-09 at 9 52 07 PM

But this was puzzling to me because the bazel documentation clearly states "By default, external dependencies are fetched as needed during bazel build", and when I run a bazel query "deps(//sw/device/tests:aes_smoketest)" to see if the lint rules are indeed a dep of a chip-level test target, I do not see that they are. See below screenshot:
Screen Shot 2022-05-09 at 9 50 14 PM

So something odd is happening that I need to track down, but until then I did not want to hold up the Bazel transition on something I know how to fix in the iterim. Specifically, I already learned how to cache Python wheels that are downloaded/installed with the pip_install Bazel rule, so I used this same approach here (which works).

I will keep this issue open, and have added a task (see above) to track down this issue to eventually simplify the need for this work around.

timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 10, 2022
This update the lowrisc_lint external repository to accomodate the
latest patches and partially address lowRISC#12538.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this issue May 10, 2022
This update the lowrisc_lint external repository to accomodate the
latest patches and partially address lowRISC#12538.

Signed-off-by: Timothy Trippel <[email protected]>
@timothytrippel timothytrippel added the Priority:P2 Priority: medium label May 10, 2022
timothytrippel added a commit that referenced this issue May 10, 2022
This update the lowrisc_lint external repository to accomodate the
latest patches and partially address #12538.

Signed-off-by: Timothy Trippel <[email protected]>
@rswarbrick
Copy link
Contributor

Ah, I see. I guess I was asking something even stupider: why are we running the licence checker in Bazel, which I thought was supposed to be for building our software? It's not clear to me that compiling a bunch of C and assembly code requires a linting script that looks at the first few lines of each file...

@timothytrippel
Copy link
Contributor Author

It is for two reasons: 1) provide developers with a simple bazel run quality-like command that allows them to run all necessary linter scripts/programs on all altered files in the repo locally (before submitting a PR), and 2) to enable CI to eventually (one day) consist of three simple steps (obviously simplifying here):
a) bazel run quality
b) bazel build //...
c) bazel test //...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:P1 Priority: high Priority:P2 Priority: medium SW:Build System
Projects
None yet
Development

No branches or pull requests

3 participants