[1/5] support C++20 Modules, add module_interfaces attr #22425

PikachuHyA · 2024-05-17T06:50:55Z

I split the XXL PR #19940 into several small patches.
This is the first patch of Support C++20 Modules, I add module_interfaces attr only

example

foo.cppm

// foo.cppm
export module foo;
// ...

BUILD.bazel

cc_library(
    name="foo",
    copts=["-std=c++20"],
    module_interfaces=["foo.cppm"],
    # features=["cpp20_module"]
)

build failed with the following message

➜  bazel build :foo
ERROR: bazel_demo/BUILD.bazel:1:11: in cc_library rule //:foo: 
Traceback (most recent call last):
        File "/virtual_builtins_bzl/common/cc/cc_library.bzl", line 40, column 42, in _cc_library_impl
        File "/virtual_builtins_bzl/common/cc/semantics.bzl", line 123, column 13, in _check_can_module_interfaces
Error in fail: attribute module_interfaces: requires --experimental_cpp20_modules
ERROR: bazel_demo/BUILD.bazel:1:11: Analysis of target '//:foo' failed
ERROR: Analysis of target '//:foo' failed; build aborted
INFO: Elapsed time: 0.106s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully

To build with C++20 Modules, the flag --experimental_cpp20_modules must be added.

➜  bazel build :foo --experimental_cpp20_modules
ERROR: bazel_demo/BUILD.bazel:1:11: in cc_library rule //:foo: 
Traceback (most recent call last):
        File "/virtual_builtins_bzl/common/cc/cc_library.bzl", line 41, column 34, in _cc_library_impl
        File "/virtual_builtins_bzl/common/cc/cc_helper.bzl", line 1225, column 13, in _check_cpp20_modules
Error in fail: to use C++20 Modules, the feature cpp20_modules must be enabled
ERROR: bazel_demo/BUILD.bazel:1:11: Analysis of target '//:foo' failed
ERROR: Analysis of target '//:foo' failed; build aborted
INFO: Elapsed time: 0.091s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
ERROR: Build did NOT complete successfully

To build with C++20 Modules, the feature cpp20_modules must be enabled.

bazel build :foo --experimental_cpp20_modules --features cpp20_modules

the flag --experimental_cpp20_modules works on global and
the feature cpp20_modules work on each target

but in this patch, do nothing with C++20 Module Interfaces.

src/main/starlark/builtins_bzl/common/cc/attrs.bzl

src/main/starlark/builtins_bzl/common/cc/cc_common.bzl

src/main/java/com/google/devtools/build/lib/rules/cpp/CppActionConfigs.java

src/main/java/com/google/devtools/build/lib/rules/cpp/CppRuleClasses.java

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java

src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java

comius

Can you add tests in the same PR?

verify the experimental flag works as expected
verify cc_binary, cc_test and cc_library work with the new attribute (although do nothing yet)
verify calling cc_common.compile works with the new parameter

PikachuHyA · 2024-05-27T03:42:55Z

You might want to consider a way to tell Bazel that some sources do not use modules and can therefore completely skip scanning (and, if nothing in the target needs scanned, the target's collation step as well).

hi @mathstuf, In current implementation, I'm using the cpp20_modules feature to control if a target should be scanned. This only works for the whole target, not for individual files.

With this feature, we can update our codebase bit by bit, turning on C++20 Modules for some code without having to scan every file.

PikachuHyA · 2024-05-28T09:26:51Z

hi @comius, I have updated the code. please review again.
for each comment, I give a small commit. Squash commits can be performed if needed.

I test the failed case //src/test/java/com/google/devtools/build/lib/dynamic:DynamicSpawnStrategyTest on GitHub Action (job link) with branch cxx20-modules-support-patch-1-ci. and the test passed.

the new branch only add ci config. There could be differing settings that prevent the replication of the CI error.
about the window CI test broken, could you give me some suggestions?

comius · 2024-05-31T10:53:05Z

src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java

+        OptionEffectTag.CHANGES_INPUTS
+      },
+      metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+      help = "If enabled, cc_binary and cc_library targets can use attribute `module_interfaces`.")


