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

Resolve redundant bazel binary+benchmark target names #13327

Closed
wants to merge 1 commit into from
Closed

Resolve redundant bazel binary+benchmark target names #13327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/common/buffer/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,6 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "buffer_speed_test_benchmark_test",
name = "buffer_benchmark_test",
benchmark_binary = "buffer_speed_test",
)
4 changes: 2 additions & 2 deletions test/common/common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "logger_speed_test_benchmark_test",
name = "logger_benchmark_test",
benchmark_binary = "logger_speed_test",
)

Expand Down Expand Up @@ -278,7 +278,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "utility_speed_test_benchmark_test",
name = "utility_benchmark_test",
benchmark_binary = "utility_speed_test",
)

Expand Down
2 changes: 1 addition & 1 deletion test/common/formatter/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,6 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "substitution_formatter_speed_test_benchmark_test",
name = "substitution_formatter_benchmark_test",
benchmark_binary = "substitution_formatter_speed_test",
)
4 changes: 2 additions & 2 deletions test/common/http/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "codes_speed_test_benchmark_test",
name = "codes_benchmark_test",
benchmark_binary = "codes_speed_test",
)

Expand Down Expand Up @@ -280,7 +280,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "header_map_impl_speed_test_benchmark_test",
name = "header_map_impl_benchmark_test",
benchmark_binary = "header_map_impl_speed_test",
)

Expand Down
4 changes: 2 additions & 2 deletions test/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "address_impl_speed_test_benchmark_test",
name = "address_impl_benchmark_test",
benchmark_binary = "address_impl_speed_test",
)

Expand Down Expand Up @@ -368,7 +368,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "lc_trie_speed_test_benchmark_test",
name = "lc_trie_benchmark_test",
benchmark_binary = "lc_trie_speed_test",
)

Expand Down
2 changes: 1 addition & 1 deletion test/common/stats/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "thread_local_store_speed_test_benchmark_test",
Copy link
Member

Choose a reason for hiding this comment

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

At a high-level, if saving of a few characters on a build label is making this difference on Windows, it seems pretty fragile. E.g. what if another directory level is added? Is there a tracking issue for a longer-term fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, benchmark_test_speed_test is hardly saving a few characters, when you multiply that by as many as three redundant occurrences in resulting bazel path names. The general limit on the fully-rooted path name is practically 255 characters, across many contexts such as using it as an arg in a script, etc. (The true limit is 32k, but msys2 unix-ish tools and many other utilities aren't aware of that.) The true limit of the entire command line to CreateProcess is 32k, and again msys2 and other shells may impose smaller limits.

We've already taken what actions we can with respect to the length of the paths, and recommend c:_eb\execroot\ as the shortest realistic bazel root path. One possible enhancement is renaming the output tree from x64_windows-fastbuild to x64_windows-fst (to be the same length as the frequently reviewed x64_windows-opt or -dbg target trees), although this change needs to happen in the envoy bazel build toolchain creation.

The underlying problem are the choices that the Bazel tooling has imposed in constructing overly verbose and redundant pathname constructs, this isn't really an Envoy problem to solve, and isn't really a solvable problem. I don't believe we are going to be able to enable short path names on the deployments of the GCP CI instances (if that were possible, it's hard to imagine why Google hasn't done that for bazel in the first place.)

This is the current build failure that this patch aims to fix; and I'm happy to fix this and this alone in conformance with the project standard, but as this PR observed, there isn't a consistent project standard.

INFO: Analyzed target //test/extensions/filters/http/common/compressor:compressor_filter_speed_test_benchmark_test (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
FAIL: //test/extensions/filters/http/common/compressor:compressor_filter_speed_test_benchmark_test (see C:/_eb/execroot/envoy/bazel-out/x64_windows-fastbuild/testlogs/test/extensions/filters/http/common/compressor/compressor_filter_speed_test_benchmark_test/test.log)
INFO: From Testing //test/extensions/filters/http/common/compressor:compressor_filter_speed_test_benchmark_test:
==================== Test output for //test/extensions/filters/http/common/compressor:compressor_filter_speed_test_benchmark_test:
ERROR(tools/test/windows/tw.cc:1288) ERROR: src/main/native/windows/process.cc(95): WaitableProcess::Create(C:\_eb\execroot\envoy\bazel-out\x64_windows-fastbuild\bin\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe.runfiles\envoy\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe): ERROR: src/main/native/windows/util.cc(287): AsExecutablePathForCreateProcess(C:\_eb\execroot\envoy\bazel-out\x64_windows-fastbuild\bin\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe.runfiles\envoy\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe): ERROR: src/main/native/windows/util.cc(257): GetShortPathNameW(\\?\C:\_eb\execroot\envoy\bazel-out\x64_windows-fastbuild\bin\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe.runfiles\envoy\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe): cannot shorten the path enough
ERROR(tools/test/windows/tw.cc:1445) Failed to start test process (arg: C:\_eb\execroot\envoy\bazel-out\x64_windows-fastbuild\bin\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe.runfiles\envoy\test\extensions\filters\http\common\compressor\compressor_filter_speed_test_benchmark_test.exe)
================================================================================
Target //test/extensions/filters/http/common/compressor:compressor_filter_speed_test_benchmark_test up-to-date:
  bazel-bin/test/extensions/filters/http/common/compressor/compressor_filter_speed_test_benchmark_test
  bazel-bin/test/extensions/filters/http/common/compressor/compressor_filter_speed_test_benchmark_test.exe
INFO: Elapsed time: 13.128s, Critical Path: 3.84s
INFO: 2 processes: 2 local.
INFO: Build completed, 1 test FAILED, 2 total actions

I'm happy to close this PR and move ahead with PR to fix of this single failure alone, and others on a one-off basis as we encounter them.

@antoniovicente can I simply pass off this PR to you (or drop the subject entirely?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the name of the test targets seems fine. I would just skip file renames.

I can deal with getting things in a more consistent state.

name = "thread_local_store_benchmark_test",
benchmark_binary = "thread_local_store_speed_test",
)

Expand Down
8 changes: 4 additions & 4 deletions test/common/upstream/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "eds_speed_test_benchmark_test",
name = "eds_benchmark_test",
benchmark_binary = "eds_speed_test",
)

