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] Update Bazel to version 7.3.1 #24392

Merged
merged 8 commits into from
Sep 17, 2024
Merged

[bazel] Update Bazel to version 7.3.1 #24392

merged 8 commits into from
Sep 17, 2024

Conversation

jwnrt
Copy link
Contributor

@jwnrt jwnrt commented Aug 23, 2024

This is a major version upgrade from Bazel 6.1.1 to Bazel 7.3.1. This is the latest version.

This PR includes a couple of fixes required due to changes between versions:

  1. The airgapped build no longer clones the Bazel Git repository to access its dependencies, it now uses the method described here in the Bazel documentation: https://bazel.build/run/build#repository_cache_with_bazel_7_or_later.
  2. The fusesoc_build rule no longer outputs the whole of fusesoc's $X.build/ directory, it now only exposes the parts explicitly listed in output_groups. This is required because Bazel 7 does not allow rules to output both a directory and its contents at once.

This PR is motivated in part because some of our dependencies (e.g. rules_rust) no longer support version 6. Migrating to Bzlmod will also require Bazel 7 since version 6 is missing a couple of features we need.

Potential for breakage

CI should pick up any breakages, but we should also check that contributors using non-Ubuntu distributions can still use this new Bazel version.

Contributors using ./bazelisk.sh should not need to manually install the new Bazel version, it should be installed automatically. Those using a native Bazel will have to update, but Bazel will read the .bazelversion file and tell them how to do this.

@jwnrt jwnrt requested review from pamaury and a-will August 23, 2024 09:46
@jwnrt jwnrt marked this pull request as ready for review August 23, 2024 09:46
@jwnrt jwnrt requested a review from cfrantz as a code owner August 23, 2024 09:46
@jwnrt jwnrt marked this pull request as draft August 23, 2024 09:53
@jwnrt
Copy link
Contributor Author

jwnrt commented Aug 23, 2024

I was too hasty in marking as ready for review. The strange license_checker breakage observed in #24216 has appeared here. I'll try work out what's going on.

@jwnrt jwnrt linked an issue Aug 23, 2024 that may be closed by this pull request
rules/fusesoc.bzl Outdated Show resolved Hide resolved
rules/fusesoc.bzl Show resolved Hide resolved
@jwnrt jwnrt force-pushed the bazel-7 branch 2 times, most recently from 4af21d0 to 9d74cd7 Compare August 24, 2024 10:16
@jwnrt
Copy link
Contributor Author

jwnrt commented Aug 24, 2024

The sw_build job in CI is finding a bunch of problems with our C code now. Not sure what changed in Bazel 7 to make these identified or why only sw_build is finding them. They don't appear when I bazel build the targets separately.

Some examples of new warnings (which it treats as errors):

  1. Missing newlines at the end of files (e.g. the generated hw/ip/uart/data/uart_regs.h.
  2. Semicolons repeated inside and outside a macro (e.g. with sw/device/lib/arch/device_fpga_cw305.c using CALCULATE_UART_NCO).
  3. Enum values being out of range for int. We do this a lot. Usually it's for u32s.
    • Apparently ISO C requires they fit into int but GCC promises to using u32 if not signed. Not sure what Clang promises.
    • Not sure how to fix this. We use enums everywhere for constants. Do we change the style guide? Ignore this warning?

@a-will
Copy link
Contributor

a-will commented Aug 25, 2024

The sw_build job in CI is finding a bunch of problems with our C code now. Not sure what changed in Bazel 7 to make these identified or why only sw_build is finding them. They don't appear when I bazel build the targets separately.

Wrong target, probably (or more accurately, the wrong configuration). This target triggers errors, for example:

./bazelisk.sh build //sw/device/silicon_creator/manuf/base:ft_provision_fpga_cw340_rom_with_fake_keys

Some examples of new warnings (which it treats as errors):

  1. Missing newlines at the end of files (e.g. the generated hw/ip/uart/data/uart_regs.h.

  2. Semicolons repeated inside and outside a macro (e.g. with sw/device/lib/arch/device_fpga_cw305.c using CALCULATE_UART_NCO).

  3. Enum values being out of range for int. We do this a lot. Usually it's for u32s.

    • Apparently ISO C requires they fit into int but GCC promises to using u32 if not signed. Not sure what Clang promises.
    • Not sure how to fix this. We use enums everywhere for constants. Do we change the style guide? Ignore this warning?

If you compare the subcommands, you'll notice that -Wpedantic got added. If we're going to use this style for enums, we really should either update to C23 or use one of the GNU --std modes.

Edit: Aww, bummer, I thought C23 was finalized already. In that case, for an ISO standard, we still only get sized enums in c++ compilation modes.

@jwnrt
Copy link
Contributor Author

jwnrt commented Aug 26, 2024

@pamaury and I found the cause of the pedantic errors: Bazel has changed --features to only apply to the exec target and not host. We set --features=-pedantic_warnings in .bazelrc, but bindgen builds our software with host so this wasn't being applied. See bazelbuild/bazel#13839.

Added a commit which adds --host_features flags to `.bazelrc.

We also found that the rules_rust we're currently using does not seem to properly support Bazel 7. bazel query 'deps(//some/rust/target)' was giving an unusual error:

$ bazel query 'deps(//sw/device/silicon_creator/manuf/base:ft_provision_fpga_cw340_rom_with_fake_keys)'

ERROR: Evaluation of query "deps(//sw/device/silicon_creator/manuf/base:ft_provision_fpga_cw340_rom_with_fake_keys)" failed: preloading transitive closure failed: no such package '@@rules_rust_util_import__syn-1.0.82//': The repository '@@rules_rust_util_import__syn-1.0.82' could not be resolved: Repository '@@rules_rust_util_import__syn-1.0.82' is not defined

To fix this, I have had to bundle the rules_rust upgrade into this PR which increases its size but hopefully we can still get this reviewed.

We cannot upgrade rules_rust first in a separate PR because new versions do not support Bazel 6. It's possible we could find a version that supports 6 and 7 which I will investigate if this PR is too large to review / merge.

@jwnrt jwnrt changed the title [bazel] Update Bazel to version 7.3.1 [bazel] Update Bazel to version 7.3.1, rules_rust to 0.47.1 Aug 26, 2024
@jwnrt jwnrt changed the title [bazel] Update Bazel to version 7.3.1, rules_rust to 0.47.1 [bazel] Update Bazel to version 7.3.1, rules_rust to 0.49.3 Aug 26, 2024
@jwnrt jwnrt changed the title [bazel] Update Bazel to version 7.3.1, rules_rust to 0.49.3 [bazel] Update Bazel to version 7.3.1 Aug 26, 2024
@jwnrt
Copy link
Contributor Author

jwnrt commented Aug 26, 2024

Sorry for the spam, we found a simpler way to fix the Rust issue without upgrading the toolchain just yet.

@jwnrt
Copy link
Contributor Author

jwnrt commented Aug 26, 2024

Hmm, using symlinks doesn't seem to work because ctx.actions.symlink does not have an inputs parameter so Bazel just creates the symlink without building the thing it's linking to.

I guess target_file is supposed to be like inputs, but as previously mentioned we can't create a file to the targets because they're within a directory that is being output.

I can't think of a way to make this work, so reverting back to the previous implementation.

@jwnrt jwnrt force-pushed the bazel-7 branch 3 times, most recently from ff72e09 to 785543f Compare August 27, 2024 14:12
@jwnrt jwnrt marked this pull request as ready for review August 27, 2024 14:12
hw/bitstream/vivado/BUILD Outdated Show resolved Hide resolved
Signed-off-by: James Wainwright <[email protected]>
We were previously taking these from the Bazel repository at the version
we were using. This has broken, but Bazel has a documented way of doing
this now independent of version:

https://bazel.build/run/build#repository_cache_with_bazel_7_or_later

Signed-off-by: James Wainwright <[email protected]>
Bazel 7 does not like a rule which outputs a directory _and_ something
from that directory's contents.

This commit removes the build directory from the outputs and exposes
only those requested by `output_groups`. The paths should stay the same.

Signed-off-by: James Wainwright <[email protected]>
This release includes a fix for Bazel 7 that we need.

Signed-off-by: James Wainwright <[email protected]>
When we build our C files for `bindgen` to consume, we use the `host`
configuration. The `--features` flag is no longer applied to `host` in
Bazel 7, so we must add this extra flag.

Signed-off-by: James Wainwright <[email protected]>
Even though we're not using these, Bazel 7 hits errors if we try to run
`bazel query deps($something)` on a Rust dependency.

Fixed in `rules_rust>=0.38.0`.

bazelbuild/rules_rust#1166 (comment)

Signed-off-by: James Wainwright <[email protected]>
Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

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

That's great, thanks a lot @jwnrt for this work!

@jwnrt jwnrt merged commit 4b26cb4 into lowRISC:master Sep 17, 2024
36 checks passed
@jwnrt jwnrt deleted the bazel-7 branch September 17, 2024 12:05
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.

[bazel] Update to Bazel 7
4 participants