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 NestedSet<PathFragment>- and Artifact-typed compile build variables #22463

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented May 21, 2024

This change prepares for C++ path mapping, which also needs to map include directories of generated headers. It may, as a side effect, reduce memory usage slightly by possibly reusing nested set instances retained elsewhere.

Work towards #6526

fmeum added 2 commits May 21, 2024 12:55
This change is intended to be memory neutral, but prepares for structured C++ path mapping.
This change is intended to reduce memory usage slightly by possibly reusing nested set instances retained elsewhere. It also prepares for C++ path mapping.
@fmeum fmeum requested a review from comius May 21, 2024 16:20
@fmeum fmeum marked this pull request as ready for review May 21, 2024 16:20
@fmeum fmeum requested a review from lberki as a code owner May 21, 2024 16:20
@fmeum
Copy link
Collaborator Author

fmeum commented May 21, 2024

This is stacked on #22462 (the first commit).

@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 21, 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.

LGTM, this matches my expectations on how this code will look in Starlark. Except the part with PathFragments, which will be reverted back to Strings. I think it’s still fine to have PathFragments in this PR.

@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 24, 2024
@fmeum fmeum changed the title Add NestedSet<PathFragment>-typed compile build variable Add NestedSet<PathFragment>- and Artifact-typed compile build variables May 24, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented May 24, 2024

@comius I updated the PR title to include #22462. I thought that you may want to merge them separately and thus created this stacked PR, but it's of course fine to merge them together.

@fmeum
Copy link
Collaborator Author

fmeum commented May 24, 2024

@comius If possible, please let me know what the effect on benchmarks was.

@comius
Copy link
Contributor

comius commented May 24, 2024

Will do. The PR will need some patching for the code behind the wall.

@comius
Copy link
Contributor

comius commented May 24, 2024

@comius If possible, please let me know what the effect on benchmarks was.

Benchmarking with --nobuild, analysis phase only on a project that's C++ intensive I get:

1.0% memory (significant )
5.1% wall time (significant)

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 24, 2024
@fmeum fmeum deleted the 6526-nested-set-variable branch May 24, 2024 12:48
@fmeum
Copy link
Collaborator Author

fmeum commented May 24, 2024

@bazel-io fork 7.2.0

fmeum pushed a commit to fmeum/bazel that referenced this pull request May 27, 2024
…iables

This change prepares for C++ path mapping, which also needs to map include directories of generated headers. It may, as a side effect, reduce memory usage slightly by possibly reusing nested set instances retained elsewhere.

Work towards bazelbuild#6526

Closes bazelbuild#22463.

PiperOrigin-RevId: 636886343
Change-Id: Ic93d6439a51f37f44bb2ac67d0813c48c0c4a8bd
github-merge-queue bot pushed a commit that referenced this pull request May 28, 2024
…uild var… (#22557)

…iables

This change prepares for C++ path mapping, which also needs to map
include directories of generated headers. It may, as a side effect,
reduce memory usage slightly by possibly reusing nested set instances
retained elsewhere.

Work towards #6526

Closes #22463.

PiperOrigin-RevId: 636886343
Change-Id: Ic93d6439a51f37f44bb2ac67d0813c48c0c4a8bd

Fixes #22533

Co-authored-by: Googler <ilist@google.com>
copybara-service bot pushed a commit 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 #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
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 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-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants