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

[quality] Update repository quality suite #15044

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Conversation

cfrantz
Copy link
Contributor

@cfrantz cfrantz commented Sep 20, 2022

Refactor the "quality" suite for the OpenTitan repository.

TL;DR: bazel test //quality/...

  1. Move all of the quality check/fix targets to //quality.
  2. Refactor the quality check targets into bazel tests (rather than executables). The motivation for this change is both ergonomic and aesthetic. One ought to be able to type:
    bazel test //quality/... [--test_output=streamed]
    
  3. Update various CI scripts that used to bazel run <some_check> to instead bazel test <some_check> --test_output=streamed.
  4. Update the bazel user-guide/cheat sheet to reflect the test invocation and new labels.

This change should not be committed until after lowRISC/misc-linters#30.

@cfrantz
Copy link
Contributor Author

cfrantz commented Sep 21, 2022

Oh, irony: The quality refactor failed the CI's quality stage.

@cfrantz cfrantz force-pushed the quality branch 2 times, most recently from f1d07ce to d6bb7fd Compare September 21, 2022 15:41
1. Move `clang_format_{check,fix}` to the `quality` subdir.
2. Execute `clang_format_check` as a test.

Signed-off-by: Chris Frantz <[email protected]>
@cfrantz cfrantz marked this pull request as ready for review September 21, 2022 18:48
Copy link
Contributor

@drewmacrae drewmacrae 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!

# Directories used exclusively to store build artifacts are still copied into.
"./build-out/**",
"./build-bin/**",
# fusesoc build dir
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the docs also get built in build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the suggestion is here. This was copied directly from the prior incarnation in the root BUILD file.

These excludes are slated for deletion once we get the CI doing everything through bazel.

@cfrantz cfrantz merged commit 7705440 into lowRISC:master Sep 23, 2022
eunchan added a commit to eunchan/opentitan that referenced this pull request Sep 24, 2022
PR lowRISC#15044 did not update bazel airgap environment prep script.

Signed-off-by: Eli Kim <[email protected]>
eunchan added a commit to eunchan/opentitan that referenced this pull request Sep 24, 2022
PR lowRISC#15044 did not update bazel airgap environment prep script.

Signed-off-by: Eli Kim <[email protected]>
timothytrippel pushed a commit that referenced this pull request Sep 24, 2022
PR #15044 did not update bazel airgap environment prep script.

Signed-off-by: Eli Kim <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Sep 26, 2022
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Sep 26, 2022
This updates the way the go toolchain is downloaded/included in our
Bazel workspace to enable Bazel builds in an airgapped environment.
Specifically, lowRISC#15044 exposed a bug in our go toolchain dependency that
caused Bazel builds to fail in the airgapped environment.

The fix for this was two fold:

1. manually specify the exact toolchain tarballs to be downloaded
   using the `go_download_sdk` repo rule, instead of the
   `go_register_toolchain` rule which attempts to automatically find
   the right toolchain to download by downloading a file from the
   Internet that is not cacheable, and therefore not accessible in an
   airgapped environment, and
2. fork and patch `rules_go` to only download go toolchains using the
   `ctx.download_and_extract` action (instead of the `ctx.download`
   action) which caches said downloads in the repository cache that is
   moved to the airgapped environment. See
   lowRISC/rules_go@07fe5f4
   for more details.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Sep 26, 2022
This updates the way the go toolchain is downloaded/included in our
Bazel workspace to enable Bazel builds in an airgapped environment.
Specifically, lowRISC#15044 exposed a bug in our go toolchain dependency that
caused Bazel builds to fail in the airgapped environment.

The fix for this was two fold:

1. manually specify the exact toolchain tarballs to be downloaded
   using the `go_download_sdk` repo rule, instead of the
   `go_register_toolchain` rule which attempts to automatically find
   the right toolchain to download by downloading a file from the
   Internet that is not cacheable, and therefore not accessible in an
   airgapped environment, and
2. fork and patch `rules_go` to only download go toolchains using the
   `ctx.download_and_extract` action (instead of the `ctx.download`
   action) which caches said downloads in the repository cache that is
   moved to the airgapped environment. See
   lowRISC/rules_go@07fe5f4
   for more details.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Sep 26, 2022
This updates the way the go toolchain is downloaded/included in our
Bazel workspace to enable Bazel builds in an airgapped environment.
Specifically, lowRISC#15044 exposed a bug in our go toolchain dependency that
caused Bazel builds to fail in the airgapped environment.

The fix for this was two fold:

1. manually specify the exact toolchain tarballs to be downloaded
   using the `go_download_sdk` repo rule, instead of the
   `go_register_toolchain` rule which attempts to automatically find
   the right toolchain to download by downloading a file from the
   Internet that is not cacheable, and therefore not accessible in an
   airgapped environment, and
2. fork and patch `rules_go` to only download go toolchains using the
   `ctx.download_and_extract` action (instead of the `ctx.download`
   action) which caches said downloads in the repository cache that is
   moved to the airgapped environment. See
   lowRISC/rules_go@07fe5f4
   for more details.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit to timothytrippel/opentitan that referenced this pull request Sep 27, 2022
This updates the way the go toolchain is downloaded/included in our
Bazel workspace to enable Bazel builds in an airgapped environment.
Specifically, lowRISC#15044 exposed a bug in our go toolchain dependency that
caused Bazel builds to fail in the airgapped environment.

The fix for this was to manually specify the exact toolchain tarballs
to be downloaded using the `go_download_sdk` repo rule, instead of the
`go_register_toolchain` rule which attempts to automatically find the
right toolchain to download by downloading a file from the Internet that
is not cacheable, since no sha256 is provided to the `ctx.download`
action, and therefore not accessible in an airgapped environment.

Signed-off-by: Timothy Trippel <[email protected]>
timothytrippel added a commit that referenced this pull request Sep 27, 2022
This updates the way the go toolchain is downloaded/included in our
Bazel workspace to enable Bazel builds in an airgapped environment.
Specifically, #15044 exposed a bug in our go toolchain dependency that
caused Bazel builds to fail in the airgapped environment.

The fix for this was to manually specify the exact toolchain tarballs
to be downloaded using the `go_download_sdk` repo rule, instead of the
`go_register_toolchain` rule which attempts to automatically find the
right toolchain to download by downloading a file from the Internet that
is not cacheable, since no sha256 is provided to the `ctx.download`
action, and therefore not accessible in an airgapped environment.

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants