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

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

Closed

Conversation

PikachuHyA
Copy link
Contributor

@PikachuHyA PikachuHyA commented May 17, 2024

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.

@PikachuHyA PikachuHyA requested a review from lberki as a code owner May 17, 2024 06:50
@github-actions 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 comius requested review from comius and removed request for lberki May 22, 2024 12:13
@comius comius self-assigned this May 22, 2024
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

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 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 PikachuHyA requested review from a team and oquenchil as code owners May 27, 2024 03:16
@PikachuHyA PikachuHyA requested review from aranguyen and removed request for a team May 27, 2024 03:16
@PikachuHyA
Copy link
Contributor Author

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 PikachuHyA force-pushed the cxx20-modules-support-patch-1 branch from f512dae to 4cc08d9 Compare May 27, 2024 03:58
@comius 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
@mathstuf
Copy link

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
Copy link
Contributor Author

hi @comius , thanks for your comments.
I have updated the code. please review again.

Changes Summary

  • update doc about --experimental_cpp20_modules option and module_interfaces attribute
  • refactor _get_cpp20module_interfaces
  • refactor Cpp20ModulesConfiguredTargetTest
  • add testSameModuleInterfacesFileTwice inspired by testSameCcFileTwice

@comius comius requested a review from trybka June 3, 2024 12:26
@comius comius added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Jun 11, 2024
@comius
Copy link
Contributor

comius commented Jun 14, 2024

The code was approved by @trybka internally, suggesting to change the name from cpp20_modules to cpp_modules in the code, except in the documentation/some comments. I applied the changes, to move things along a bit faster. The PR will be merged now.

@PikachuHyA
Copy link
Contributor Author

change the name from cpp20_modules to cpp_modules in the code

Do I need to make this change in this patch?

@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 14, 2024
@peakschris
Copy link

@PikachuHyA thank you so much for this work, I am looking forward to using C++20 modules in our bazel builds :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants