From c1f0dba7452306932f0054a7a30f29d6abc3966a Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 22 Aug 2018 16:55:22 -0400 Subject: [PATCH] Add check_format for instantiating time-sources in a new source file (tests excluded). Signed-off-by: Joshua Marantz --- tools/check_format.py | 18 ++++++++++++++++++ tools/check_format_test_helper.py | 8 ++++++++ .../check_format/prod_monotonic_time.cc | 4 ++++ .../testdata/check_format/prod_system_time.cc | 3 +++ 4 files changed, 33 insertions(+) create mode 100644 tools/testdata/check_format/prod_monotonic_time.cc create mode 100644 tools/testdata/check_format/prod_system_time.cc diff --git a/tools/check_format.py b/tools/check_format.py index c9e8fc243535..6f643cd6b7f9 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -23,6 +23,15 @@ GOOGLE_PROTOBUF_WHITELIST = ("ci/prebuilt", "source/common/protobuf", "api/test") REPOSITORIES_BZL = "bazel/repositories.bzl" +# Files matching these exact names can reference prod time. These include the class +# definitions for prod time, the construction of them in main(), and perf annotation. +# For now it includes the validation server but that really should be injected too. +PROD_TIME_WHITELIST = ('./source/common/common/utility.h', + './source/exe/main_common.cc', + './source/exe/main_common.h', + './source/server/config_validation/server.cc', + './source/common/common/perf_annotation.h') + CLANG_FORMAT_PATH = os.getenv("CLANG_FORMAT", "clang-format-6.0") BUILDIFIER_PATH = os.getenv("BUILDIFIER_BIN", "$GOPATH/bin/buildifier") ENVOY_BUILD_FIXER_PATH = os.path.join( @@ -65,6 +74,12 @@ def whitelistedForProtobufDeps(file_path): return (file_path.endswith(PROTO_SUFFIX) or file_path.endswith(REPOSITORIES_BZL) or \ any(path_segment in file_path for path_segment in GOOGLE_PROTOBUF_WHITELIST)) +# Production time sources should not be instantiated in the source, except for a few +# specific cases. They should be passed down from where they are instantied to where +# they need to be used, e.g. through the ServerInstance, Dispatcher, or ClusterManager. +def whitelistedForProdTime(file_path): + return file_path in PROD_TIME_WHITELIST or file_path.startswith('./test/') + def findSubstringAndReturnError(pattern, file_path, error_message): with open(file_path) as f: text = f.read() @@ -145,6 +160,9 @@ def checkSourceLine(line, file_path, reportError): # comments, for example this one. reportError("Don't use or , switch to " "Thread::MutexBasicLockable in source/common/common/thread.h") + if not whitelistedForProdTime(file_path): + if 'ProdSystemTimeSource' in line or 'ProdMonotonicTimeSource' in line: + reportError("Don't reference real-time sources from production code; use injection") def checkBuildLine(line, file_path, reportError): if not whitelistedForProtobufDeps(file_path) and '"protobuf"' in line: diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 17ad7edf9f58..2d7b02081a34 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -143,6 +143,10 @@ def checkFileExpectingOK(filename): "Don't use or ") errors += fixFileExpectingFailure("condition_variable_any.cc", "Don't use or ") + errors += fixFileExpectingFailure( + "prod_system_time.cc", "Don't reference real-time sources from production code; use injection") + errors += fixFileExpectingFailure( + "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection") errors += fixFileExpectingNoChange("ok_file.cc") @@ -163,6 +167,10 @@ def checkFileExpectingOK(filename): errors += checkFileExpectingError("bad_envoy_build_sys_ref.BUILD", "Superfluous '@envoy//' prefix") errors += checkFileExpectingError("proto_format.proto", "clang-format check failed") + errors += checkFileExpectingError( + "prod_system_time.cc", "Don't reference real-time sources from production code; use injection") + errors += checkFileExpectingError( + "prod_monotonic_time.cc", "Don't reference real-time sources from production code; use injection") errors += checkFileExpectingOK("ok_file.cc") diff --git a/tools/testdata/check_format/prod_monotonic_time.cc b/tools/testdata/check_format/prod_monotonic_time.cc new file mode 100644 index 000000000000..10bcfc7c5d3e --- /dev/null +++ b/tools/testdata/check_format/prod_monotonic_time.cc @@ -0,0 +1,4 @@ +int foo() { + ProdSystemTimeSource system_time; + ProdMonotonicTimeSource monotonic_time; +} diff --git a/tools/testdata/check_format/prod_system_time.cc b/tools/testdata/check_format/prod_system_time.cc new file mode 100644 index 000000000000..a533661b9805 --- /dev/null +++ b/tools/testdata/check_format/prod_system_time.cc @@ -0,0 +1,3 @@ +int foo() { + ProdMonotonicTimeSource monotonic_time; +}