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.

Sign up for GitHub

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

Jump to bottom

[bazel] Use includes instead of strip_include_prefix #1803

Merged
merged 1 commit into from
Jun 17, 2024
Merged

[bazel] Use includes instead of strip_include_prefix #1803

merged 1 commit into from
Jun 17, 2024

Conversation

alexkaratarakis
Copy link
Contributor

When using includes, consumers will apply the headers using -isystem, instead of -I. This will allow diagnostics of consumers to not apply to benchmark.

More info:

https://bazel.build/reference/be/c-cpp#cc_library.includes

https://bazel.build/reference/be/c-cpp#cc_library.strip_include_prefix

gtest uses includes as well:
https://github.com/google/googletest/blob/1d17ea141d2c11b8917d2c7d029f1c4e2b9769b2/BUILD.bazel#L120

alexkaratarakis added a commit to alexkaratarakis/fixed-containers that referenced this pull request Jun 14, 2024
@alexkaratarakis
Fix benchmark builds for bazel
50570c1 
google/benchmark#1803
@alexkaratarakis alexkaratarakis mentioned this pull request Jun 14, 2024
Merged
alexkaratarakis added a commit to alexkaratarakis/fixed-containers that referenced this pull request Jun 14, 2024
@alexkaratarakis
Fix benchmark builds for bazel
8555cca 
google/benchmark#1803

```
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1801:25: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
 1801 |           memory_result(NULL),
      |                         ^~~~
      |                         nullptr
/usr/lib/llvm-17/lib/clang/17/include/stddef.h:84:18: note: expanded from macro 'NULL'
   84 | #    define NULL __null
      |                  ^
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:378:7: error: 'MemoryManager' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
  378 | class MemoryManager {
      |       ^
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1437:7: error: 'Fixture' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
 1437 | class Fixture : public internal::Benchmark {
      |       ^
3 errors generated.

```
alexkaratarakis added a commit to teslamotors/fixed-containers that referenced this pull request Jun 14, 2024
@alexkaratarakis
Fix benchmark builds for bazel
24e67c4 
google/benchmark#1803

```
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1801:25: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
 1801 |           memory_result(NULL),
      |                         ^~~~
      |                         nullptr
/usr/lib/llvm-17/lib/clang/17/include/stddef.h:84:18: note: expanded from macro 'NULL'
   84 | #    define NULL __null
      |                  ^
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:378:7: error: 'MemoryManager' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
  378 | class MemoryManager {
      |       ^
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1437:7: error: 'Fixture' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
 1437 | class Fixture : public internal::Benchmark {
      |       ^
3 errors generated.

```
@dmah42
Copy link
Member

dmah42 commented Jun 14, 2024

i had originally decided to not use includes for exactly this reason: that it is not a system include and would normally be included with a manual installation as -I rather than -isystem. however, i recognise the diagnostic issues that can be exposed through this, and that this PR simplifies integration for clients, so i'm happy to accept the change.

thanks so much :)

@dmah42
Copy link
Member

dmah42 commented Jun 14, 2024

looks like precommit has an opinion about ordering of fields.

@alexkaratarakis
[bazel] Use includes instead of strip_include_prefix
4af61d1 
When using `includes`, consumers will apply the headers
using `-isystem`, instead of `-I`. This will allow diagnostics
of consumers to not apply to `benchmark`.

More info:

https://bazel.build/reference/be/c-cpp#cc_library.includes

https://bazel.build/reference/be/c-cpp#cc_library.strip_include_prefix

gtest uses `includes` as well:
https://github.com/google/googletest/blob/1d17ea141d2c11b8917d2c7d029f1c4e2b9769b2/BUILD.bazel#L120
@alexkaratarakis alexkaratarakis force-pushed the benchmark_as_system_include branch from a53f931 to 4af61d1 Compare June 15, 2024 08:28
@alexkaratarakis
Copy link
Contributor Author

looks like precommit has an opinion about ordering of fields.

Fixed and verified locally:

$ pre-commit run --all-files
buildifier...............................................................Passed
buildifier-lint..........................................................Passed
mypy.....................................................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed

@dmah42
Copy link
Member

dmah42 commented Jun 17, 2024

unrelated failure. thank you for the fix.

@dmah42 dmah42 merged commit 4477525 into google:main Jun 17, 2024
79 of 80 checks passed
@alexkaratarakis alexkaratarakis deleted the benchmark_as_system_include branch June 18, 2024 06:59
alexkaratarakis added a commit to alexkaratarakis/fixed-containers that referenced this pull request Jun 18, 2024
@alexkaratarakis
Uprev google/benchmark and remove upstreamed patch
b5b81fa 
google/benchmark#1803
@alexkaratarakis alexkaratarakis mentioned this pull request Jun 18, 2024
Merged
alexkaratarakis added a commit to teslamotors/fixed-containers that referenced this pull request Jun 18, 2024
@alexkaratarakis
Uprev google/benchmark and remove upstreamed patch
e1ecea7 
google/benchmark#1803
EazyReal pushed a commit to EazyReal/fixed-containers that referenced this pull request Jul 16, 2024
@alexkaratarakis
Fix benchmark builds for bazel
c1a1b53 
google/benchmark#1803

```
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1801:25: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
 1801 |           memory_result(NULL),
      |                         ^~~~
      |                         nullptr
/usr/lib/llvm-17/lib/clang/17/include/stddef.h:84:18: note: expanded from macro 'NULL'
   84 | #    define NULL __null
      |                  ^
In file included from test/fixed_map_perf_test.cpp:4:
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:378:7: error: 'MemoryManager' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
  378 | class MemoryManager {
      |       ^
bazel-out/k8-fastbuild/bin/external/com_google_benchmark/_virtual_includes/benchmark_main/benchmark/benchmark.h:1437:7: error: 'Fixture' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit [-Werror,-Wweak-vtables]
 1437 | class Fixture : public internal::Benchmark {
      |       ^
3 errors generated.

```
EazyReal pushed a commit to EazyReal/fixed-containers that referenced this pull request Jul 16, 2024
@alexkaratarakis
Uprev google/benchmark and remove upstreamed patch
48d5bdd 
google/benchmark#1803
@BrewTestBot BrewTestBot mentioned this pull request Jul 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmah42 dmah42 dmah42 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alexkaratarakis @dmah42