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

Add basic C++ path mapping support #22445

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 18, 2024

Basic support for path mapping for CppCompile actions is added by wiring up PathMapper with:

  • structured path variables for files and include paths (Artifact and NestedSet<PathFragment>)
  • inputs/outputs via Spawn#getPathMapper()
  • header discovery

Also turns external_include_paths into a structured variable, which was missed in #22463.

The following features are known to be unsupported for now:

  • layering_check (requires rewriting paths to and in module maps)
  • source tree artifacts (requires wiring up PathMapper in CppCompileActionTemplate)
  • location expanded paths to generated files such as sanitizer suppression lists (requires heuristically rewriting paths in user_compile_flags)

These limitations will be lifted in follow-up PRs.

Work towards #6526

RELNOTES: Experimental support for path mapping CppCompile actions can be enabled via --modify_execution_info=CppCompile=+supports-path-mapping.

@fmeum fmeum force-pushed the 6526-cc-path-mapping branch 8 times, most recently from 1a21e7d to 417c6d1 Compare May 24, 2024 22:04
@fmeum fmeum force-pushed the 6526-cc-path-mapping branch 3 times, most recently from 46e8993 to 5fed2d0 Compare May 26, 2024 14:19
@fmeum fmeum changed the title Add ArtifactVariable Add basic C++ path mapping support May 26, 2024
@fmeum fmeum force-pushed the 6526-cc-path-mapping branch 2 times, most recently from 3e0ee8b to fd78ca8 Compare May 26, 2024 14:35
@fmeum fmeum marked this pull request as ready for review May 26, 2024 14:35
@fmeum fmeum requested review from comius and removed request for lberki and oquenchil May 26, 2024 14:35
@github-actions github-actions bot added team-Performance Issues for Performance teams team-Rules-CPP Issues for C++ rules awaiting-review PR is awaiting review from an assigned reviewer labels May 26, 2024

expect_log 'Hi there, lib1!'
expect_log 'Hi there, lib2!'
# Compilation actions for lib1, lib2 and main should result in cache hits due
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@comius This test should serve as a representative example of what path mapping can do, but please let me know if there is any additional context I can provide to help you with the review.

@gregestren
Copy link
Contributor

Sanity check: what's the story with debug symbols?

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 10, 2024

Sanity check: what's the story with debug symbols?

At this point users need to opt in explicitly via --modify_execution_info and can simply choose not to do so when building with -c dbg. Long term we could either implement this behavior in rule logic or switch to a necessarily more complicated path mapping scheme based on content hashes or action hashes, but that would be very involved.

tools,
outputs,
mandatoryOutputs,
null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Readability: add /* localResources= */ . Only on arguments, where you can't figure out the parameter name. Same below.

@comius comius removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 14, 2024
@comius
Copy link
Contributor

comius commented Jun 14, 2024

I'll merge this manually, since other internal tests need fixups.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 14, 2024

I'll merge this manually, since other internal tests need fixups.

Thanks, are you going to take care of the /* ...= */ comments or should I?

fmeum added a commit to fmeum/bazel that referenced this pull request Jun 24, 2024
Basic support for path mapping for `CppCompile` actions is added by wiring up `PathMapper` with:
* structured path variables for files and include paths (`Artifact` and `NestedSet<PathFragment>`)
* inputs/outputs via `Spawn#getPathMapper()`
* header discovery

Also turns `external_include_paths` into a structured variable, which was missed in bazelbuild#22463.

The following features are known to be unsupported for now:
* `layering_check` (requires rewriting paths to and in module maps)
* source tree artifacts (requires wiring up `PathMapper` in `CppCompileActionTemplate`)
* location expanded paths to generated files such as sanitizer suppression lists (requires heuristically rewriting paths in `user_compile_flags`)

These limitations will be lifted in follow-up PRs.

Work towards bazelbuild#6526

RELNOTES: Experimental support for path mapping `CppCompile` actions can be enabled via `--modify_execution_info=CppCompile=+supports-path-mapping`.

Closes bazelbuild#22445.

PiperOrigin-RevId: 646109274
Change-Id: I6f4eb92b6be3052547f144c681b6588e9fc40693
@fmeum fmeum deleted the 6526-cc-path-mapping branch June 24, 2024 18:13
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 24, 2024

@bazel-io fork 7.3.0

fmeum added a commit to fmeum/bazel that referenced this pull request Jun 24, 2024
Basic support for path mapping for `CppCompile` actions is added by wiring up `PathMapper` with:
* structured path variables for files and include paths (`Artifact` and `NestedSet<PathFragment>`)
* inputs/outputs via `Spawn#getPathMapper()`
* header discovery

Also turns `external_include_paths` into a structured variable, which was missed in bazelbuild#22463.

The following features are known to be unsupported for now:
* `layering_check` (requires rewriting paths to and in module maps)
* source tree artifacts (requires wiring up `PathMapper` in `CppCompileActionTemplate`)
* location expanded paths to generated files such as sanitizer suppression lists (requires heuristically rewriting paths in `user_compile_flags`)

These limitations will be lifted in follow-up PRs.

Work towards bazelbuild#6526

RELNOTES: Experimental support for path mapping `CppCompile` actions can be enabled via `--modify_execution_info=CppCompile=+supports-path-mapping`.

Closes bazelbuild#22445.

PiperOrigin-RevId: 646109274
Change-Id: I6f4eb92b6be3052547f144c681b6588e9fc40693
fmeum added a commit to fmeum/bazel that referenced this pull request Jun 24, 2024
Basic support for path mapping for `CppCompile` actions is added by wiring up `PathMapper` with:
* structured path variables for files and include paths (`Artifact` and `NestedSet<PathFragment>`)
* inputs/outputs via `Spawn#getPathMapper()`
* header discovery

Also turns `external_include_paths` into a structured variable, which was missed in bazelbuild#22463.

The following features are known to be unsupported for now:
* `layering_check` (requires rewriting paths to and in module maps)
* source tree artifacts (requires wiring up `PathMapper` in `CppCompileActionTemplate`)
* location expanded paths to generated files such as sanitizer suppression lists (requires heuristically rewriting paths in `user_compile_flags`)

These limitations will be lifted in follow-up PRs.

Work towards bazelbuild#6526

RELNOTES: Experimental support for path mapping `CppCompile` actions can be enabled via `--modify_execution_info=CppCompile=+supports-path-mapping`.

Closes bazelbuild#22445.

PiperOrigin-RevId: 646109274
Change-Id: I6f4eb92b6be3052547f144c681b6588e9fc40693
fmeum added a commit to fmeum/bazel that referenced this pull request Jul 5, 2024
Basic support for path mapping for `CppCompile` actions is added by wiring up `PathMapper` with:
* structured path variables for files and include paths (`Artifact` and `NestedSet<PathFragment>`)
* inputs/outputs via `Spawn#getPathMapper()`
* header discovery

Also turns `external_include_paths` into a structured variable, which was missed in bazelbuild#22463.

The following features are known to be unsupported for now:
* `layering_check` (requires rewriting paths to and in module maps)
* source tree artifacts (requires wiring up `PathMapper` in `CppCompileActionTemplate`)
* location expanded paths to generated files such as sanitizer suppression lists (requires heuristically rewriting paths in `user_compile_flags`)

These limitations will be lifted in follow-up PRs.

Work towards bazelbuild#6526

RELNOTES: Experimental support for path mapping `CppCompile` actions can be enabled via `--modify_execution_info=CppCompile=+supports-path-mapping`.

Closes bazelbuild#22445.

PiperOrigin-RevId: 646109274
Change-Id: I6f4eb92b6be3052547f144c681b6588e9fc40693
github-merge-queue bot pushed a commit that referenced this pull request Jul 10, 2024
Basic support for path mapping for `CppCompile` actions is added by
wiring up `PathMapper` with:
* structured path variables for files and include paths (`Artifact` and
`NestedSet<PathFragment>`)
* inputs/outputs via `Spawn#getPathMapper()`
* header discovery

Also turns `external_include_paths` into a structured variable, which
was missed in #22463.

The following features are known to be unsupported for now:
* `layering_check` (requires rewriting paths to and in module maps)
* source tree artifacts (requires wiring up `PathMapper` in
`CppCompileActionTemplate`)
* location expanded paths to generated files such as sanitizer
suppression lists (requires heuristically rewriting paths in
`user_compile_flags`)

These limitations will be lifted in follow-up PRs.

Work towards #6526

RELNOTES: Experimental support for path mapping `CppCompile` actions can
be enabled via
`--modify_execution_info=CppCompile=+supports-path-mapping`.

Closes #22445.

PiperOrigin-RevId: 646109274
Change-Id: I6f4eb92b6be3052547f144c681b6588e9fc40693

Closes #22875

Co-authored-by: Yun Peng <pcloudy@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Performance Issues for Performance teams team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants