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

If a C++ directory referenced by include_prefix already exists in the repo, prefer to use that rather than _virtual_includes #17591

Open
johngkhs opened this issue Feb 24, 2023 · 3 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@johngkhs
Copy link

johngkhs commented Feb 24, 2023

Description of the feature request:

Hi,

I am compiling my C++ project with bazel, and we use include_prefix referencing folders which already exist in our build. For LSP completion, we generate a compile_commands.json which then has lines referencing the inclusion of these virtual includes. However, it is preferable that these includes are direct when possible, since this doesn't entail copying / symlinking files and also would make the LSP references better (referencing the actual files rather than _virtual_includes files deep in the bazel-out directory). More context and discussion can be found here: hedronvision/bazel-compile-commands-extractor#102.

What underlying problem are you trying to solve with this feature?

include_prefix can be much more user friendly if it only generates virtual directories when it actually needs to.
When possible, can bazel optimize include_prefix and strip_include_prefix to just use includes directly, so the end user can use any of those three options without worrying about it.

Which operating system are you running Bazel on?

Ubuntu 20.04.1 LTS

What is the output of bazel info release?

release 4.0.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@cpsauer
Copy link
Contributor

cpsauer commented Feb 25, 2023

[Compile commands extractor maintainer here, echoing the same.] I think a key benefit of having this optimization in Bazel would be that it improves the handling of the construct across editor plugins and simplifies the interface for bazel users by eliminating needing to think about the difference between includes and include_prefix&strip_include_prefix. There's also some performance improvement, of course.

Thanks!
Chris
(ex-Googler)

P.S. For clarity, note that not all include_prefixes&strip_include_prefix pairs can be optimized to the equivalent includes. This is just about the ones that can.

@buildbreaker2021 buildbreaker2021 added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Mar 6, 2023
@cpsauer
Copy link
Contributor

cpsauer commented May 31, 2023

[Newly discovered] Heads up: Some repositories depend on the implementation details of strip_include_prefix over includes to enforce something like layering check and prevent the leaking of implementation headers that then shadow system headers because they have the same name. See discussion in google/glog#926. Your call on whether this eliminates the optimization opportunity--or whether this is behavior you don't want depended on and they should rename the colliding headers.

@keith
Copy link
Member

keith commented Jun 24, 2024

related: #18683 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants