Resolve redundant bazel binary+benchmark target names

[wrowe]

Commit Message:
Resolve redundant bazel binary+benchmark target names

• Solves some path-too-long problems on x64-windows-fastbuild targets
  (shorter ...-opt and ...-dbg path names had just squeeked by till now)

• benchmark_test already implies speed_test

• speed_test describes the measurement tool

• benchmark_test describes the observation/evaluation

Additional Description:
Risk Level: low
Testing: local
Docs Changes: n/a
Release Notes: n/a

[wrowe]

Note two specific _speed_test(s) which never appear to be used as _benchmark_tests?
//test/common/stats:recent_lookups_speed_test
//test/common/stats:symbol_table_speed_test

@jmarantz these seem to be yours?
@antoniovicente were these missed by your 4ab8abd refactoring?

[wrowe]

This normalizes the existing most-prevalent pattern, where is best that we might document it @htuch or is that not really necessary (using the existing BUILD as living template for new components?)

[antoniovicente]

Note two specific _speed_test(s) which never appear to be used as _benchmark_tests?
//test/common/stats:recent_lookups_speed_test
//test/common/stats:symbol_table_speed_test

@jmarantz these seem to be yours?
@antoniovicente were these missed by your 4ab8abd refactoring?

thanks for pointing that out, I'll make it note to get that resolved.

[antoniovicente]

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.

[wrowe]

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?

[antoniovicente]

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.

[htuch]

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?

[wrowe]

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?)

[antoniovicente]

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.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[antoniovicente]

Checking back on this now I'm back from vacation, could you take a look at open comment threads?

[antoniovicente]

Note two specific _speed_test(s) which never appear to be used as _benchmark_tests?
//test/common/stats:recent_lookups_speed_test
//test/common/stats:symbol_table_speed_test

@jmarantz these seem to be yours?
@antoniovicente were these missed by your 4ab8abd refactoring?

Sent out #14331

[wrowe]

I need to close this PR, as we are losing our shared dev org and reconsolidating, and github doesn't let me rebase to a different remote. At this time, can I hand off any remaining normalization tasks to you, @antoniovicente, we don't seem to have any lingering long-path issues, but I'll reconfirm as we get clang-cl building for the new CI pipeline.

[wrowe]

Just FYI this PR branch won't disappear until just after year-end, if there is anything you wish to scrape.