-
Notifications
You must be signed in to change notification settings - Fork 4.4k
[9.0.0] Add support for multiple module interfaces per cc_library
#27927
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
Conversation
When multiple `module_interfaces` are specified on a single `cc_library`, the individual compilation actions form a DAG based on `import`s between these modules. Consider the following situation: * `a.cppm` imports `b.cppm`, both of which are in the `module_interfaces` of a single `cc_library`. * Building the target populates the action cache with an entry for `a.pcm` that stores `b.pcm` as a discovered input. * Now edit `a.cppm` and `b.cppm` so that `b.cppm` imports `a.cppm` and `a.cppm` no longer imports `b.cppm`. * Build again (optionally after a shutdown). Before this commit, this resulted in an action cycle since during action cache checking, Bazel would reuse or look up the inputs discovered in the previous build, thus introducing an edge from `a.pcm` to `b.pcm`. Together with the newly discovered edge from `b.pcm` to `a.pcm`, this resulted in a cycle. This is fixed by not requesting the previously discovered inputs (either retained in memory or in the action cache) if the mandatory inputs changed. In the case of C++20 modules, this is sufficient since the modmap file, which lists all transitive `.pcm` files required for compilation, is a mandatory input. As part of this change, `MetadataDigestUtils.fromMetadata` had to be modified to always return a byte array of proper digest length, even if called with an empty map, to match the assumptions of the action cache. This change is pretty much Fabian's PR bazelbuild#27492 with a tiny fix added on top (not returning from computeMandatoryInputsDigest() early on valuesMissing() if inErrorBubbling() is true) Closes bazelbuild#27492. PiperOrigin-RevId: 842733471 Change-Id: I48fa2c0bceb888dcb58db29d50c30719b2122c5d (cherry picked from commit cb9bd86)
Summary of ChangesHello @fmeum, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in Bazel's action caching system that could lead to action cycles when working with C++20 modules, particularly when the import relationships between Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust mechanism to handle dependency changes within C++20 modules by tracking a digest of mandatory inputs. This effectively prevents action cycles when module import graphs are modified. The changes are well-implemented across the action caching and execution logic. I've identified a minor typo in a new test case and an opportunity to improve code clarity with a comment. Overall, this is a solid improvement for incremental builds involving C++ modules.
| if (mandatoryInputsDigest.length != digest.length) { | ||
| throw new IOException("Corrupted mandatory inputs digest"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is correct because both mandatoryInputsDigest and the action digest are generated using the same hash function, resulting in digests of the same length. However, this is not immediately obvious from the code. Adding a comment to explain this assumption would improve clarity for future maintainers.
if (mandatoryInputsDigest.length != digest.length) {
// Both digests are generated using the same hash function, so they must have the same
// length. A mismatch indicates corruption.
throw new IOException("Corrupted mandatory inputs digest");
}This is now supported in Bazel (HEAD and 9.0.0) since bazelbuild/bazel#27927.
Head branch was pushed to by a user without write access
|
@fmeum Could you please resolve the conflicts? |
|
@iancha1992 Should be fixed |
251bfef
This is now supported in Bazel (HEAD and 9.0.0) since bazelbuild/bazel#27927. Closes #545 COPYBARA_INTEGRATE_REVIEW=#545 from fmeum:patch-1 b4ec47b PiperOrigin-RevId: 844562596 Change-Id: If41dc44ddaaba7d3cf940b5fa126b29bd2e461b4
…brary` (bazelbuild#27927)" This reverts commit 251bfef.
When multiple
module_interfacesare specified on a singlecc_library, the individual compilation actions form a DAG based onimports between these modules. Consider the following situation:a.cppmimportsb.cppm, both of which are in themodule_interfacesof a singlecc_library.a.pcmthat storesb.pcmas a discovered input.a.cppmandb.cppmso thatb.cppmimportsa.cppmanda.cppmno longer importsb.cppm.Before this commit, this resulted in an action cycle since during action cache checking, Bazel would reuse or look up the inputs discovered in the previous build, thus introducing an edge from
a.pcmtob.pcm. Together with the newly discovered edge fromb.pcmtoa.pcm, this resulted in a cycle.This is fixed by not requesting the previously discovered inputs (either retained in memory or in the action cache) if the mandatory inputs changed. In the case of C++20 modules, this is sufficient since the modmap file, which lists all transitive
.pcmfiles required for compilation, is a mandatory input.As part of this change,
MetadataDigestUtils.fromMetadatahad to be modified to always return a byte array of proper digest length, even if called with an empty map, to match the assumptions of the action cache.This change is pretty much Fabian's PR #27492 with a tiny fix added on top (not returning from computeMandatoryInputsDigest() early on valuesMissing() if inErrorBubbling() is true)
Closes #27492.
PiperOrigin-RevId: 842733471
Change-Id: I48fa2c0bceb888dcb58db29d50c30719b2122c5d
(cherry picked from commit cb9bd86)
Closes #27544