Expand Down Expand Up @@ -457,8 +457,8 @@ envoy_cc_test(
)

envoy_cc_benchmark_binary(
name = "load_balancer_benchmark",
srcs = ["load_balancer_benchmark.cc"],
name = "load_balancer_speed_test",
srcs = ["load_balancer_speed_test.cc"],
Copy link
Contributor

Choose a reason for hiding this comment

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

The ideal naming for these would be closer to the naming of load_balancer_benchmark and load_balancer_benchmark_test

All the "speed_test" binaries are not actually tests. The don't use cc_test target. They are benchmark binaries. If anything we should rename the speed_tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess my concern is that _benchmark alone is a rather ambiguous.

What about a compromise such as load_balancer_benchmark_test being the invocation (testing) of the benchmark, and give binary we invoke to do that testing a title such as load_balancer_benchmark_utility? E.g. _test in almost all existing envoy cases being the verb/action (and very often synonymous with tool employed to perform that test), while many _test targets actually invoke envoy_static or other scripts or utilities. (For example, build_id_test, envoy_static_test, pie_test, and version_out_test all are actually invoking envoy-static.)

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is to not do these cc file renames in this PR.

Ideally the _test suffix should only be used for targets that end up running when you invoke bazel test. It would be great to use a consistent and unambiguous suffix for all CPU usage measurement targets; I can take an AI to come up with something and make the renames happen.

external_deps = [
"benchmark",
],
Expand All @@ -478,7 +478,7 @@ envoy_cc_benchmark_binary(
envoy_benchmark_test(
name = "load_balancer_benchmark_test",
timeout = "long",
benchmark_binary = "load_balancer_benchmark",
benchmark_binary = "load_balancer_speed_test",
)

envoy_cc_test(
Expand Down
0 ...ommon/upstream/load_balancer_benchmark.cc → ...mmon/upstream/load_balancer_speed_test.cc
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion test/extensions/filters/http/common/compressor/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,6 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "compressor_filter_speed_test_benchmark_test",
name = "compressor_filter_benchmark_test",
benchmark_binary = "compressor_filter_speed_test",
)
6 changes: 3 additions & 3 deletions test/extensions/filters/listener/tls_inspector/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ envoy_cc_fuzz_test(
)

envoy_extension_cc_benchmark_binary(
name = "tls_inspector_benchmark",
srcs = ["tls_inspector_benchmark.cc"],
name = "tls_inspector_speed_test",
srcs = ["tls_inspector_speed_test.cc"],
extension_name = "envoy.filters.listener.tls_inspector",
external_deps = [
"benchmark",
Expand All @@ -71,7 +71,7 @@ envoy_extension_cc_benchmark_binary(

envoy_extension_benchmark_test(
name = "tls_inspector_benchmark_test",
benchmark_binary = "tls_inspector_benchmark",
benchmark_binary = "tls_inspector_speed_test",
extension_name = "envoy.filters.listener.tls_inspector",
)

Expand Down
0 .../tls_inspector/tls_inspector_benchmark.cc → ...tls_inspector/tls_inspector_speed_test.cc
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions test/extensions/filters/network/redis_proxy/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ envoy_extension_cc_benchmark_binary(
)

envoy_extension_benchmark_test(
name = "command_lookup_speed_test_benchmark_test",
name = "command_lookup_benchmark_test",
benchmark_binary = "command_lookup_speed_test",
extension_name = "envoy.filters.network.redis_proxy",
)
Expand Down Expand Up @@ -184,7 +184,7 @@ envoy_extension_cc_benchmark_binary(
)

envoy_extension_benchmark_test(
name = "command_split_speed_test_benchmark_test",
name = "command_split_benchmark_test",
benchmark_binary = "command_split_speed_test",
extension_name = "envoy.filters.network.redis_proxy",
)
8 changes: 4 additions & 4 deletions test/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,8 +427,8 @@ envoy_cc_test(
)

envoy_cc_benchmark_binary(
name = "filter_chain_benchmark_test",
srcs = ["filter_chain_benchmark_test.cc"],
name = "filter_chain_speed_test",
srcs = ["filter_chain_speed_test.cc"],
external_deps = [
"benchmark",
"googletest",
Expand All @@ -445,7 +445,7 @@ envoy_cc_benchmark_binary(
)

envoy_benchmark_test(
name = "filter_chain_benchmark_test_benchmark_test",
name = "filter_chain_benchmark_test",
timeout = "long",
benchmark_binary = "filter_chain_benchmark_test",
benchmark_binary = "filter_chain_speed_test",
)
0 test/server/filter_chain_benchmark_test.cc → test/server/filter_chain_speed_test.cc
View file
Open in desktop
File renamed without changes.