Put under the help string:

Enables experimental C++20 modules support. Use it with `module_interfaces` attribute on `cc_binary` and `cc_library`. While the support is behind the experimental flag, there are no guarantees about incompatible changes to it or even keeping the support in the future. Consider those risks when using it.

src/main/starlark/builtins_bzl/common/cc/attrs.bzl

comius · 2024-05-31T10:59:19Z

src/main/starlark/builtins_bzl/common/cc/semantics.bzl

@@ -117,6 +117,11 @@ def _check_can_use_implementation_deps(ctx):
    if (not experimental_cc_implementation_deps and ctx.attr.implementation_deps):
        fail("requires --experimental_cc_implementation_deps", attr = "implementation_deps")

+def _check_can_module_interfaces(ctx):


Move this code into cc_helper.check_cpp20_modules. There's no need to have it in 2 different locations.

(Also semantics.bzl is a bit special, because we have a different internal version of it. There's no need to make this difference here).

comius · 2024-05-31T11:02:23Z

src/main/starlark/builtins_bzl/common/cc/attrs.bzl

+<li>MSVC use ixx </li>
+</ul>
+</p>
+<p>For now usage is guarded by the flag


nit: For now usage -> The use

Technical documentation should avoid using for now or other time constructs. Especially at Google :)

comius · 2024-05-31T11:08:32Z

src/main/starlark/builtins_bzl/common/cc/cc_helper.bzl

-
-    # "srcs" attribute is a LABEL_LIST in cc_rules, which might also contain files.
+# "srcs" attribute is a LABEL_LIST in cc_rules, which might also contain files.
+def _calculate_artifact_label_map(srcs, attr):


nit: rename srcs -> attr_list, attr -> attr_name, src -> attr. (The code is not used only for srcs attribute anymore)

change the comment over the function into a doc string:

""" Converts a label_list attribute into a list of (Artifact, Label) tuples. Each tuple represents an input source file and the label of the rule that generates it (or the label of the source file itself if it is an input file. """

Remove other (duplicated) comments above _get_srcs and _get_cpp20module_interfaces.

comius · 2024-05-31T11:10:09Z

src/test/java/com/google/devtools/build/lib/rules/cpp/Cpp20ModulesConfiguredTargetTest.java

+  @Override
+  protected ConfiguredRuleClassProvider createRuleClassProvider() {
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
+    TestRuleClassProvider.addStandardRules(builder);
+    builder.addConfigurationFragment(DummyTestFragment.class);
+    return builder.addRuleDefinition(new TestRuleClassProvider.MakeVariableTesterRule()).build();
+  }


I don't think you need this block. Please remove it.

comius · 2024-05-31T11:14:21Z

src/test/java/com/google/devtools/build/lib/rules/cpp/Cpp20ModulesConfiguredTargetTest.java

