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

Open
wants to merge 2 commits into
base: master
Choose a base branch
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review PR is awaiting review from an assigned reviewer 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.

None yet

2 participants