This is an automated email from the ASF dual-hosted git repository.
michaelsmith pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 25aa95a98 IMPALA-14540: Fix dump_breakpad_symbols.py's -b option and
add tests
25aa95a98 is described below
commit 25aa95a98d79b15aa2ae82b4ab001bc2d2766fad
Author: Joe McDonnell <[email protected]>
AuthorDate: Tue Nov 18 17:25:13 2025 -0800
IMPALA-14540: Fix dump_breakpad_symbols.py's -b option and add tests
When dump_breakpad_symbols.py switched to Python 3, the -b
option, which specifies a build directory, broke due to
Python 3's distinction between bytes and strings. This impacts
the bin/jenkins/finalize.sh script that runs at the end of
Jenkins jobs.
This fixes the -b option and adds tests for basic scenarios
for dump_breakpad_symbols.py. Specifically, it tests positive
scenarios for -b (build directory), -f (specific binary), and
-i (list provided via stdin). This adds additional validation
of dump_breakpad_symbols.py's inputs (e.g. verifying that the
file/directory exists, etc) and adds negative test cases for
that validation.
Testing:
- Ran new pytests locally
Change-Id: I8596e994b9d15be33d99e91ecc38f87c22545a08
Reviewed-on: http://gerrit.cloudera.org:8080/23688
Tested-by: Impala Public Jenkins <[email protected]>
Reviewed-by: Michael Smith <[email protected]>
---
bin/dump_breakpad_symbols.py | 31 +++++++++++--
tests/infra/test_breakpad_symbols.py | 87 ++++++++++++++++++++++++++++++++++++
2 files changed, 115 insertions(+), 3 deletions(-)
diff --git a/bin/dump_breakpad_symbols.py b/bin/dump_breakpad_symbols.py
index 844a01382..2b566845d 100755
--- a/bin/dump_breakpad_symbols.py
+++ b/bin/dump_breakpad_symbols.py
@@ -178,7 +178,7 @@ def is_regular_file(path):
def is_elf_file(path):
"""Check whether 'path' is an ELF file."""
- return is_regular_file(path) and 'ELF' in magic.from_file(path)
+ return is_regular_file(path) and b'ELF' in magic.from_file(path)
def find_elf_files(path):
@@ -213,7 +213,7 @@ def extract_pkg(pkg, out_dir):
def assert_file_exists(path):
if not os.path.isfile(path):
- die('File does not exists: %s' % path)
+ die('File does not exist: %s' % path)
def enumerate_pkg_files(pkg, symbol_pkg):
@@ -256,6 +256,29 @@ def enumerate_pkg_files(pkg, symbol_pkg):
shutil.rmtree(tmp_dir)
+def assert_binaries_exist(args):
+ """Validate the arguments specifying the location of binaries to assert that
+ they exist"""
+ if args.binary_files:
+ # Each binary file specified should exist
+ for f in args.binary_files:
+ assert_file_exists(f)
+ elif args.stdin_files:
+ # Each binary file specified on stdin should exist
+ for f in sys.stdin.read().splitlines():
+ assert_file_exists(f)
+ elif args.pkg:
+ assert_file_exists(args.pkg)
+ if args.symbol_pkg:
+ assert_file_exists(args.symbol_pkg)
+ elif args.build_dir:
+ # Specifying a non-existant directory should produce an error
+ if not os.path.isdir(args.build_dir):
+ die("Build directory (-b) does not exist: {0}".format(args.build_dir))
+ if len(list(find_elf_files(args.build_dir))) == 0:
+ die("No ELF files found in {0}".format(args.build_dir))
+
+
def enumerate_binaries(args):
"""Enumerate all BinarySymbolInfo tuples, from which symbols should be
extracted.
@@ -314,7 +337,8 @@ def process_binary(dump_syms, objcopy, binary, out_dir):
if binary.debug_path:
args.append(binary.debug_path)
- proc = subprocess.Popen(args, stdout=os.fdopen(tmp_fd, 'wb'),
stderr=subprocess.PIPE)
+ proc = subprocess.Popen(args, stdout=os.fdopen(tmp_fd, 'wb'),
stderr=subprocess.PIPE,
+ universal_newlines=True)
_, stderr = proc.communicate()
if proc.returncode != 0:
sys.stderr.write('dump_syms: Failed to dump symbols from %s, return code
%s\n' %
@@ -353,6 +377,7 @@ def main():
assert objcopy
status = 0
ensure_dir_exists(args.dest_dir)
+ assert_binaries_exist(args)
# The logic for handling DEB/RPM packages does not currently work with
# parallelism, so disable parallelism if using the -r/--pkg option.
if args.num_processes > 1 and not bool(args.pkg):
diff --git a/tests/infra/test_breakpad_symbols.py
b/tests/infra/test_breakpad_symbols.py
new file mode 100644
index 000000000..b9c3439f2
--- /dev/null
+++ b/tests/infra/test_breakpad_symbols.py
@@ -0,0 +1,87 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from __future__ import absolute_import, division, print_function
+
+import os
+import subprocess
+import tempfile
+
+from tests.common.base_test_suite import BaseTestSuite
+
+
+class TestDumpBreakpadSymbols(BaseTestSuite):
+
+ def __positive_test(self, binary_location_args, stdin_input=None):
+ impala_home = os.getenv("IMPALA_HOME")
+ with tempfile.TemporaryDirectory() as temp_dir:
+ subprocess.check_call(
+ ["{0}/bin/dump_breakpad_symbols.py".format(impala_home),
+ "-d", temp_dir] + binary_location_args, stdin=stdin_input)
+
+ def __negative_test(self, binary_location_args, expected_error_messages):
+ impala_home = os.getenv("IMPALA_HOME")
+ with tempfile.TemporaryDirectory() as temp_dir:
+ p = subprocess.Popen(
+ ["{0}/bin/dump_breakpad_symbols.py".format(impala_home),
+ "-d", temp_dir] + binary_location_args, stderr=subprocess.PIPE,
+ universal_newlines=True)
+ _, stderr_output = p.communicate()
+ # Verify that it failed with the expected error messages
+ assert p.returncode != 0
+ for expected_error in expected_error_messages:
+ assert expected_error in stderr_output
+
+ def test_binary_locations_positive(self):
+ """Test different ways to specify binaries for dump_breakpad_symbols.py to
catch
+ obvious issues"""
+ impala_build_dir = os.path.join(os.getenv("IMPALA_HOME"),
"be/build/latest")
+
+ # Test specifying an exact executable file (-f)
+ self.__positive_test(["-f", os.path.join(impala_build_dir,
"service/impalad")])
+
+ # Test specifying a build directory with executables (-b)
+ self.__positive_test(["-b", os.path.join(impala_build_dir, "service")])
+
+ # Test specifying a list of executables via stdin (-i)
+ with tempfile.TemporaryDirectory() as temp_dir:
+ with open(os.path.join(temp_dir, "input_file"), "w") as input_list:
+ input_list.write(os.path.join(impala_build_dir, "service/impalad"))
+ input_list.write("\n")
+ input_list.write(os.path.join(impala_build_dir,
"util/impala-profile-tool"))
+
+ with open(os.path.join(temp_dir, "input_file")) as input_list:
+ self.__positive_test(["-i"], stdin_input=input_list)
+
+ def test_binary_locations_negative(self):
+ """Test a couple error cases for dump_breakpad_symbols.py to verify
+ reasonable error messages"""
+ impala_build_dir = os.path.join(os.getenv("IMPALA_HOME"),
"be/build/latest")
+ nonexistant_path = os.path.join(impala_build_dir, "doesnt_exist")
+
+ # Non-existant executable with -f option
+ self.__negative_test(["-f", nonexistant_path],
+ ["File does not exist", nonexistant_path])
+
+ # Non-existant build directory with -b option
+ self.__negative_test(["-b", nonexistant_path],
+ ["Build directory (-b) does not exist", nonexistant_path])
+
+ # Build directory that doesn't contain any ELF files (we'll use a temporary
+ # directory for this)
+ with tempfile.TemporaryDirectory() as temp_dir:
+ self.__negative_test(["-b", temp_dir], ["No ELF files found in",
temp_dir])
