Skip to content

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 lberki and oquenchil as code owners 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.

@fmeum fmeum force-pushed the 6526-cc-path-mapping branch from fd78ca8 to 5be34a6 Compare May 26, 2024 14:37
@fmeum fmeum force-pushed the 6526-cc-path-mapping branch from 5be34a6 to 943ba4e Compare May 31, 2024 07:02
@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 <[email protected]>
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