Skip to content

Disable quic_platform_test by default#5791

Closed
wu-bin wants to merge 1 commit intoenvoyproxy:masterfrom
wu-bin:nobuildquiche
Closed

Disable quic_platform_test by default#5791
wu-bin wants to merge 1 commit intoenvoyproxy:masterfrom
wu-bin:nobuildquiche

Conversation

@wu-bin
Copy link
Copy Markdown
Contributor

@wu-bin wu-bin commented Jan 31, 2019

Description:

Disable quic_platform_test by default. Enable it only when "--define enable_quiche=enabled" is specified in the bazel command line.

This is needed to workaround a issue with --experimental_remap_main_repo + ci asan build, see here for details.

Risk Level: minimal: build only
Testing:

I introduced a failure in quic_platform_test, then confirmed quic_platform_test still passes by default(since it does nothing), and failed when "--define enable_quiche=enabled" is specified.

Also ran "./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.asan'" to confirm the ci issue is gone.

Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

enable_quiche=enabled" is specified in the bazel command line.

Signed-off-by: Bin Wu <wub@google.com>
@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Jan 31, 2019

/assign @mpwarres
@htuch


config_setting(
name = "enable_quiche",
values = {"define": "enable_quiche=enabled"},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"enable_quiche=enabled" sounds a little redundant. How about just "quiche=enabled"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in ##5838

envoy_cc_test(
name = "quic_platform_test",
srcs = ["quic_platform_test.cc"],
srcs = envoy_select_quiche(["quic_platform_test.cc"]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@htuch , should we also be applying this to non-test build rules (e.g. those under source/extensions/quic_listeners/quiche/...), or does the omission of QUICHE from source/extensions/extensions_build_config.bzl already suffice for that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the latter should suffice, but worth validating.

envoy_cc_test(
name = "quic_platform_test",
srcs = ["quic_platform_test.cc"],
srcs = envoy_select_quiche(["quic_platform_test.cc"]),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you confirm that if you make a typo to the Quiche implementation that bazel build //source/exe/... works? Also, CI is broken. Other than that, this looks like a good approach, thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any idea why ci is broken?

bazel build //source/exe/... works on my machine with the diff below. Actually, even bazel build //source/extensions/... worked, which is unexpected to me.

$ git diff
diff --git a/source/extensions/quic_listeners/quiche/platform/quic_string_impl.h b/source/extensions/quic_listeners/quiche/platform/quic_string_impl.h
index 9370a02b1..68bf14b4a 100644
--- a/source/extensions/quic_listeners/quiche/platform/quic_string_impl.h
+++ b/source/extensions/quic_listeners/quiche/platform/quic_string_impl.h
@@ -1,4 +1,4 @@
-#pragma once
+fdsfdsfdsfdsfds#pragma once

#include

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is weird, it looks like we are hitting CI resource limits again, gcc is likely OOMing. What's disturbing is:

Resource class xlarge for docker is not available for your project. Default class medium will be used. Please contact your CSM person or our support team to whitelist your project.

at https://circleci.com/gh/wu-bin/envoy/37?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

The CI is running on your fork, but https://circleci.com/gh/envoyproxy/envoy/158261?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link in #5801 isn't.

@lizan do you know what might be happening? I'm suspecting some git flow issue.

@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Feb 1, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5791 (comment) was created by @wu-bin.

see: more, trace.

@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Feb 1, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5791 (comment) was created by @wu-bin.

see: more, trace.

@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Feb 4, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🙀 failed invoking rebuild of ci/circleci: coverage: 503 Service Unavailable
🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)

🐱

Caused by: a #5791 (comment) was created by @wu-bin.

see: more, trace.

@wu-bin
Copy link
Copy Markdown
Contributor Author

wu-bin commented Feb 4, 2019

ci keeps fail for this PR, I've re-created #5838 to replace this. Sorry for the churn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants