Skip to content

[bazel] Use includes instead of strip_include_prefix#1803

Merged
dmah42 merged 1 commit into
google:mainfrom
alexkaratarakis:benchmark_as_system_include
Jun 17, 2024
Merged

[bazel] Use includes instead of strip_include_prefix#1803
dmah42 merged 1 commit into
google:mainfrom
alexkaratarakis:benchmark_as_system_include

Conversation

@alexkaratarakis

@alexkaratarakis alexkaratarakis commented Jun 14, 2024

Copy link
Copy Markdown
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

dmah42 commented Jun 14, 2024

Copy link
Copy Markdown
Member

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

dmah42 commented Jun 14, 2024

Copy link
Copy Markdown
Member

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

alexkaratarakis commented Jun 15, 2024

Copy link
Copy Markdown
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

dmah42 commented Jun 17, 2024

Copy link
Copy Markdown
Member

unrelated failure. thank you for the fix.

@dmah42 dmah42 merged commit 4477525 into google:main Jun 17, 2024
@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