Skip to content

Remove allowlist_include_directories from data#535

Closed
keith wants to merge 5 commits intobazelbuild:mainfrom
keith:ks/remove-allowlist_include_directories-from-data
Closed

Remove allowlist_include_directories from data#535
keith wants to merge 5 commits intobazelbuild:mainfrom
keith:ks/remove-allowlist_include_directories-from-data

Conversation

@keith
Copy link
Copy Markdown
Member

@keith keith commented Nov 25, 2025

When very recent versions of bazel, improvements in the merkle tree
recommend that instead of passing a huge list of files to the toolchain,
you instead pass the root directory of those files once, and then bazel
symlinks the entire directory as opposed to every file one by one. In
order to do this you must pass a filegroup pointing at only the
directory. In this case, if you then also pass all the files, you get
conflicts in bazel trying to link the files, and the directory. If you
didn't get those conflicts you would still lose the perf benefits.

This change makes allowlist_include_directories only affect the
cxx_builtin_include_directories field. If you need the directory also in
data, you must now also add it to data. If you want to continue using
the entire list of files, that still works fine.

When very recent versions of bazel, improvements in the merkle tree
recommend that instead of passing a huge list of files to the toolchain,
you instead pass the root directory of those files once, and then bazel
symlinks the entire directory as opposed to every file one by one. In
order to do this you must pass a filegroup pointing at only the
directory. In this case, if you then also pass all the files, you get
conflicts in bazel trying to link the files, and the directory. If you
didn't get those conflicts you would still lose the perf benefits.

This change makes allowlist_include_directories only affect the
cxx_builtin_include_directories field. If you need the directory also in
data, you must now also add it to `data`. If you want to continue using
the entire list of files, that still works fine.
@matts1
Copy link
Copy Markdown
Contributor

matts1 commented Nov 25, 2025

Tests appear to be broken, but LGTM otherwise

@armandomontanez
Copy link
Copy Markdown
Collaborator

Yeah, the tests ensure cxx_builtin_include_directories add files to toolchain file groups since it was part of the API contract.

@armandomontanez
Copy link
Copy Markdown
Collaborator

On that note, probably deserves a documentation update to clarify any changes in behavior.

@armandomontanez armandomontanez dismissed their stale review November 25, 2025 01:39

Need to update tests/docs.

@keith
Copy link
Copy Markdown
Member Author

keith commented Nov 25, 2025

updated tests and docs

@keith
Copy link
Copy Markdown
Member Author

keith commented Dec 3, 2025

@armandomontanez friendly ping 🙏🏻

@matts1
Copy link
Copy Markdown
Contributor

matts1 commented Dec 17, 2025

Our internal CI is complaining that //docs:toolchain_api_diff_test is failing. I would assume you updated the docs in starlark, but didn't regenerate them or something?

@keith
Copy link
Copy Markdown
Member Author

keith commented Dec 18, 2025

thanks, updated

@keith
Copy link
Copy Markdown
Member Author

keith commented Jan 21, 2026

@armandomontanez are you good with this one?

Copy link
Copy Markdown
Collaborator

@trybka trybka left a comment

Choose a reason for hiding this comment

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

Approving based on Armando and Matt's prior approvals with the tests/docs now fixed (docs CI fixed elsewhere)

@armandomontanez
Copy link
Copy Markdown
Collaborator

Yes, thanks! Should go in shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants