lint: Make command line arguments accessible to helper functions

[LisaLudique]

Commit Message: Make command line arguments accessible to helper functions in formatting script
Additional Description: The formatting script does not recognize the variables from the command line arguments when running Python >=3.8 on MacOS due to changes in multiprocessing behavior in the new Python version (https://bugs.python.org/issue39931). This change passes in the command line arguments so they are accessible by the helper functions in which they are needed.
Risk Level: Low
Testing: Existing unit tests
Docs Changes: N/A
Release Notes: N/A
Fixes #12740

[htuch]

Thanks for the fix, one question on a possibly cleaner alternative.

[htuch]

What do you think about making this a class and have command line args be a member variable? That could reduce the amount of threading required.

[LisaLudique]

This sounds like a better approach. I'll get to this this week.

[LisaLudique]

Sorry for the delay, this is now ready for another look

[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!

[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!

[htuch]

This is really nice, thanks so much for the refactor, this will be way more maintainable! QQ on what changes have happened, it's kind of hard to understand due to GitHub's diff view being linear.

[htuch]

Thanks for this update. For review benefit, are there any other sites that aren't refactors or CLI plumbing related?

[LisaLudique]

No worries, the diff is hard to read.

I defined a FormatChecker class and moved all the functions in

envoy/tools/code_format/check_format.py

Lines 186 to 1030 in 130c7c4

	# Map a line transformation function across each line of a file,
	# writing the result lines as requested.
	# If there is a clang format nesting or mismatch error, return the first occurrence
	def evaluateLines(path, line_xform, write=True):
	error_message = None
	format_flag = True
	output_lines = []
	for line_number, line in enumerate(readLines(path)):
	if line.find("// clang-format off") != -1:
	if not format_flag and error_message is None:
	error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested off")
	format_flag = False
	if line.find("// clang-format on") != -1:
	if format_flag and error_message is None:
	error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested on")
	format_flag = True
	if format_flag:
	output_lines.append(line_xform(line, line_number))
	else:
	output_lines.append(line)
	# We used to use fileinput in the older Python 2.7 script, but this doesn't do
	# inplace mode and UTF-8 in Python 3, so doing it the manual way.
	if write:
	pathlib.Path(path).write_text('\n'.join(output_lines), encoding='utf-8')
	if not format_flag and error_message is None:
	error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format remains off")
	return error_message
	# Obtain all the lines in a given file.
	def readLines(path):
	return readFile(path).split('\n')
	# Read a UTF-8 encoded file as a str.
	def readFile(path):
	return pathlib.Path(path).read_text(encoding='utf-8')
	# lookPath searches for the given executable in all directories in PATH
	# environment variable. If it cannot be found, empty string is returned.
	def lookPath(executable):
	return shutil.which(executable) or ''
	# pathExists checks whether the given path exists. This function assumes that
	# the path is absolute and evaluates environment variables.
	def pathExists(executable):
	return os.path.exists(os.path.expandvars(executable))
	# executableByOthers checks whether the given path has execute permission for
	# others.
	def executableByOthers(executable):
	st = os.stat(os.path.expandvars(executable))
	return bool(st.st_mode & stat.S_IXOTH)
	# Check whether all needed external tools (clang-format, buildifier, buildozer) are
	# available.
	def checkTools():
	error_messages = []
	clang_format_abs_path = lookPath(CLANG_FORMAT_PATH)
	if clang_format_abs_path:
	if not executableByOthers(clang_format_abs_path):
	error_messages.append("command {} exists, but cannot be executed by other "
	"users".format(CLANG_FORMAT_PATH))
	else:
	error_messages.append(
	"Command {} not found. If you have clang-format in version 10.x.x "
	"installed, but the binary name is different or it's not available in "
	"PATH, please use CLANG_FORMAT environment variable to specify the path. "
	"Examples:\n"
	" export CLANG_FORMAT=clang-format-10.0.0\n"
	" export CLANG_FORMAT=/opt/bin/clang-format-10\n"
	" export CLANG_FORMAT=/usr/local/opt/llvm@10/bin/clang-format".format(CLANG_FORMAT_PATH))
	def checkBazelTool(name, path, var):
	bazel_tool_abs_path = lookPath(path)
	if bazel_tool_abs_path:
	if not executableByOthers(bazel_tool_abs_path):
	error_messages.append("command {} exists, but cannot be executed by other "
	"users".format(path))
	elif pathExists(path):
	if not executableByOthers(path):
	error_messages.append("command {} exists, but cannot be executed by other "
	"users".format(path))
	else:
	error_messages.append("Command {} not found. If you have {} installed, but the binary "
	"name is different or it's not available in $GOPATH/bin, please use "
	"{} environment variable to specify the path. Example:\n"
	" export {}=`which {}`\n"
	"If you don't have {} installed, you can install it by:\n"
	" go get -u github.com/bazelbuild/buildtools/{}".format(
	path, name, var, var, name, name, name))
	checkBazelTool('buildifier', BUILDIFIER_PATH, 'BUILDIFIER_BIN')
	checkBazelTool('buildozer', BUILDOZER_PATH, 'BUILDOZER_BIN')
	return error_messages
	def checkNamespace(file_path):
	for excluded_path in namespace_check_excluded_paths:
	if file_path.startswith(excluded_path):
	return []
	nolint = "NOLINT(namespace-%s)" % namespace_check.lower()
	text = readFile(file_path)
	if not re.search("^\s*namespace\s+%s\s*{" % namespace_check, text, re.MULTILINE) and \
	not nolint in text:
	return ["Unable to find %s namespace or %s for file: %s" % (namespace_check, nolint, file_path)]
	return []
	def packageNameForProto(file_path):
	package_name = None
	error_message = []
	result = PROTO_PACKAGE_REGEX.search(readFile(file_path))
	if result is not None and len(result.groups()) == 1:
	package_name = result.group(1)
	if package_name is None:
	error_message = ["Unable to find package name for proto file: %s" % file_path]
	return [package_name, error_message]
	# To avoid breaking the Lyft import, we just check for path inclusion here.
	def allowlistedForProtobufDeps(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_ALLOWLIST))
	# Real-world 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 allowlistedForRealTime(file_path):
	if file_path.endswith(".md"):
	return True
	return file_path in REAL_TIME_ALLOWLIST
	def allowlistedForRegisterFactory(file_path):
	if not file_path.startswith("./test/"):
	return True
	return any(file_path.startswith(prefix) for prefix in REGISTER_FACTORY_TEST_ALLOWLIST)
	def allowlistedForSerializeAsString(file_path):
	return file_path in SERIALIZE_AS_STRING_ALLOWLIST or file_path.endswith(DOCS_SUFFIX)
	def allowlistedForJsonStringToMessage(file_path):
	return file_path in JSON_STRING_TO_MESSAGE_ALLOWLIST
	def allowlistedForHistogramSiSuffix(name):
	return name in HISTOGRAM_WITH_SI_SUFFIX_ALLOWLIST
	def allowlistedForStdRegex(file_path):
	return file_path.startswith("./test") or file_path in STD_REGEX_ALLOWLIST or file_path.endswith(
	DOCS_SUFFIX)
	def allowlistedForGrpcInit(file_path):
	return file_path in GRPC_INIT_ALLOWLIST
	def allowlistedForUnpackTo(file_path):
	return file_path.startswith("./test") or file_path in [
	"./source/common/protobuf/utility.cc", "./source/common/protobuf/utility.h"
	]
	def denylistedForExceptions(file_path):
	# Returns true when it is a non test header file or the file_path is in DENYLIST or
	# it is under toos/testdata subdirectory.
	if file_path.endswith(DOCS_SUFFIX):
	return False
	return (file_path.endswith('.h') and not file_path.startswith("./test/")) or file_path in EXCEPTION_DENYLIST \
	or isInSubdir(file_path, 'tools/testdata')
	def findSubstringAndReturnError(pattern, file_path, error_message):
	text = readFile(file_path)
	if pattern in text:
	error_messages = [file_path + ": " + error_message]
	for i, line in enumerate(text.splitlines()):
	if pattern in line:
	error_messages.append(" %s:%s" % (file_path, i + 1))
	return error_messages
	return []
	def errorIfNoSubstringFound(pattern, file_path, error_message):
	return [] if pattern in readFile(file_path) else [file_path + ": " + error_message]
	def isApiFile(file_path):
	return file_path.startswith(args.api_prefix) or file_path.startswith(args.api_shadow_prefix)
	def isBuildFile(file_path):
	basename = os.path.basename(file_path)
	if basename in {"BUILD", "BUILD.bazel"} or basename.endswith(".BUILD"):
	return True
	return False
	def isExternalBuildFile(file_path):
	return isBuildFile(file_path) and (file_path.startswith("./bazel/external/") or
	file_path.startswith("./tools/clang_tools"))
	def isStarlarkFile(file_path):
	return file_path.endswith(".bzl")
	def isWorkspaceFile(file_path):
	return os.path.basename(file_path) == "WORKSPACE"
	def isBuildFixerExcludedFile(file_path):
	for excluded_path in build_fixer_check_excluded_paths:
	if file_path.startswith(excluded_path):
	return True
	return False
	def hasInvalidAngleBracketDirectory(line):
	if not line.startswith(INCLUDE_ANGLE):
	return False
	path = line[INCLUDE_ANGLE_LEN:]
	slash = path.find("/")
	if slash == -1:
	return False
	subdir = path[0:slash]
	return subdir in SUBDIR_SET
	VERSION_HISTORY_NEW_LINE_REGEX = re.compile("\* ([a-z \-_]+): ([a-z:`]+)")
	VERSION_HISTORY_SECTION_NAME = re.compile("^[A-Z][A-Za-z ]*$")
	RELOADABLE_FLAG_REGEX = re.compile(".*(.)(envoy.reloadable_features.[^ ]*)\s.*")
	# Check for punctuation in a terminal ref clause, e.g.
	# :ref:`panic mode. <arch_overview_load_balancing_panic_threshold>`
	REF_WITH_PUNCTUATION_REGEX = re.compile(".*\. <[^<]*>`\s*")
	def checkCurrentReleaseNotes(file_path, error_messages):
	first_word_of_prior_line = ''
	next_word_to_check = '' # first word after :
	prior_line = ''
	def endsWithPeriod(prior_line):
	if not prior_line:
	return True # Don't punctuation-check empty lines.
	if prior_line.endswith('.'):
	return True # Actually ends with .
	if prior_line.endswith('`') and REF_WITH_PUNCTUATION_REGEX.match(prior_line):
	return True # The text in the :ref ends with a .
	return False
	for line_number, line in enumerate(readLines(file_path)):
	def reportError(message):
	error_messages.append("%s:%d: %s" % (file_path, line_number + 1, message))
	if VERSION_HISTORY_SECTION_NAME.match(line):
	if line == "Deprecated":
	# The deprecations section is last, and does not have enforced formatting.
	break
	# Reset all parsing at the start of a section.
	first_word_of_prior_line = ''
	next_word_to_check = '' # first word after :
	prior_line = ''
	# make sure flags are surrounded by ``s
	flag_match = RELOADABLE_FLAG_REGEX.match(line)
	if flag_match:
	if not flag_match.groups()[0].startswith('`'):
	reportError("Flag `%s` should be enclosed in back ticks" % flag_match.groups()[1])
	if line.startswith("* "):
	if not endsWithPeriod(prior_line):
	reportError("The following release note does not end with a '.'\n %s" % prior_line)
	match = VERSION_HISTORY_NEW_LINE_REGEX.match(line)
	if not match:
	reportError("Version history line malformed. "
	"Does not match VERSION_HISTORY_NEW_LINE_REGEX in check_format.py\n %s" % line)
	else:
	first_word = match.groups()[0]
	next_word = match.groups()[1]
	# Do basic alphabetization checks of the first word on the line and the
	# first word after the :
	if first_word_of_prior_line and first_word_of_prior_line > first_word:
	reportError(
	"Version history not in alphabetical order (%s vs %s): please check placement of line\n %s. "
	% (first_word_of_prior_line, first_word, line))
	if first_word_of_prior_line == first_word and next_word_to_check and next_word_to_check > next_word:
	reportError(
	"Version history not in alphabetical order (%s vs %s): please check placement of line\n %s. "
	% (next_word_to_check, next_word, line))
	first_word_of_prior_line = first_word
	next_word_to_check = next_word
	prior_line = line
	elif not line:
	# If we hit the end of this release note block block, check the prior line.
	if not endsWithPeriod(prior_line):
	reportError("The following release note does not end with a '.'\n %s" % prior_line)
	elif prior_line:
	prior_line += line
	def checkFileContents(file_path, checker):
	error_messages = []
	if file_path.endswith("version_history/current.rst"):
	# Version file checking has enough special cased logic to merit its own checks.
	# This only validates entries for the current release as very old release
	# notes have a different format.
	checkCurrentReleaseNotes(file_path, error_messages)
	def checkFormatErrors(line, line_number):
	def reportError(message):
	error_messages.append("%s:%d: %s" % (file_path, line_number + 1, message))
	checker(line, file_path, reportError)
	evaluate_failure = evaluateLines(file_path, checkFormatErrors, False)
	if evaluate_failure is not None:
	error_messages.append(evaluate_failure)
	return error_messages
	DOT_MULTI_SPACE_REGEX = re.compile("\\. +")
	def fixSourceLine(line, line_number):
	# Strip double space after '.' This may prove overenthusiastic and need to
	# be restricted to comments and metadata files but works for now.
	line = re.sub(DOT_MULTI_SPACE_REGEX, ". ", line)
	if hasInvalidAngleBracketDirectory(line):
	line = line.replace("<", '"').replace(">", '"')
	# Fix incorrect protobuf namespace references.
	for invalid_construct, valid_construct in PROTOBUF_TYPE_ERRORS.items():
	line = line.replace(invalid_construct, valid_construct)
	# Use recommended cpp stdlib
	for invalid_construct, valid_construct in LIBCXX_REPLACEMENTS.items():
	line = line.replace(invalid_construct, valid_construct)
	return line
	# We want to look for a call to condvar.waitFor, but there's no strong pattern
	# to the variable name of the condvar. If we just look for ".waitFor" we'll also
	# pick up time_system_.waitFor(...), and we don't want to return true for that
	# pattern. But in that case there is a strong pattern of using time_system in
	# various spellings as the variable name.
	def hasCondVarWaitFor(line):
	wait_for = line.find(".waitFor(")
	if wait_for == -1:
	return False
	preceding = line[0:wait_for]
	if preceding.endswith("time_system") or preceding.endswith("timeSystem()") or \
	preceding.endswith("time_system_"):
	return False
	return True
	# Determines whether the filename is either in the specified subdirectory, or
	# at the top level. We consider files in the top level for the benefit of
	# the check_format testcases in tools/testdata/check_format.
	def isInSubdir(filename, *subdirs):
	# Skip this check for check_format's unit-tests.
	if filename.count("/") <= 1:
	return True
	for subdir in subdirs:
	if filename.startswith('./' + subdir + '/'):
	return True
	return False
	# Determines if given token exists in line without leading or trailing token characters
	# e.g. will return True for a line containing foo() but not foo_bar() or baz_foo
	def tokenInLine(token, line):
	index = 0
	while True:
	index = line.find(token, index)
	# the following check has been changed from index < 1 to index < 0 because
	# this function incorrectly returns false when the token in question is the
	# first one in a line. The following line returns false when the token is present:
	# (no leading whitespace) violating_symbol foo;
	if index < 0:
	break
	if index == 0 or not (line[index - 1].isalnum() or line[index - 1] == '_'):
	if index + len(token) >= len(line) or not (line[index + len(token)].isalnum() or
	line[index + len(token)] == '_'):
	return True
	index = index + 1
	return False
	def checkSourceLine(line, file_path, reportError):
	# Check fixable errors. These may have been fixed already.
	if line.find(". ") != -1:
	reportError("over-enthusiastic spaces")
	if isInSubdir(file_path, 'source', 'include') and X_ENVOY_USED_DIRECTLY_REGEX.match(line):
	reportError(
	"Please do not use the raw literal x-envoy in source code. See Envoy::Http::PrefixValue.")
	if hasInvalidAngleBracketDirectory(line):
	reportError("envoy includes should not have angle brackets")
	for invalid_construct, valid_construct in PROTOBUF_TYPE_ERRORS.items():
	if invalid_construct in line:
	reportError("incorrect protobuf type reference %s; "
	"should be %s" % (invalid_construct, valid_construct))
	for invalid_construct, valid_construct in LIBCXX_REPLACEMENTS.items():
	if invalid_construct in line:
	reportError("term %s should be replaced with standard library term %s" %
	(invalid_construct, valid_construct))
	# Do not include the virtual_includes headers.
	if re.search("#include.*/_virtual_includes/", line):
	reportError("Don't include the virtual includes headers.")
	# Some errors cannot be fixed automatically, and actionable, consistent,
	# navigable messages should be emitted to make it easy to find and fix
	# the errors by hand.
	if not allowlistedForProtobufDeps(file_path):
	if '"google/protobuf' in line or "google::protobuf" in line:
	reportError("unexpected direct dependency on google.protobuf, use "
	"the definitions in common/protobuf/protobuf.h instead.")
	if line.startswith("#include <mutex>") or line.startswith("#include <condition_variable"):
	# We don't check here for std::mutex because that may legitimately show up in
	# comments, for example this one.
	reportError("Don't use <mutex> or <condition_variable*>, switch to "
	"Thread::MutexBasicLockable in source/common/common/thread.h")
	if line.startswith("#include <shared_mutex>"):
	# We don't check here for std::shared_timed_mutex because that may
	# legitimately show up in comments, for example this one.
	reportError("Don't use <shared_mutex>, use absl::Mutex for reader/writer locks.")
	if not allowlistedForRealTime(file_path) and not "NO_CHECK_FORMAT(real_time)" in line:
	if "RealTimeSource" in line or \
	("RealTimeSystem" in line and not "TestRealTimeSystem" in line) or \
	"std::chrono::system_clock::now" in line or "std::chrono::steady_clock::now" in line or \
	"std::this_thread::sleep_for" in line or hasCondVarWaitFor(line):
	reportError("Don't reference real-world time sources from production code; use injection")
	duration_arg = DURATION_VALUE_REGEX.search(line)
	if duration_arg and duration_arg.group(1) != "0" and duration_arg.group(1) != "0.0":
	# Matching duration(int-const or float-const) other than zero
	reportError(
	"Don't use ambiguous duration(value), use an explicit duration type, e.g. Event::TimeSystem::Milliseconds(value)"
	)
	if not allowlistedForRegisterFactory(file_path):
	if "Registry::RegisterFactory<" in line or "REGISTER_FACTORY" in line:
	reportError("Don't use Registry::RegisterFactory or REGISTER_FACTORY in tests, "
	"use Registry::InjectFactory instead.")
	if not allowlistedForUnpackTo(file_path):
	if "UnpackTo" in line:
	reportError("Don't use UnpackTo() directly, use MessageUtil::unpackTo() instead")
	# Check that we use the absl::Time library
	if tokenInLine("std::get_time", line):
	if "test/" in file_path:
	reportError("Don't use std::get_time; use TestUtility::parseTime in tests")
	else:
	reportError("Don't use std::get_time; use the injectable time system")
	if tokenInLine("std::put_time", line):
	reportError("Don't use std::put_time; use absl::Time equivalent instead")
	if tokenInLine("gmtime", line):
	reportError("Don't use gmtime; use absl::Time equivalent instead")
	if tokenInLine("mktime", line):
	reportError("Don't use mktime; use absl::Time equivalent instead")
	if tokenInLine("localtime", line):
	reportError("Don't use localtime; use absl::Time equivalent instead")
	if tokenInLine("strftime", line):
	reportError("Don't use strftime; use absl::FormatTime instead")
	if tokenInLine("strptime", line):
	reportError("Don't use strptime; use absl::FormatTime instead")
	if tokenInLine("strerror", line):
	reportError("Don't use strerror; use Envoy::errorDetails instead")
	# Prefer using abseil hash maps/sets over std::unordered_map/set for performance optimizations and
	# non-deterministic iteration order that exposes faulty assertions.
	# See: https://abseil.io/docs/cpp/guides/container#hash-tables
	if "std::unordered_map" in line:
	reportError("Don't use std::unordered_map; use absl::flat_hash_map instead or "
	"absl::node_hash_map if pointer stability of keys/values is required")
	if "std::unordered_set" in line:
	reportError("Don't use std::unordered_set; use absl::flat_hash_set instead or "
	"absl::node_hash_set if pointer stability of keys/values is required")
	if "std::atomic_" in line:
	# The std::atomic_* free functions are functionally equivalent to calling
	# operations on std::atomic<T> objects, so prefer to use that instead.
	reportError("Don't use free std::atomic_* functions, use std::atomic<T> members instead.")
	# Block usage of certain std types/functions as iOS 11 and macOS 10.13
	# do not support these at runtime.
	# See: https://github.com/envoyproxy/envoy/issues/12341
	if tokenInLine("std::any", line):
	reportError("Don't use std::any; use absl::any instead")
	if tokenInLine("std::get_if", line):
	reportError("Don't use std::get_if; use absl::get_if instead")
	if tokenInLine("std::holds_alternative", line):
	reportError("Don't use std::holds_alternative; use absl::holds_alternative instead")
	if tokenInLine("std::make_optional", line):
	reportError("Don't use std::make_optional; use absl::make_optional instead")
	if tokenInLine("std::monostate", line):
	reportError("Don't use std::monostate; use absl::monostate instead")
	if tokenInLine("std::optional", line):
	reportError("Don't use std::optional; use absl::optional instead")
	if tokenInLine("std::string_view", line):
	reportError("Don't use std::string_view; use absl::string_view instead")
	if tokenInLine("std::variant", line):
	reportError("Don't use std::variant; use absl::variant instead")
	if tokenInLine("std::visit", line):
	reportError("Don't use std::visit; use absl::visit instead")
	if "__attribute__((packed))" in line and file_path != "./include/envoy/common/platform.h":
	# __attribute__((packed)) is not supported by MSVC, we have a PACKED_STRUCT macro that
	# can be used instead
	reportError("Don't use __attribute__((packed)), use the PACKED_STRUCT macro defined "
	"in include/envoy/common/platform.h instead")
	if DESIGNATED_INITIALIZER_REGEX.search(line):
	# Designated initializers are not part of the C++14 standard and are not supported
	# by MSVC
	reportError("Don't use designated initializers in struct initialization, "
	"they are not part of C++14")
	if " ?: " in line:
	# The ?: operator is non-standard, it is a GCC extension
	reportError("Don't use the '?:' operator, it is a non-standard GCC extension")
	if line.startswith("using testing::Test;"):
	reportError("Don't use 'using testing::Test;, elaborate the type instead")
	if line.startswith("using testing::TestWithParams;"):
	reportError("Don't use 'using testing::Test;, elaborate the type instead")
	if TEST_NAME_STARTING_LOWER_CASE_REGEX.search(line):
	# Matches variants of TEST(), TEST_P(), TEST_F() etc. where the test name begins
	# with a lowercase letter.
	reportError("Test names should be CamelCase, starting with a capital letter")
	if not allowlistedForSerializeAsString(file_path) and "SerializeAsString" in line:
	# The MessageLite::SerializeAsString doesn't generate deterministic serialization,
	# use MessageUtil::hash instead.
	reportError(
	"Don't use MessageLite::SerializeAsString for generating deterministic serialization, use MessageUtil::hash instead."
	)
	if not allowlistedForJsonStringToMessage(file_path) and "JsonStringToMessage" in line:
	# Centralize all usage of JSON parsing so it is easier to make changes in JSON parsing
	# behavior.
	reportError("Don't use Protobuf::util::JsonStringToMessage, use TestUtility::loadFromJson.")
	if isInSubdir(file_path, 'source') and file_path.endswith('.cc') and \
	('.counterFromString(' in line or '.gaugeFromString(' in line or \
	'.histogramFromString(' in line or '.textReadoutFromString(' in line or \
	'->counterFromString(' in line or '->gaugeFromString(' in line or \
	'->histogramFromString(' in line or '->textReadoutFromString(' in line):
	reportError("Don't lookup stats by name at runtime; use StatName saved during construction")
	if MANGLED_PROTOBUF_NAME_REGEX.search(line):
	reportError("Don't use mangled Protobuf names for enum constants")
	hist_m = HISTOGRAM_SI_SUFFIX_REGEX.search(line)
	if hist_m and not allowlistedForHistogramSiSuffix(hist_m.group(0)):
	reportError(
	"Don't suffix histogram names with the unit symbol, "
	"it's already part of the histogram object and unit-supporting sinks can use this information natively, "
	"other sinks can add the suffix automatically on flush should they prefer to do so.")
	if not allowlistedForStdRegex(file_path) and "std::regex" in line:
	reportError("Don't use std::regex in code that handles untrusted input. Use RegexMatcher")
	if not allowlistedForGrpcInit(file_path):
	grpc_init_or_shutdown = line.find("grpc_init()")
	grpc_shutdown = line.find("grpc_shutdown()")
	if grpc_init_or_shutdown == -1 or (grpc_shutdown != -1 and
	grpc_shutdown < grpc_init_or_shutdown):
	grpc_init_or_shutdown = grpc_shutdown
	if grpc_init_or_shutdown != -1:
	comment = line.find("// ")
	if comment == -1 or comment > grpc_init_or_shutdown:
	reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " +
	"Grpc::GoogleGrpcContext. See #8282")
	if denylistedForExceptions(file_path):
	# Skpping cases where 'throw' is a substring of a symbol like in "foothrowBar".
	if "throw" in line.split():
	comment_match = COMMENT_REGEX.search(line)
	if comment_match is None or comment_match.start(0) > line.find("throw"):
	reportError("Don't introduce throws into exception-free files, use error " +
	"statuses instead.")
	if "lua_pushlightuserdata" in line:
	reportError("Don't use lua_pushlightuserdata, since it can cause unprotected error in call to" +
	"Lua API (bad light userdata pointer) on ARM64 architecture. See " +
	"https://github.com/LuaJIT/LuaJIT/issues/450#issuecomment-433659873 for details.")
	def checkBuildLine(line, file_path, reportError):
	if "@bazel_tools" in line and not (isStarlarkFile(file_path) or
	file_path.startswith("./bazel/") or "python/runfiles" in line):
	reportError("unexpected @bazel_tools reference, please indirect via a definition in //bazel")
	if not allowlistedForProtobufDeps(file_path) and '"protobuf"' in line:
	reportError("unexpected direct external dependency on protobuf, use "
	"//source/common/protobuf instead.")
	if (envoy_build_rule_check and not isStarlarkFile(file_path) and
	not isWorkspaceFile(file_path) and not isExternalBuildFile(file_path) and "@envoy//" in line):
	reportError("Superfluous '@envoy//' prefix")
	def fixBuildLine(file_path, line, line_number):
	if (envoy_build_rule_check and not isStarlarkFile(file_path) and
	not isWorkspaceFile(file_path) and not isExternalBuildFile(file_path)):
	line = line.replace("@envoy//", "//")
	return line
	def fixBuildPath(file_path):
	evaluateLines(file_path, functools.partial(fixBuildLine, file_path))
	error_messages = []
	# TODO(htuch): Add API specific BUILD fixer script.
	if not isBuildFixerExcludedFile(file_path) and not isApiFile(file_path) and not isStarlarkFile(
	file_path) and not isWorkspaceFile(file_path):
	if os.system("%s %s %s" % (ENVOY_BUILD_FIXER_PATH, file_path, file_path)) != 0:
	error_messages += ["envoy_build_fixer rewrite failed for file: %s" % file_path]
	if os.system("%s -lint=fix -mode=fix %s" % (BUILDIFIER_PATH, file_path)) != 0:
	error_messages += ["buildifier rewrite failed for file: %s" % file_path]
	return error_messages
	def checkBuildPath(file_path):
	error_messages = []
	if not isBuildFixerExcludedFile(file_path) and not isApiFile(file_path) and not isStarlarkFile(
	file_path) and not isWorkspaceFile(file_path):
	command = "%s %s | diff %s -" % (ENVOY_BUILD_FIXER_PATH, file_path, file_path)
	error_messages += executeCommand(command, "envoy_build_fixer check failed", file_path)
	if isBuildFile(file_path) and (file_path.startswith(args.api_prefix + "envoy") or
	file_path.startswith(args.api_shadow_prefix + "envoy")):
	found = False
	for line in readLines(file_path):
	if "api_proto_package(" in line:
	found = True
	break
	if not found:
	error_messages += ["API build file does not provide api_proto_package()"]
	command = "%s -mode=diff %s" % (BUILDIFIER_PATH, file_path)
	error_messages += executeCommand(command, "buildifier check failed", file_path)
	error_messages += checkFileContents(file_path, checkBuildLine)
	return error_messages
	def fixSourcePath(file_path):
	evaluateLines(file_path, fixSourceLine)
	error_messages = []
	if not file_path.endswith(DOCS_SUFFIX):
	if not file_path.endswith(PROTO_SUFFIX):
	error_messages += fixHeaderOrder(file_path)
	error_messages += clangFormat(file_path)
	if file_path.endswith(PROTO_SUFFIX) and isApiFile(file_path):
	package_name, error_message = packageNameForProto(file_path)
	if package_name is None:
	error_messages += error_message
	return error_messages
	def checkSourcePath(file_path):
	error_messages = checkFileContents(file_path, checkSourceLine)
	if not file_path.endswith(DOCS_SUFFIX):
	if not file_path.endswith(PROTO_SUFFIX):
	error_messages += checkNamespace(file_path)
	command = ("%s --include_dir_order %s --path %s | diff %s -" %
	(HEADER_ORDER_PATH, include_dir_order, file_path, file_path))
	error_messages += executeCommand(command, "header_order.py check failed", file_path)
	command = ("%s %s | diff %s -" % (CLANG_FORMAT_PATH, file_path, file_path))
	error_messages += executeCommand(command, "clang-format check failed", file_path)
	if file_path.endswith(PROTO_SUFFIX) and isApiFile(file_path):
	package_name, error_message = packageNameForProto(file_path)
	if package_name is None:
	error_messages += error_message
	return error_messages
	# Example target outputs are:
	# - "26,27c26"
	# - "12,13d13"
	# - "7a8,9"
	def executeCommand(command,
	error_message,
	file_path,
	regex=re.compile(r"^(\d+)[a|c|d]?\d*(?:,\d+[a|c|d]?\d*)?$")):
	try:
	output = subprocess.check_output(command, shell=True, stderr=subprocess.STDOUT).strip()
	if output:
	return output.decode('utf-8').split("\n")
	return []
	except subprocess.CalledProcessError as e:
	if (e.returncode != 0 and e.returncode != 1):
	return ["ERROR: something went wrong while executing: %s" % e.cmd]
	# In case we can't find any line numbers, record an error message first.
	error_messages = ["%s for file: %s" % (error_message, file_path)]
	for line in e.output.decode('utf-8').splitlines():
	for num in regex.findall(line):
	error_messages.append(" %s:%s" % (file_path, num))
	return error_messages
	def fixHeaderOrder(file_path):
	command = "%s --rewrite --include_dir_order %s --path %s" % (HEADER_ORDER_PATH, include_dir_order,
	file_path)
	if os.system(command) != 0:
	return ["header_order.py rewrite error: %s" % (file_path)]
	return []
	def clangFormat(file_path):
	command = "%s -i %s" % (CLANG_FORMAT_PATH, file_path)
	if os.system(command) != 0:
	return ["clang-format rewrite error: %s" % (file_path)]
	return []
	def checkFormat(file_path):
	if file_path.startswith(EXCLUDED_PREFIXES):
	return []
	if not file_path.endswith(SUFFIXES):
	return []
	error_messages = []
	# Apply fixes first, if asked, and then run checks. If we wind up attempting to fix
	# an issue, but there's still an error, that's a problem.
	try_to_fix = operation_type == "fix"
	if isBuildFile(file_path) or isStarlarkFile(file_path) or isWorkspaceFile(file_path):
	if try_to_fix:
	error_messages += fixBuildPath(file_path)
	error_messages += checkBuildPath(file_path)
	else:
	if try_to_fix:
	error_messages += fixSourcePath(file_path)
	error_messages += checkSourcePath(file_path)
	if error_messages:
	return ["From %s" % file_path] + error_messages
	return error_messages
	def checkFormatReturnTraceOnError(file_path):
	"""Run checkFormat and return the traceback of any exception."""
	try:
	return checkFormat(file_path)
	except:
	return traceback.format_exc().split("\n")
	def checkOwners(dir_name, owned_directories, error_messages):
	"""Checks to make sure a given directory is present either in CODEOWNERS or OWNED_EXTENSIONS
	Args:
	dir_name: the directory being checked.
	owned_directories: directories currently listed in CODEOWNERS.
	error_messages: where to put an error message for new unowned directories.
	"""
	found = False
	for owned in owned_directories:
	if owned.startswith(dir_name) or dir_name.startswith(owned):
	found = True
	if not found and dir_name not in UNOWNED_EXTENSIONS:
	error_messages.append("New directory %s appears to not have owners in CODEOWNERS" % dir_name)
	def checkApiShadowStarlarkFiles(api_shadow_root, file_path, error_messages):
	command = "diff -u "
	command += file_path + " "
	api_shadow_starlark_path = api_shadow_root + re.sub(r"\./api/", '', file_path)
	command += api_shadow_starlark_path
	error_message = executeCommand(command, "invalid .bzl in generated_api_shadow", file_path)
	if operation_type == "check":
	error_messages += error_message
	elif operation_type == "fix" and len(error_message) != 0:
	shutil.copy(file_path, api_shadow_starlark_path)
	return error_messages
	def checkFormatVisitor(arg, dir_name, names):
	"""Run checkFormat in parallel for the given files.
	Args:
	arg: a tuple (pool, result_list, owned_directories, error_messages)
	pool and result_list are for starting tasks asynchronously.
	owned_directories tracks directories listed in the CODEOWNERS file.
	error_messages is a list of string format errors.
	dir_name: the parent directory of the given files.
	names: a list of file names.
	"""
	# Unpack the multiprocessing.Pool process pool and list of results. Since
	# python lists are passed as references, this is used to collect the list of
	# async results (futures) from running checkFormat and passing them back to
	# the caller.
	pool, result_list, owned_directories, api_shadow_root, error_messages = arg
	# Sanity check CODEOWNERS. This doesn't need to be done in a multi-threaded
	# manner as it is a small and limited list.
	source_prefix = './source/'
	full_prefix = './source/extensions/'
	# Check to see if this directory is a subdir under /source/extensions
	# Also ignore top level directories under /source/extensions since we don't
	# need owners for source/extensions/access_loggers etc, just the subdirectories.
	if dir_name.startswith(full_prefix) and '/' in dir_name[len(full_prefix):]:
	checkOwners(dir_name[len(source_prefix):], owned_directories, error_messages)
	for file_name in names:
	if dir_name.startswith("./api") and isStarlarkFile(file_name):
	result = pool.apply_async(checkApiShadowStarlarkFiles,
	args=(api_shadow_root, dir_name + "/" + file_name, error_messages))
	result_list.append(result)
	result = pool.apply_async(checkFormatReturnTraceOnError, args=(dir_name + "/" + file_name,))
	result_list.append(result)
	# checkErrorMessages iterates over the list with error messages and prints
	# errors and returns a bool based on whether there were any errors.
	def checkErrorMessages(error_messages):
	if error_messages:
	for e in error_messages:
	print("ERROR: %s" % e)
	return True
	return False

under it. I moved these arguments

envoy/tools/code_format/check_format.py

Lines 1083 to 1098 in 130c7c4

	operation_type = args.operation_type
	target_path = args.target_path
	api_shadow_root = args.api_shadow_prefix
	envoy_build_rule_check = not args.skip_envoy_build_rule_check
	namespace_check = args.namespace_check
	namespace_check_excluded_paths = args.namespace_check_excluded_paths + [
	"./tools/api_boost/testdata/",
	"./tools/clang_tools/",
	]
	build_fixer_check_excluded_paths = args.build_fixer_check_excluded_paths + [
	"./bazel/external/",
	"./bazel/toolchains/",
	"./bazel/BUILD",
	"./tools/clang_tools",
	]
	include_dir_order = args.include_dir_order

to the initializer.

That particular highlighted change to the owners list is possibly due to my needing to rebase?

[htuch]

LGTM, thanks!