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

virtual includes and headers only cc_library #18913

Closed
lromor opened this issue Jul 12, 2023 · 10 comments
Closed

virtual includes and headers only cc_library #18913

lromor opened this issue Jul 12, 2023 · 10 comments
Labels
awaiting-user-response Awaiting a response from the author P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: support / not a bug (process)

Comments

@lromor
Copy link

lromor commented Jul 12, 2023

Description of the bug:

I'm trying to port into bazel a cc library. The library uses system includes (say #include <dir/foo.h>) internally.
The headers are included both using double quotes and angle brackets. Externally is used mostly via angle brackets.

I made a simple repository that mocks the situation:

When I try to compile bazel build //dir:foo I get:

dir/foo.cc:3:10: fatal error: dir/foo.h: No such file or directory
    3 | #include <dir/foo.h>

By inspecting the sandbox with and adding more verbosity, the command is the following:

 (cd /home/lromor/.cache/bazel/_bazel_lromor/72cb75e6582d64c688eee6d6659304e6/sandbox/linux-sandbox/2/execroot/__main__ && \
  exec env - \
    PATH=<redacted> \
    PWD=/proc/self/cwd \
    TMPDIR=/tmp \
  /home/lromor/.cache/bazel/_bazel_lromor/install/50a5a3eaeaa5297d31568ab6a3a702d9/linux-sandbox -t 15 -w /home/lromor/.cache/bazel/_bazel_lromor/72cb75e6582d64c688eee6d6659304e6/sandbox/linux-sandbox/2/execroot/__main__ -w /tmp -w /dev/shm -S /home/lromor/.cache/bazel/_bazel_lromor/72cb75e6582d64c688eee6d6659304e6/sandbox/linux-sandbox/2/stats.out -D -- /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++0x' -MD -MF bazel-out/k8-fastbuild/bin/dir/_objs/foo/foo.pic.d '-frandom-seed=bazel-out/k8-fastbuild/bin/dir/_objs/foo/foo.pic.o' -fPIC '-DBAZEL_CURRENT_REPOSITORY=""' -iquote . -iquote bazel-out/k8-fastbuild/bin -Ibazel-out/k8-fastbuild/bin/dir/_virtual_includes/dir_headers -fno-canonical-system-headers -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c dir/foo.cc -o bazel-out/k8-fastbuild/bin/dir/_objs/foo/foo.pic.o)

As you can see, bazel-out/k8-fastbuild/bin/dir/_virtual_includes/dir_headers is included, but there's no such directory or path in the sandbox environment. My expectation reading the commandline would be:

  1. To find the the location to be existing.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Example

Which operating system are you running Bazel on?

Debian

What is the output of bazel info release?

release 6.2.1

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

N/A

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

N/A

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

N/A

Have you found anything relevant by searching the web?

N/A

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

N/A

@fmeum
Copy link
Collaborator

fmeum commented Jul 17, 2023

The virtual headers directory isn't created since include_prefix = "dir" is the default include prefix for a library defined in package dir. If you change this to include_prefix = "not_dir", you can see that the virtual header symlink is indeed created.

Not creating the virtual includes in this situation appears to be an optimization, but since default include paths use -iquote and virtual includes appear to use -I, there is a difference in behavior. I don't know whether this is intentional.

@lromor
Copy link
Author

lromor commented Jul 17, 2023

I see! Thanks for clarifying. Indeed for my use case would have been nice to have a -I instead of a -iquote.. But I guess there must be, as you said, some internal reason for this choice. I wonder if there's any way to get around this or is it the only way to make a custom rule?

@buildbreaker2021
Copy link
Contributor

The behavior you are looking for can be achieved via copts I believe. However I am not sure if this is ideal but here it goes.

In your BUILD file if you remove: https://github.com/lromor/bazel_cc_prefix/blob/e55daf4285b82fbdcdd755a3b6d21c183e24d0d3/dir/BUILD#L16

And add copts = ["-I", "."] to foo cc_library and build should work.
Also wrong include here: https://github.com/lromor/bazel_cc_prefix/blob/e55daf4285b82fbdcdd755a3b6d21c183e24d0d3/dir/foo.cc#L3

needs to be bar.h.

@buildbreaker2021 buildbreaker2021 added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: support / not a bug (process) awaiting-user-response Awaiting a response from the author and removed type: bug untriaged labels Aug 16, 2023
@lromor
Copy link
Author

lromor commented Aug 21, 2023

Hi @buildbreaker2021 , thank you for the heads up, I have now fixed the example.
Your solution indeed works for compiling the library, but not for the dependencies depending on such library. The include prefix would have to be included for each one of them. Quoting the docs:

Each string in this attribute is added in the given order to COPTS before compiling the binary target. The flags take effect only for compiling this target, not its dependencies, so be careful about header files included elsewhere. All paths should be relative to the workspace, not to the current package.

This makes the usage of the third party library less maintainable. What would be your recommended approach? I've seen people copying public headers on a separate "generated" folder and use it as exported system headers.

@lromor
Copy link
Author

lromor commented Aug 21, 2023

Also, it is still very surprising for me that adding https://github.com/lromor/bazel_cc_prefix/blob/main/dir/BUILD#L16 adds a virtual include parameter to the compiler, but doesn't generate the actual folder. Is this behavior wanted or documented?

@buildbreaker2021
Copy link
Contributor

buildbreaker2021 commented Aug 22, 2023

Also, it is still very surprising for me that adding https://github.com/lromor/bazel_cc_prefix/blob/main/dir/BUILD#L16 adds a virtual include parameter to the compiler, but doesn't generate the actual folder. Is this behavior wanted or documented?

My answer to both is no. Probably it is because of this check:

if (!originalHeader.getExecPath().equals(includePath)) {

And my assumption is that we do not do similar check when we construct command line. However that should only effect cases when package directory is the same as include prefix.

What would be your recommended approach?

Would you still have problem if you used other directory name instead of dir? Basically what @fmeum has mentioned then you would get -I on the command line which should be able to find headers in angle brackets no?

As a side note, you can try and use includes attribute. However then you probably need to do #include <bar.h> instead of #include <dir/bar.h>. Good thing about includes is that it uses -I to add headers to the command line.

@lromor
Copy link
Author

lromor commented Aug 22, 2023

Thank you for your answer. The problem is for me during the porting of some thirdy party library which has these system imports all over the codebase. It would either mean to patch the whole library and change the dir prefix or find another way.

By looking around in other projects I noticed that people tend to make a copy into a "generated" folder to get around it, for instance: https://github.com/hdl/bazel_rules_hdl/blob/main/dependency_support/org_sourceware_libffi/bundled.BUILD.bazel#L89-L92

This is the cleanest method I found so far and sounds an extension of what you suggested.

@buildbreaker2021
Copy link
Contributor

The problem is for me during the porting of some thirdy party library which has these system imports all over the codebase.

Right, that's what I thought might create problems. To answer your question considering everything above, I think generated folder approach looks fine, I don't see anything better ATM.

@PikachuHyA
Copy link
Contributor

Also, it is still very surprising for me that adding https://github.com/lromor/bazel_cc_prefix/blob/main/dir/BUILD#L16 adds a virtual include parameter to the compiler, but doesn't generate the actual folder. Is this behavior wanted or documented?

These links may explain why the actual folders are not being generated.

Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 14, 2025
@lromor lromor closed this as completed Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-user-response Awaiting a response from the author P3 We're not considering working on this, but happy to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-CPP Issues for C++ rules type: support / not a bug (process)
Projects
None yet
Development

No branches or pull requests

6 participants