Add support for `layering_check` #246

fmeum · 2024-01-14T19:55:13Z

The general support for this feature is inherited from the Bazel-provided Unix C++ toolchain, except that a module map for the toolchain and sysroot headers has to be supplied to the cc_toolchain.

This requires updates of rules_go, abseil-cpp and openssl BUILD file changes to become compatible with the stricter checks. Also fixes CI issues caused by the release of Bazel 7.0.0.

Fixes #203

fmeum · 2024-01-14T19:56:02Z

@jsharpe I saw that you were interested in this feature, any kind of testing would be highly appreciated.

The general support for this feature is inherited from the Bazel-provided Unix C++ toolchain, except that a module map for the toolchain and sysroot headers has to be supplied to the `cc_toolchain`. Requires an update of abseil-cpp to make it compatible with `layering_check`.

jsharpe · 2024-01-16T09:10:50Z

@jsharpe I saw that you were interested in this feature, any kind of testing would be highly appreciated.

Yep; I'll try this in the next few days hopefully!

`/tmp` is hermetic by default in Bazel 7.

fmeum · 2024-01-17T20:26:16Z

@jsharpe The remaining failure appears to be unrelated to the changes in this PR. Have you tested rules_foreign_cc with Bazel 7 and its flip of --incompatible_sandbox_hermetic_tmp yet? It looks like some symlinks dangle with it.

Edit: I looked into this. With --incompatible_sandbox_hermetic_tmp, source files are mounted under a path in the sandbox that doesn't exist outside the sandbox. Since cmake install seems to preserve symlinks if it finds any, the INSTALLDIR will contain these symlinks that are dangling after the action succeeded. Ideally, absolute symlinks would be followed while relative symlinks (e.g. to implement versioned shared libs) would not.

fmeum · 2024-01-17T21:33:11Z

@siddharthab After quite a bit of trial and error, this is ready for review now. Let me know if you want me to move the unrelated CI fixes required due to the Bazel 7 release out into a separate PR.

siddharthab

Overall looks good. Let me know if you have the bandwidth to do the changes I have suggested. If not, maybe I can push to your branch. I am going to give some time to the repo this weekend.

Also, thank you so much for working through the nits from Bazel 7 upgrade, and maybe other causes.

README.md

tests/MODULE.bazel

toolchain/cc_toolchain_config.bzl

toolchain/internal/system_module_map.bzl

toolchain/internal/generate_system_module_map.sh

siddharthab · 2024-01-18T00:53:13Z

toolchain/internal/generate_system_module_map.sh

+    /*) ;;
+    *) header=${EXECROOT_PREFIX}"${header}" ;;
+    esac
+    echo "  textual header \"${header}\""


Did you really want to echo this line under all cases, i.e. even under the /* case?

Yes, added a comment.

toolchain/internal/system_module_map.bzl

siddharthab · 2024-01-18T00:57:39Z

toolchain/internal/system_module_map.bzl

+    doc = """Generates a Clang module map for the toolchain and sysroot headers.""",
+    implementation = _system_module_map,
+    attrs = {
+        "toolchain": attr.label(mandatory = True),


It seems to me that it will be better if we renamed this cxx_builtin_include_filegroup or something like that, instead of calling it toolchain. It makes the code easier to read.

Likewise, we should create a separate filegroup target to pass here instead of all-files.

I renamed the attribute (I chose a files suffix as it wouldn't strictly have to be a filegroup) and introduced a separate filegroup.

toolchain/internal/generate_system_module_map.sh

toolchain/cc_toolchain_config.bzl

Co-authored-by: Siddhartha Bagaria <[email protected]>

toolchain/internal/generate_system_module_map.sh

fmeum · 2024-01-18T19:45:05Z

toolchain/internal/generate_system_module_map.sh

+    /*) ;;
+    *) header=${EXECROOT_PREFIX}"${header}" ;;
+    esac
+    echo "  textual header \"${header}\""


Yes, added a comment.

fmeum · 2024-01-18T19:49:41Z

toolchain/internal/system_module_map.bzl

+    doc = """Generates a Clang module map for the toolchain and sysroot headers.""",
+    implementation = _system_module_map,
+    attrs = {
+        "toolchain": attr.label(mandatory = True),


I renamed the attribute (I chose a files suffix as it wouldn't strictly have to be a filegroup) and introduced a separate filegroup.

fmeum · 2024-01-18T20:10:57Z

toolchain/internal/configure.bzl

+system_module_map(
+    name = "module-{suffix}",
+    cxx_builtin_include_files = ":include-components-{suffix}",
+    cxx_builtin_include_directories = cxx_builtin_include_directories,


@siddharthab Nvm, this is problematic: The builtin include directories are computed in the cc_toolchain_config rule and not available here. I could either extract this into a separate function and pass the result into cc_toolchain_config or have cc_toolchain_config return the information via a provider. That's why I went for the macro in the first place. Maybe you see a better way to pass around this information.

Yes, I was thinking that we would compute it outside of the macro (i.e. the BUILD file should already have that info) and pass it in.

fmeum · 2024-01-19T21:34:41Z

I tried to use your pipefail suggestion, but that just caused the script to fail.

siddharthab · 2024-01-19T22:30:54Z

Thank you for all the hard work. This looks very good. I might do some very minor cleanup for nitpicks later. Let's merge this!

fmeum force-pushed the layering_check_impl branch 7 times, most recently from 8ce1411 to b1a44e4 Compare January 15, 2024 13:57

fmeum marked this pull request as draft January 15, 2024 13:59

fmeum force-pushed the layering_check_impl branch from b1a44e4 to 200fe6d Compare January 15, 2024 14:22

fmeum mentioned this pull request Jan 15, 2024

Disable layering_check feature in CGo tests bazel-contrib/rules_go#3829

Merged

fmeum force-pushed the layering_check_impl branch 4 times, most recently from 6e9188c to 455bf61 Compare January 16, 2024 07:20

fmeum added 5 commits January 16, 2024 08:22

Fix CI

a17b0cc

TMP: Debug failure

1ecbfbc

Use newer rules_go

c25bc41

Reenable Bzlmod

4c12655

fmeum force-pushed the layering_check_impl branch from 455bf61 to c09b634 Compare January 16, 2024 07:41

Update abseil-cpp further

212a84c

fmeum force-pushed the layering_check_impl branch from c09b634 to 212a84c Compare January 16, 2024 07:51

Fix CI

ce87a8b

fmeum marked this pull request as ready for review January 16, 2024 09:43

fmeum force-pushed the layering_check_impl branch from 3bbfb69 to ce87a8b Compare January 16, 2024 20:49

Fix hermeticity issue in system_module_map

63680cc

fmeum force-pushed the layering_check_impl branch from 44e689f to 63680cc Compare January 17, 2024 19:45

Store system LLVM toolchain in /opt

dec9b1e

`/tmp` is hermetic by default in Bazel 7.

Work around rules_foreign_cc issue with Bazel 7

fddc2fd

siddharthab reviewed Jan 18, 2024

View reviewed changes

Apply suggestions from code review

d54f741

Co-authored-by: Siddhartha Bagaria <[email protected]>

fmeum commented Jan 18, 2024

View reviewed changes

Apply more review comments

bb38ddf

fmeum force-pushed the layering_check_impl branch from a9e88cf to bb38ddf Compare January 18, 2024 20:10

fmeum commented Jan 18, 2024

View reviewed changes

Address more comments

92662c5

fmeum force-pushed the layering_check_impl branch from d980a59 to 92662c5 Compare January 19, 2024 21:34

fmeum requested a review from siddharthab January 19, 2024 21:34

siddharthab merged commit 05f0bc1 into bazel-contrib:master Jan 19, 2024
24 checks passed

fmeum deleted the layering_check_impl branch January 19, 2024 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `layering_check` #246

Add support for `layering_check` #246

fmeum commented Jan 14, 2024 •

edited

Loading

fmeum commented Jan 14, 2024

jsharpe commented Jan 16, 2024

fmeum commented Jan 17, 2024 •

edited

Loading

fmeum commented Jan 17, 2024

siddharthab left a comment

siddharthab Jan 18, 2024

fmeum Jan 18, 2024

siddharthab Jan 18, 2024

fmeum Jan 18, 2024

fmeum Jan 18, 2024

fmeum Jan 18, 2024

fmeum Jan 18, 2024

siddharthab Jan 18, 2024

fmeum Jan 19, 2024

fmeum commented Jan 19, 2024

siddharthab commented Jan 19, 2024

Add support for layering_check #246

Add support for layering_check #246

Conversation

fmeum commented Jan 14, 2024 • edited Loading

fmeum commented Jan 14, 2024

jsharpe commented Jan 16, 2024

fmeum commented Jan 17, 2024 • edited Loading

fmeum commented Jan 17, 2024

siddharthab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum commented Jan 19, 2024

siddharthab commented Jan 19, 2024

Add support for `layering_check` #246

Add support for `layering_check` #246

fmeum commented Jan 14, 2024 •

edited

Loading

fmeum commented Jan 17, 2024 •

edited

Loading