+    ImmutableList<String> targetList = ImmutableList.of("//foo:lib", "//foo:bin", "//foo:test");
+    scratch.file(
+        "foo/BUILD",
+        """


I'd propose to move this scratch.file into a setup method, i.e.

@Before public void setupBasicRulesWithModules() { scratch.file(....); }

DRY is usually an antipattern for the tests, but in this case I'd recommend it.

supernit: fix weird indentation, make cc_library line up with opening """.

comius · 2024-05-31T11:18:04Z

src/test/java/com/google/devtools/build/lib/rules/cpp/Cpp20ModulesConfiguredTargetTest.java

+                    module_interfaces = ["foo.cppm"],
+                )
+                """);
+    for(String targetName: targetList) {


Unroll the loop, i.e copy-paste each of the assert without using a for. Same for the other two tests.

This is where you don't want to follow DRY in the test. Without a loop when an assert fails, the line number will tell you where it failed. (With a for loop you can't tell which targetName failed)

comius · 2024-05-31T11:22:53Z

src/test/java/com/google/devtools/build/lib/rules/cpp/Cpp20ModulesConfiguredTargetTest.java

+                """);
+    for(String targetName: targetList) {
+      AssertionError e = assertThrows(AssertionError.class, () -> getConfiguredTarget(targetName));
+      assertThat(e).hasMessageThat().contains("requires --experimental_cpp20_modules");


I think that this assertion always passes - due to a weird unit test setup we have. Have you tried changing the message, does it fail?

The pattern to use is https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java;l=111-113;drc=03ac7ae4e2c7743ab797c8179d428235c1a9d052

reporter.removeHandler(failFastHandler); getConfiguredTarget("//a:a"); assertContainsEvent("..."); // or assertNoEvents();

mathstuf · 2024-05-31T11:32:53Z

In current implementation, I'm using the cpp20_modules feature to control if a target should be scanned. This only works for the whole target, not for individual files.

That's probably fine given what I know of Bazel (that per-source behaviors are discouraged).

PikachuHyA requested a review from lberki as a code owner May 17, 2024 06:50

github-actions bot added team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels May 17, 2024

comius requested review from comius and removed request for lberki May 22, 2024 12:13

comius self-assigned this May 22, 2024

comius reviewed May 22, 2024

View reviewed changes

src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java Outdated Show resolved Hide resolved

comius reviewed May 22, 2024

View reviewed changes

PikachuHyA mentioned this pull request May 23, 2024

support C++20 Modules #19940

Open

PikachuHyA changed the title ~~support C++20 Modules, add module_interfaces attr~~ [WIP] support C++20 Modules, add module_interfaces attr May 24, 2024

PikachuHyA requested review from a team and oquenchil as code owners May 27, 2024 03:16

PikachuHyA requested review from aranguyen and removed request for a team May 27, 2024 03:16

PikachuHyA added 13 commits May 27, 2024 11:58

support C++20 Modules, add module_interfaces attr

cac4929

remove tag DIRECT_COMPILE_TIME_INPUT

ba28f7c

set module_interfaces default value to _UNBOUND and check it

829c581

remove feature cpp20_module in getLegacyFeatures

65c8973

add JavaDoc for cpp20_module features

c92a23e

use plural, cpp20_modules

a849792

move moduleInterfacesUnchecked next to separateModuleHeadersObject

354a993

rename get_module_interfaces to get_cpp20module_interfaces

dabccb4

add cpp20_modules feature to toolchains

d997c6e

introduce a new flag --experimental_cpp20_modules

3beb4c8

add test

f82951d

add note to module_interfaces attr

b729f86

remove duplicate check on module_interfaces

4cc08d9

PikachuHyA force-pushed the cxx20-modules-support-patch-1 branch from f512dae to 4cc08d9 Compare May 27, 2024 03:58

oquenchil removed their request for review May 27, 2024 07:52

PikachuHyA changed the title ~~[WIP] support C++20 Modules, add module_interfaces attr~~ [WIP] [1/5] support C++20 Modules, add module_interfaces attr May 27, 2024

PikachuHyA changed the title ~~[WIP] [1/5] support C++20 Modules, add module_interfaces attr~~ [1/5] support C++20 Modules, add module_interfaces attr May 28, 2024

sgowroji added awaiting-user-response Awaiting a response from the author awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-review PR is awaiting review from an assigned reviewer awaiting-user-response Awaiting a response from the author labels May 29, 2024

comius reviewed May 31, 2024

View reviewed changes

comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/5] support C++20 Modules, add module_interfaces attr #22425

[1/5] support C++20 Modules, add module_interfaces attr #22425

PikachuHyA commented May 17, 2024 •

edited

comius left a comment

PikachuHyA commented May 27, 2024

PikachuHyA commented May 28, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

comius May 31, 2024

mathstuf commented May 31, 2024

[1/5] support C++20 Modules, add module_interfaces attr #22425

Are you sure you want to change the base?

[1/5] support C++20 Modules, add module_interfaces attr #22425

Conversation

PikachuHyA commented May 17, 2024 • edited

comius left a comment

Choose a reason for hiding this comment

PikachuHyA commented May 27, 2024

PikachuHyA commented May 28, 2024

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

mathstuf commented May 31, 2024

PikachuHyA commented May 17, 2024 •

edited