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

Windows: enable tests and envoy-static.exe pdb file #13688

Merged
merged 30 commits into from
Dec 1, 2020
Merged

Windows: enable tests and envoy-static.exe pdb file #13688

merged 30 commits into from
Dec 1, 2020

Conversation

sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Oct 21, 2020

Commit Message: Windows: enable tests and envoy-static.exe pdb file
Additional Description:

  • Introduce debugging output for msvc-cl and clang-cl opt binaries in
    order to be able to unwind stack traces from windows core files.
  • Mark additional tests fails on windows based on clang-cl build
    observed test failures
  • Work around problematic googletests with clang-cl compilation
  • Solve problematic missing override declarations of mocks by temporarilly
    adding -Wno-inconsistent-missing-override for clang. (It appears
    googletest on clang-cl isn't handling something that our gcc, clang and
    msvc-cl builds all ignore without the warning override.
  • Workaround THROW tests which googletest with clang-cl are framing as
    as -Wdiscard errors.
  • Workaround nodiscard next() method error
  • Follow up about discarded results of singleton construction and other
    discarded odd or apparently irrelevant function call results.
  • Holding on adding clang-cl CI until LLVM 11.0.0 can be used on Windows
    with Bazel (requires Bazel 3.7.1 and 4.0.0 releases)

Risk Level: Low (only affects Windows build)
Testing: Verified locally
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Related to #11974 (partially addresses)

@sunjayBhatia
Copy link
Member Author

cc @envoyproxy/windows-dev

wrowe and others added 6 commits October 29, 2020 16:39
Solve problematic missing override declarations of mocks by temporarilly
adding -Wno-inconsistent-missing-override for clang. (It appears
googletest on clang-cl isn't handling something that our gcc, clang and
msvc-cl builds all ignore without the warning override.

Workaround THROW tests which googletest with clang-cl are framing as
as -Wdiscard errors.

Workaround nodiscard next() method error

Follow up about discarded results of singleton construction and other
discarded odd or apparently irrelevant function call results.

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
- Adds a second build clang-cl using the LLVM toolchain

- Introduce debugging output for msvc-cl and clang-cl opt binaries in
  order to be able to unwind stack traces from windows core files.
  (required increasing the available heap on the RBE GCP workers)

- Fix lld-link.exe errors caused by unexported destructors

- Mark additional tests flaky/fails on windows based on clang-cl build
  observed test failures

Potential additional changes for more efficient fastbuild linkage

- Stop using alwayslink/whole_archive logic to link all //source/...
  libraries used by //test/... binaries, because this increases the .exe
  and .pdb output file sizes and computational time. This requires we
  identify the libraries which must be resolved at run-time such as
  singletons, filter configs, etc, vs. those libraries which do not need
  to be preserved in full, and then override any default alwayslink.

- Simplify binary of the objects used. (-OPT:REF optimization) due to
  tremendous size of resulting .exe and .pdb files, one alwayslink is
  resolved.

- Enable dynamic envoy libs and clib/stdlib for test fastbuild objects
  (this presumes the objects would always be invoked on the machine with
  the same compiler and dynamic runtime already installed.) Requires that
  we fix the luajit/moonjit patches to toggle the /MD or /MT flag based on
  bazel config (much like we vary their msvcbuild.bat file for debug vs.
  non-debug.)

Potential changes to bazel rules_cc for better builds

- Deduplicate `-ignore:4221` flag and system library names from link params.

- Allow /DEBUG:FASTLINK to be replaced with /DEBUG:FULL or /DEBUG:NONE,
  and validate that FASTLINK results in faster linkage+invocation times
  for tests than /DEBUG:FULL. Also determine whether /DEBUG:NONE is in
  the spirit of "minimal debugging information" as documented by Bazel.

Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Add the broken_clang_cl_dtor tag to tests which cannot compile due
to some windows-specific clang scope or optimization issue.

Issue is observed when the class used is based on a : public virtual foo,
base class, that ~foo() = default; destructor declaration is ignored, and
the linker reports that foo's dtor symbol cannot be resolved at link time.
It appears that when the base is redefined as : public foo the defect goes
away. (The virtual base class may be nested in a non-virtual base class).

It isn't possible to solve this by dropping every virtual class inherited,
due to diamond inheritance patterns.

This may be a compiler bug, or an optimization toggled by /O2 in clang-cl
which they do not enable in -O2 on linux. Tableing this for the time being.

Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
This restores commit 01c6aa7.

This commit needs a finer grain of precision to avoid building on
clang-cl but not avoid building on msvc-cl.

Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
These fixes reflect that the timing on windows is often less
synchronous than linux observations, due to the design of the
underlying socket stack.

- listener_impl_test
    TcpListenerImplTest.SetListenerRejectFractionIntermediate

- Enable no-longer-failing tests (no failure observed in RBE or locally)
    //test/extensions/grpc_credentials/file_based_metadata:integration_test
    //test/extensions/transport_sockets/alts:tsi_handshaker_test
    //test/integration:http2_flood_integration_test
    //test/integration:http2_upstream_integration_test
    //test/integration:overload_integration_test
    //test/server:guarddog_impl_test

Co-authored-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
- These skipped tests are all missing exported destructors of pure
  virtual classes. Enable the compilation to fail the clang-cl build
  and demonstrate what needs to be fixed (this may be a clang-cl
  compiler bug, or a gmocks bug, or buried elsewhere, but they do
  not appear to be envoy source code bugs.)

Co-authored-by: William A Rowe Jr <[email protected]>
Co-authored-by: Sunjay Bhatia <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

Reassigning to @htuch since #11974 was instigated by himself.

Note this patch includes one commit of miscellaneous failure resolutions, which actually land in PR #13837 ... As I'm converting this to ready for review, we are done force-pushing changes, but we will merge master once that other straightforward PR is done and these will be simplified here.

Also, some 16 tests fail to compile. These are set up to all deliberately fail to build in the clang-cl pipeline after-the-fact, but the failures can be examined in that build pipeline's log. It's possible to merge this PR and simply skip those tests in the short-term, but if we can identify a fix in the coming week, we may as well hold off introducing a new pipeline till that's solved.

@@ -60,7 +60,7 @@ def _envoy_linkopts():
"-pagezero_size 10000",
"-image_base 100000000",
],
"@envoy//bazel:clang_cl_opt_build": [
"@envoy//bazel:windows_opt_build": [
Copy link
Contributor

Choose a reason for hiding this comment

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

This expands creation of a .pdb debuginfo symbol file to the opt build of msvc + clang, now that our rbe gcp pool has a larger working set of memory to collect that during the link phase.

@@ -54,7 +54,7 @@ def envoy_copts(repository, test = False):
repository + "//bazel:opt_build": [] if test else ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:fastbuild_build": [],
repository + "//bazel:dbg_build": ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_opt_build": [] if test else ["-Z7"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This collects debuginfo symbol info to the .obj files during the opt build of msvc + clang, only for non-test sources. For test sources, do not collect debuginfo for the hundreds of test executables. (We would build fastbuild or dbg individually to troubleshoot a crashing test.)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to the .bzl comments so this intuition is kept for posterity?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that this comment...

Simplify the amount of symbolic debug info for test binaries,
targets listed in order from generic to increasing specificity.

did exactly that?

Copy link
Member

Choose a reason for hiding this comment

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

You might be technically correct here, the best kind of correct. I think this is not really as helpful as something like "do not collect debuginfo for the hundreds of test executables. (We would build fastbuild or dbg individually to troubleshoot a crashing test.)"

Copy link
Contributor

Choose a reason for hiding this comment

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

@htuch have a look at the new iteration and let me know if that's helpful.

skip_test_tags="${skip_test_tags},-skip_on_windows_clang,-fails_on_windows_clang"
fail_test_tags="${fail_test_tags},fails_on_windows_clang"
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

This extra logic will be discarded after all compile and runtime failures unique to clang-cl have been resolved.

@@ -480,7 +480,7 @@ TEST_F(AsyncDataSourceTest, BaseIntervalGreaterThanMaxInterval) {
TestUtility::loadFromYamlAndValidate(yaml, config);
EXPECT_TRUE(config.has_remote());

EXPECT_THROW_WITH_MESSAGE(std::make_unique<Config::DataSource::RemoteAsyncDataProvider>(
EXPECT_THROW_WITH_MESSAGE((void)std::make_unique<Config::DataSource::RemoteAsyncDataProvider>(
cm_, init_manager_, config.remote(), dispatcher_, random_, true,
[&](const std::string&) {}),
EnvoyException,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a wide open question, @htuch - is there someone of the google / googletest participants who could identify why this emits an error under clang-cl, but on no other architectures (such as gcc, linux clang, and not even msvc.)

We could add the explicit flag -Wno-discard-results for clang-cl, but we hadn't done this for other architectures, and I'm wondering if that would be wise. I've fixed a couple of discarded non-expectation results, but in this EXPECT_[NO]THROW googletest pattern, it does get a little wordy.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Definitely would rather not have this void cast. I'll follow up on this offline, but one option is to build the cast explicitly into the EXPECT_THROW_WITH_MESSAGE macro, which is something we define.

Copy link
Member

Choose a reason for hiding this comment

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

LMK if this is still an issue on Clang 11.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still an issue with -Wall -Werror; however, it makes sense to keep this potential defect detection in source/... --- but it doesn't seem interesting to track discarded results in test/... sources. Moved the warning suppression to the appropriate tests-only copt logic for clang-cl builds, and left a handful of corrections for source/... --- including documentation of why the results are discarded.

@@ -52,6 +54,8 @@ envoy_cc_test(
"--runtime-feature-disable-for-tests=envoy.reloadable_features.new_codec_behavior",
],
shard_count = 5,
# TODO(envoyproxy/windows-dev): Resolve missing pure virtual destructor bug
tags = ["skip_on_windows_clang"],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I'm looking for some possible assistance from google folks, @htuch - we are having serious problems deciphering why clang-cl is failing in a manner unique to building opt (/O2) on Windows using clang-cl, and in no other environments or compilers. This particular link failure reads as follows;

ERROR: C:/users/wrowe/dev/envoy/test/common/http/http2/BUILD:40:14: Linking of rule '//test/common/http/http2:codec_impl_test' failed (Exit 1)
lld-link: error: undefined symbol: public: virtual __cdecl Envoy::Http::ConnectionCallbacks::~ConnectionCallbacks(void)
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(int `public: __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::Http2CodecImplTestFixture(class std::tuple<unsigned int, unsigned int, unsigned int, unsigned int>, class std::tuple<unsigned int, unsigned int, unsigned int, unsigned int>)'::`1'::dtor$69)
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(int `public: __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::Http2CodecImplTestFixture(class std::tuple<unsigned int, unsigned int, unsigned int, unsigned int>, class std::tuple<unsigned int, unsigned int, unsigned int, unsigned int>)'::`1'::dtor$76)
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(public: void __cdecl Envoy::Http::MockServerConnectionCallbacks::`vbase dtor'(void))
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(public: void __cdecl Envoy::Http::MockConnectionCallbacks::`vbase dtor'(void))>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(public: virtual __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::~Http2CodecImplTestFixture(void))
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(public: virtual __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::~Http2CodecImplTestFixture(void))
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(int `public: __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::Http2CodecImplTestFixture(void)'::`1'::dtor$18)
>>> referenced by bazel-out/x64_windows-opt/bin/test/common/http/http2/_objs/codec_impl_test/codec_impl_test.obj:(int `public: __cdecl Envoy::Http::Http2::Http2CodecImplTestFixture::Http2CodecImplTestFixture(void)'::`1'::dtor$25)
Target //test/common/http/http2:codec_impl_test failed to build

Any chance you can find some support for us researching what might be happening here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Quick update for @htuch and @lizan - we attempted to verify this error with the llvm 11.0.0 release of clang-cl, however that update breaks bazel. Holding for this 1-line PR to be accepted to allow us to pick up the new clang-cl, and confirm whether the bug remains or has been resolved.

bazelbuild/rules_cc#88

/wait

Copy link
Contributor

Choose a reason for hiding this comment

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

This does appear to be resolved in clang 11.0.0 release.

@wrowe wrowe assigned htuch and unassigned wrowe Oct 30, 2020
@sunjayBhatia sunjayBhatia changed the title Draft: Windows: Add clang cl build Windows: Add clang-cl pipeline Oct 30, 2020
@sunjayBhatia sunjayBhatia marked this pull request as ready for review October 30, 2020 20:33
Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe
Copy link
Contributor

wrowe commented Nov 3, 2020

Currently stalled to pick up clang-cl 11 and reproduce the linkage error. See bazelbuild/rules_cc#88

- per htuch feedback keep these expectation simple
- macros can be enhanced, but discard vs non-discard flavors would be required
- work around the error messages for clang-cl only, and clean up c[xx]opt formatting

Signed-off-by: William A Rowe Jr <[email protected]>
- This activates a few tests observed to fail locally, to confirm
behavior in the CI pipeline when the clang-cl pipeline is added next
week.

Co-authored-by: Sunjay Bhatia <[email protected]>
Signed-off-by: Sunjay Bhatia <[email protected]>
Co-authored-by: William A Rowe Jr <[email protected]>
Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe
Copy link
Contributor

wrowe commented Nov 20, 2020

Apologies, had to force-push the DCO I'd missed in the first push.

Signed-off-by: William A Rowe Jr <[email protected]>
@wrowe
Copy link
Contributor

wrowe commented Nov 21, 2020

This is, finally, ready for final review and adoption ahead of integrating LLVM clang-cl in 11.0.0 (#14135).

We will need to watch in the coming days and mark any newfound exceptions promptly as "flaky_on_windows", but there are future patches in the pipeline to automate this. I'd only ask that we note which specific test(s) fail (or timeout) to make quick progress on diagnosing the last few flakes.

@@ -326,6 +326,12 @@ build:clang-cl --define clang_cl=1
# Override determinism flags (DATE etc) is valid on clang-cl compiler
build:clang-cl --copt="-Wno-macro-redefined"
build:clang-cl --copt="-Wno-builtin-macro-redefined"
# Workaround problematic missing override declarations of mocks
# TODO: resolve this class of problematic mocks, e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Is the plan to do this in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear whether 11.0.0 is going to fix this for us, whether we can fix this in a broad stroke, or whether every mock declaration affected must be updated and was missed in the last googletest update. This is why it was left unassigned.

Copy link
Member

Choose a reason for hiding this comment

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

Can you file a tracking issue?

@@ -54,7 +54,7 @@ def envoy_copts(repository, test = False):
repository + "//bazel:opt_build": [] if test else ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:fastbuild_build": [],
repository + "//bazel:dbg_build": ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_opt_build": [] if test else ["-Z7"],
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this to the .bzl comments so this intuition is kept for posterity?

test/common/network/socket_option_test.h Show resolved Hide resolved
{"+", "%2B"},
{"/", "%2F"},
{"=", "%3D"}});
std::string urlencoded = absl::StrReplaceAll(
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to split the test and build changes where possible, upstream these tests changes in a dedicated PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a ridiculously small number of source changes here, which are all easily understood from the primary commit context. If you insist, I'll split it.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree with the definition of ridiculously small ;-) I'm not going to insist, but as a reviewer I think it makes life easier to split these concerns usually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've commented above on the one //source/... case we should consider peeling away from this patch, everything else is test or build.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are no build changes that don't depend on the test changes, and vis-a-versa. So there would be no means to pass CI, so I'm really unclear on your ask.

@@ -342,8 +342,8 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path,
}

std::string TestEnvironment::readFileToStringForTest(const std::string& filename,
bool require_existence) {
std::ifstream file(filename, std::ios::binary);
bool require_existence, bool read_binary) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little worried about this proliferating in options; should we have a struct for options here? We can keep the existing signature for backwards compatibility as a wrapper to avoid changing the entire world.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't envision additional flags, so a struct seems like overkill.

Copy link
Member

Choose a reason for hiding this comment

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

Arguably it makes the tests that have been modified easier to read (should the default be true or false?), but I don't feel that strongly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally a fan of bitwise flags, and was annoyed as we wrote this that named args aren't a c++ thing, so I'll rewrite it with enums if you insist. But I'd like a use case that suggests a third toggle before we bother.

wrowe
wrowe previously approved these changes Nov 23, 2020
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

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

This appears complete to me. @lizan has identified issues with our coverage tests which suggest this will not be workable as bazel 3.7.1 lands, so there is less urgency and time for further discussion, @htuch.

@@ -326,6 +326,12 @@ build:clang-cl --define clang_cl=1
# Override determinism flags (DATE etc) is valid on clang-cl compiler
build:clang-cl --copt="-Wno-macro-redefined"
build:clang-cl --copt="-Wno-builtin-macro-redefined"
# Workaround problematic missing override declarations of mocks
# TODO: resolve this class of problematic mocks, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unclear whether 11.0.0 is going to fix this for us, whether we can fix this in a broad stroke, or whether every mock declaration affected must be updated and was missed in the last googletest update. This is why it was left unassigned.

@@ -54,7 +54,7 @@ def envoy_copts(repository, test = False):
repository + "//bazel:opt_build": [] if test else ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:fastbuild_build": [],
repository + "//bazel:dbg_build": ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_opt_build": [] if test else ["-Z7"],
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that this comment...

Simplify the amount of symbolic debug info for test binaries,
targets listed in order from generic to increasing specificity.

did exactly that?

test/common/network/socket_option_test.h Show resolved Hide resolved
{"+", "%2B"},
{"/", "%2F"},
{"=", "%3D"}});
std::string urlencoded = absl::StrReplaceAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a ridiculously small number of source changes here, which are all easily understood from the primary commit context. If you insist, I'll split it.

@@ -326,6 +326,12 @@ build:clang-cl --define clang_cl=1
# Override determinism flags (DATE etc) is valid on clang-cl compiler
build:clang-cl --copt="-Wno-macro-redefined"
build:clang-cl --copt="-Wno-builtin-macro-redefined"
# Workaround problematic missing override declarations of mocks
# TODO: resolve this class of problematic mocks, e.g.
Copy link
Member

Choose a reason for hiding this comment

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

Can you file a tracking issue?

@@ -54,7 +54,7 @@ def envoy_copts(repository, test = False):
repository + "//bazel:opt_build": [] if test else ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:fastbuild_build": [],
repository + "//bazel:dbg_build": ["-ggdb3", "-gsplit-dwarf"],
repository + "//bazel:windows_opt_build": [],
repository + "//bazel:windows_opt_build": [] if test else ["-Z7"],
Copy link
Member

Choose a reason for hiding this comment

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

You might be technically correct here, the best kind of correct. I think this is not really as helpful as something like "do not collect debuginfo for the hundreds of test executables. (We would build fastbuild or dbg individually to troubleshoot a crashing test.)"

{"+", "%2B"},
{"/", "%2F"},
{"=", "%3D"}});
std::string urlencoded = absl::StrReplaceAll(
Copy link
Member

Choose a reason for hiding this comment

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

I disagree with the definition of ridiculously small ;-) I'm not going to insist, but as a reviewer I think it makes life easier to split these concerns usually.

@@ -342,8 +342,8 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path,
}

std::string TestEnvironment::readFileToStringForTest(const std::string& filename,
bool require_existence) {
std::ifstream file(filename, std::ios::binary);
bool require_existence, bool read_binary) {
Copy link
Member

Choose a reason for hiding this comment

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

Arguably it makes the tests that have been modified easier to read (should the default be true or false?), but I don't feel that strongly.

test/common/network/cidr_range_test.cc Show resolved Hide resolved
std::next(it, rand_idx);
host = Upstream::HostSharedPtr{new RedisHost(parent_.info(), "", *it, parent_, true)};
host = Upstream::HostSharedPtr{
new RedisHost(parent_.info(), "", *std::next(it, rand_idx), parent_, true)};
Copy link
Contributor

Choose a reason for hiding this comment

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

@htuch - this is the only non-build, non-test change in this entire patchset. Would you like me to break it out for you as an independently reviewed PR, or are we ok to merge and iterate at this point? It's the remaining example of a discarded no-op result in the //source/... since I re-merged.

Copy link
Member

Choose a reason for hiding this comment

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

No, I was not asking for this. I was pointing out that the test changes alone could have been upstreamed independently. Anyway, NBD.

htuch
htuch previously approved these changes Nov 24, 2020
@htuch
Copy link
Member

htuch commented Dec 1, 2020

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Dec 1, 2020
@htuch htuch merged commit 2745d66 into envoyproxy:master Dec 1, 2020
@sunjayBhatia sunjayBhatia deleted the add-clang-cl-build branch December 1, 2020 15:46
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 2, 2020
* master: (70 commits)
  upstream: avoid reset after end_stream in TCP HTTP upstream (envoyproxy#14106)
  bazelci: add fuzz coverage (envoyproxy#14179)
  dependencies: allowlist CVE-2020-8277 to prevent false positives. (envoyproxy#14228)
  cleanup: replace ad-hoc [0, 1] value types with UnitFloat (envoyproxy#14081)
  Update docs for skywalking tracer (envoyproxy#14210)
  Fix some errors in the switch statement when decode dubbo response (envoyproxy#14207)
  Windows: enable tests and envoy-static.exe pdb file (envoyproxy#13688)
  http: add Kill Request HTTP filter (envoyproxy#14170)
  dependencies: fix release_dates error behavior. (envoyproxy#14216)
  thrift filter: support skip decoding data after metadata in the thrift message (envoyproxy#13592)
  update cares (envoyproxy#14213)
  docs: clarify behavior of hedge_on_per_try_timeout (envoyproxy#12983)
  repokitteh: add support for randomized auto-assign. (envoyproxy#14185)
  [grpc] validate grpc config for illegal characters (envoyproxy#14129)
  server: Return nullopt when process_context is nullptr (envoyproxy#14181)
  [Windows] Fix thrift proxy tests (envoyproxy#13220)
  kafka: add missing unit tests (envoyproxy#14195)
  doc: mention gperftools explicitly in PPROF.md (envoyproxy#14199)
  Removed `--use-fake-symbol-table` option. (envoyproxy#14178)
  filter contract: clarification around local replies (envoyproxy#14193)
  ...

Signed-off-by: Michael Puncel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants