[lldb/test] Print build commands in trace mode

[labath]

Summary

Running tests with -t prints all lldb commands being run. It makes sense
to print all the build commands as well.

[labath]

lldb/packages/Python/lldbsuite/test/lldbtest.py

1429

PSA: this function is available from python-3.5 onwards.

[DavidSpickett]

lldb/packages/Python/lldbsuite/test/lldbtest.py

1285–1288

You could use universal_newlines here to get the decoded string. It's a bit cryptic but saves the decode below.

There is an alias text name in 3.7 but requiring that seems ambitious.

1429

Any reason to use run(...check=True) rather than check_call?

https://docs.python.org/3/library/subprocess.html#subprocess.check_call

(universal_newlines applies here too if you want)

[labath]

Respond to reviewer comments (thanks).

lldb/packages/Python/lldbsuite/test/lldbtest.py

1285–1288

how about error="replace" on the check_output invocation it's equally cryptic, and does not throw an exception on non-utf output?

(I'm generally unsure about the best way to handle the byte-string duality in python3 -- whether to try to handle things at the byte level (if they can) or to convert everything to strings as soon as possible and pretend bytes don't exist.)

1429

I must have thought I needed some of this functionality, but now that I look at it -- I don't. So check_output it is.

[DavidSpickett]

lldb/packages/Python/lldbsuite/test/lldbtest.py

1285–1288

and does not throw an exception on non-utf output

I didn't consider that so yeah this looks fine.

1426

Is this ever going to have more than one command to run?

Seems like the source is the function above that puts a single command into a list.

[dblaikie]

Does this sort of thing itself get tested? (like this one had a test: https://reviews.llvm.org/D111978 but not sure how much that generalizes/whether there are different parts of the infrastructure are more or less testable)

[teemperor]

In D112212#3078681, @dblaikie wrote:

Does this sort of thing itself get tested? (like this one had a test: https://reviews.llvm.org/D111978 but not sure how much that generalizes/whether there are different parts of the infrastructure are more or less testable)

I think historically we haven't really tested the test infrastructure, but we're not historians (badum-ts) so I would vote in favour of adding tests for this (and the other testing infra).

(I'll also gladly write some tests for the already existing testing utils unless someone objects).

[labath]

• support single command only
• add a test

[labath]

I agree with what Raphael said.

Here's my attempt at a test case. Let me know what you think.

lldb/packages/Python/lldbsuite/test/lldbtest.py

1426

Well.. the general idea of the builder plugins was that they could build the test executable in any way they see fit, but I don't reallisticaly see it using anything other than makefiles any time soon. The whole builder module idea is a bit flawed, because it keys everything off of the host platform, but most of the customizations need to happen because of target platform specifics. So, I guess I'll just drop the multiple command mode.

[teemperor]

This LGTM, but shlex.join is actually Py3 exclusive and I don't think there is a good Py2 replacement. I think we're just in time for the Py2->3 migration according to the timeline Jonas posted last year, so let's use this patch to actually do that? Then we can also get rid of all the six stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise this is good to go.

lldb/test/API/test_utils/TestBaseTest.py

1 ↗

(On Diff #381500)

Could we move this file into test_utils/build or some other subdir? Then I can also make the few other 'test'-tests their own subdir of test_utils (which seems like a good place for all of this).

18 ↗

(On Diff #381500)

I think a comment that this overrides the normal test trace method would be nice (I wish Python had some builtin thing for indicating overrides...)

[JDevlieghere]

In D112212#3080491, @teemperor wrote:

This LGTM, but shlex.join is actually Py3 exclusive and I don't think there is a good Py2 replacement. I think we're just in time for the Py2->3 migration according to the timeline Jonas posted last year, so let's use this patch to actually do that? Then we can also get rid of all the six stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise this is good to go.

We're planning to branch from open source on October 26th. If there's no urgency, it would really be great if we can hold off breaking Py2 until then.

I'm all in favor for getting rid of Python 2 support, but sweeping changes like dropping the six stuff will introduce a lot of headaches (merge conflicts) for us. If we could postpone that for another release that would save us a bunch of engineering time.

[dblaikie]

In D112212#3081828, @JDevlieghere wrote:

In D112212#3080491, @teemperor wrote:

This LGTM, but shlex.join is actually Py3 exclusive and I don't think there is a good Py2 replacement. I think we're just in time for the Py2->3 migration according to the timeline Jonas posted last year, so let's use this patch to actually do that? Then we can also get rid of all the six stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise this is good to go.

We're planning to branch from open source on October 26th. If there's no urgency, it would really be great if we can hold off breaking Py2 until then.

I'm all in favor for getting rid of Python 2 support, but sweeping changes like dropping the six stuff will introduce a lot of headaches (merge conflicts) for us. If we could postpone that for another release that would save us a bunch of engineering time.

No judgment (I think it's a reasonable request to punt a patch like this a few days if it helps out major contributors) - but I'm curious/just not quite wrapping my head around: Why would it be easier if this sort of patch went in after you branch? I'd have thought it'd be easier if it goes in before the branch. That way when you're backporting patches from upstream after the branch there will be fewer unrelated changes/merge conflicts, yeah?

[JDevlieghere]

In D112212#3081935, @dblaikie wrote:

In D112212#3081828, @JDevlieghere wrote:

In D112212#3080491, @teemperor wrote:

This LGTM, but shlex.join is actually Py3 exclusive and I don't think there is a good Py2 replacement. I think we're just in time for the Py2->3 migration according to the timeline Jonas posted last year, so let's use this patch to actually do that? Then we can also get rid of all the six stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise this is good to go.

We're planning to branch from open source on October 26th. If there's no urgency, it would really be great if we can hold off breaking Py2 until then.

I'm all in favor for getting rid of Python 2 support, but sweeping changes like dropping the six stuff will introduce a lot of headaches (merge conflicts) for us. If we could postpone that for another release that would save us a bunch of engineering time.

No judgment (I think it's a reasonable request to punt a patch like this a few days if it helps out major contributors) - but I'm curious/just not quite wrapping my head around: Why would it be easier if this sort of patch went in after you branch? I'd have thought it'd be easier if it goes in before the branch. That way when you're backporting patches from upstream after the branch there will be fewer unrelated changes/merge conflicts, yeah?

The patch introduces a dependency on Python 3 and unfortunately we still have a small (but important) group of users that haven't fully migrated yet. If the patch were to land before the branch, I'd have to revert it (same result) or find a way to do what shlex.join does in Python 2. I did a quick search yesterday and didn't immediately find a good alternative and with the timeline I've given in the past, I also don't think the burden should be on the patch author (Pavel). So that's why I suggested holding off on landing it. If it does turn out to cause a lot of conflicts, I can always reconsider.

But yes, backporting is a real concern, which is the main reason I'm asking not to start making big mechanical changes like replacing all the six stuff unless there's a pressing reason to do so.

[dblaikie]

In D112212#3082352, @JDevlieghere wrote:

In D112212#3081935, @dblaikie wrote:

In D112212#3081828, @JDevlieghere wrote:

In D112212#3080491, @teemperor wrote:

This LGTM, but shlex.join is actually Py3 exclusive and I don't think there is a good Py2 replacement. I think we're just in time for the Py2->3 migration according to the timeline Jonas posted last year, so let's use this patch to actually do that? Then we can also get rid of all the six stuff etc.

Let's see if Jonas has any objections against dropping Py2 with this, otherwise this is good to go.

We're planning to branch from open source on October 26th. If there's no urgency, it would really be great if we can hold off breaking Py2 until then.

I'm all in favor for getting rid of Python 2 support, but sweeping changes like dropping the six stuff will introduce a lot of headaches (merge conflicts) for us. If we could postpone that for another release that would save us a bunch of engineering time.

No judgment (I think it's a reasonable request to punt a patch like this a few days if it helps out major contributors) - but I'm curious/just not quite wrapping my head around: Why would it be easier if this sort of patch went in after you branch? I'd have thought it'd be easier if it goes in before the branch. That way when you're backporting patches from upstream after the branch there will be fewer unrelated changes/merge conflicts, yeah?

The patch introduces a dependency on Python 3 and unfortunately we still have a small (but important) group of users that haven't fully migrated yet. If the patch were to land before the branch, I'd have to revert it (same result) or find a way to do what shlex.join does in Python 2. I did a quick search yesterday and didn't immediately find a good alternative and with the timeline I've given in the past, I also don't think the burden should be on the patch author (Pavel). So that's why I suggested holding off on landing it. If it does turn out to cause a lot of conflicts, I can always reconsider.

But yes, backporting is a real concern, which is the main reason I'm asking not to start making big mechanical changes like replacing all the six stuff unless there's a pressing reason to do so.

Ah, fair enough - thanks for the context! (given that it'd be a matter of reverting the patch on your release branch - doesn't seem like a huge difference, but also easy to wait a few days) - *nod* cleanup's hopefully not too expensive to defer for a little while longer, if it's not getting in the way of some feature work.

[teemperor]

Given that shlex.join is only used for making the diagnostics copy-pastable into the terminal, we could probably get away by just making it use the normal string conversion of list or something like " ".join(...). I don't think we have anyone that really debugs tests in Python2 builds.

[labath]

In D112212#3083194, @teemperor wrote:

Given that shlex.join is only used for making the diagnostics copy-pastable into the terminal, we could probably get away by just making it use the normal string conversion of list or something like " ".join(...). I don't think we have anyone that really debugs tests in Python2 builds.

Yeah, I was thinking the same thing, but given that Oct 26th is like tomorrow, I think I can just wait a couple of days.

[JDevlieghere]

Thanks everyone. LGTM!

[labath]

lldb/test/API/test_utils/TestBaseTest.py

1 ↗

(On Diff #381500)

I've created a base folder for the Base class. It could either be a root of an additional hierarchy or we can shove all Base tests there. I'd hope it's the latter, as one can go quite far by just passing CXX_SOURCE to the make invocations, and these aren't particularly complex tests or inferiors.

lldb/packages/Python/lldbsuite/test/builders/builder.py

def getCmdLine(self, d):
             if k in append_vars and k in os.environ:
                 v = os.environ[k] + " " + v
             return '%s=%s' % (k, v)
 
         cmdline = [setOrAppendVariable(k, v) for k, v in list(d.items())]
 
         return cmdline
 
-    def runBuildCommands(self, commands):
-        try:
-            lldbtest.system(commands)
-        except subprocess.CalledProcessError as called_process_error:
-            # Convert to a build-specific error.
-            # We don't do that in lldbtest.system() since that
-            # is more general purpose.
-            raise build_exception.BuildError(called_process_error)
-
     def getArchSpec(self, architecture):
         """
         Helper function to return the key-value string to specify the architecture
         used for the make system.
         """
         return ["ARCH=" + architecture] if architecture else []
 
     def getCCSpec(self, compiler):
def _getDebugInfoArgs(self, debug_info):
         if debug_info == "dwarf":
             return ["MAKE_DSYM=NO"]
         if debug_info == "dwo":
             return ["MAKE_DSYM=NO", "MAKE_DWO=YES"]
         if debug_info == "gmodules":
             return ["MAKE_DSYM=NO", "MAKE_GMODULES=YES"]
         return None
 
-    def build(self, debug_info, architecture=None, compiler=None,
+    def getBuildCommand(self, debug_info, architecture=None, compiler=None,
             dictionary=None, testdir=None, testname=None):
         debug_info_args = self._getDebugInfoArgs(debug_info)
         if debug_info_args is None:
-            return False
+            return None
 
         command_parts = [
             self.getMake(testdir, testname), debug_info_args, ["all"],
             self.getArchCFlags(architecture), self.getArchSpec(architecture),
             self.getCCSpec(compiler), self.getExtraMakeArgs(),
             self.getSDKRootSpec(), self.getModuleCacheSpec(),
             self.getCmdLine(dictionary)]
         command = list(itertools.chain(*command_parts))
 
-        self.runBuildCommands([command])
-        return True
+        return command
 
     def cleanup(self, dictionary=None):
         """Perform a platform-specific cleanup after the test."""
         return True

lldb/packages/Python/lldbsuite/test/lldbtest.py

 from functools import wraps
 import gc
 import glob
 import io
 import os.path
 import re
 import shutil
 import signal
+import shlex
 from subprocess import *
 import sys
 import time
 import traceback
 import distutils.spawn
 
 # Third-party modules
 import unittest2
 from six import add_metaclass
 from six import StringIO as SixStringIO
 import six
 
 # LLDB modules
 import lldb
 from . import configuration
 from . import decorators
 from . import lldbplatformutil
 from . import lldbtest_config
 from . import lldbutil
 from . import test_categories
 from lldbsuite.support import encoded_file
 from lldbsuite.support import funcutils
 from lldbsuite.test.builders import get_builder
+from lldbsuite.test_event import build_exception
 
 # See also dotest.parseOptionsAndInitTestdirs(), where the environment variables
 # LLDB_COMMAND_TRACE is set from '-t' option.
 
 # By default, traceAlways is False.
 if "LLDB_COMMAND_TRACE" in os.environ and os.environ[
         "LLDB_COMMAND_TRACE"] == "YES":
     traceAlways = True
def launch(self, executable, args, extra_env):
             raise Exception(
                 "remote_platform.Launch('%s', '%s') failed: %s" %
                 (dst_path, args, err))
         self._pid = launch_info.GetProcessID()
 
     def terminate(self):
         lldb.remote_platform.Kill(self._pid)
 
-# From 2.7's subprocess.check_output() convenience function.
-# Return a tuple (stdoutdata, stderrdata).
-
-
-def system(commands, **kwargs):
-    r"""Run an os command with arguments and return its output as a byte string.
-
-    If the exit code was non-zero it raises a CalledProcessError.  The
-    CalledProcessError object will have the return code in the returncode
-    attribute and output in the output attribute.
-
-    The arguments are the same as for the Popen constructor.  Example:
-
-    >>> check_output(["ls", "-l", "/dev/null"])
-    'crw-rw-rw- 1 root root 1, 3 Oct 18  2007 /dev/null\n'
-
-    The stdout argument is not allowed as it is used internally.
-    To capture standard error in the result, use stderr=STDOUT.
-
-    >>> check_output(["/bin/sh", "-c",
-    ...               "ls -l non_existent_file ; exit 0"],
-    ...              stderr=STDOUT)
-    'ls: non_existent_file: No such file or directory\n'
-    """
-
-    output = ""
-    error = ""
-    for shellCommand in commands:
-        if 'stdout' in kwargs:
-            raise ValueError(
-                'stdout argument not allowed, it will be overridden.')
-        process = Popen(
-            shellCommand,
-            stdout=PIPE,
-            stderr=STDOUT,
-            **kwargs)
-        pid = process.pid
-        this_output, this_error = process.communicate()
-        retcode = process.poll()
-
-        if retcode:
-            cmd = kwargs.get("args")
-            if cmd is None:
-                cmd = shellCommand
-            cpe = CalledProcessError(retcode, cmd)
-            # Ensure caller can access the stdout/stderr.
-            cpe.lldb_extensions = {
-                "combined_output": this_output,
-                "command": shellCommand
-            }
-            raise cpe
-        output = output + this_output.decode("utf-8", errors='ignore')
-    return output
-
-
 def getsource_if_available(obj):
     """
     Return the text of the source code for an object if available.  Otherwise,
     a print representation is returned.
     """
     import inspect
     try:
         return inspect.getsource(obj)
def getCompilerBinary(self):
         """Returns the compiler binary the test suite is running with."""
         return self.getCompiler().split()[0]
 
     def getCompilerVersion(self):
         """ Returns a string that represents the compiler version.
             Supports: llvm, clang.
         """
         compiler = self.getCompilerBinary()
-        version_output = system([[compiler, "--version"]])
-        for line in version_output.split(os.linesep):
-            m = re.search('version ([0-9.]+)', line)
-            if m:
-                return m.group(1)
+        version_output = check_output([compiler, "--version"], errors="replace")
+        m = re.search('version ([0-9.]+)', version_output)
         return 'unknown'
 
     def getDwarfVersion(self):
         """ Returns the dwarf version generated by clang or '0'. """
         if configuration.dwarf_version:
             return str(configuration.dwarf_version)
         if 'clang' in self.getCompiler():
             try:
def build(
             debug_info = self.getDebugInfo()
 
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
 
         testdir = self.mydir
         testname = self.getBuildDirBasename()
 
         module = builder_module()
-        if not module.build(debug_info, architecture, compiler, dictionary,
-                testdir, testname):
+        command = builder_module().getBuildCommand(debug_info, architecture,
+                compiler, dictionary, testdir, testname)
+        if command is None:
             raise Exception("Don't know how to build binary")
 
+        self.runBuildCommand(command)
+
+    def runBuildCommand(self, command):
+        self.trace(shlex.join(command))
+        try:
+            output = check_output(command, stderr=STDOUT, errors="replace")
+        except CalledProcessError as cpe:
+            raise build_exception.BuildError(cpe)
+        self.trace(output)
+
+
     # ==================================================
     # Build methods supported through a plugin interface
     # ==================================================
 
     def getstdlibFlag(self):
         """ Returns the proper -stdlib flag, or empty if not required."""
         if self.platformIsDarwin() or self.getPlatform() == "freebsd" or self.getPlatform() == "openbsd":
             stdlibflag = "-stdlib=libc++"
def yaml2obj(self, yaml_path, obj_path):
         Create an object file at the given path from a yaml file.
 
         Throws subprocess.CalledProcessError if the object could not be created.
         """
         yaml2obj_bin = configuration.get_yaml2obj_path()
         if not yaml2obj_bin:
             self.assertTrue(False, "No valid yaml2obj executable specified")
         command = [yaml2obj_bin, "-o=%s" % obj_path, yaml_path]
-        system([command])
+        self.runBuildCommand(command)
 
     def getBuildFlags(
             self,
             use_cpp11=True,
             use_libcxx=False,
             use_libstdcxx=False):
         """ Returns a dictionary (which can be provided to build* functions above) which
             contains OS-specific build flags.

lldb/packages/Python/lldbsuite/test_event/build_exception.py

+import shlex
+
 class BuildError(Exception):
 
     def __init__(self, called_process_error):
         super(BuildError, self).__init__("Error when building test subject")
-        self.command = called_process_error.lldb_extensions.get(
-            "command", "<command unavailable>")
-        self.build_error = called_process_error.lldb_extensions["combined_output"]
+        self.command = shlex.join(called_process_error.cmd)
+        self.build_error = called_process_error.output
 
     def __str__(self):
         return self.format_build_error(self.command, self.build_error)
 
     @staticmethod
     def format_build_error(command, command_output):
         return "Error when building test subject.\n\nBuild Command:\n{}\n\nBuild Command Output:\n{}".format(
-            command, command_output.decode("utf-8", errors='ignore'))
+            command, command_output)

lldb/test/API/test_utils/base/Makefile

+include Makefile.rules

lldb/test/API/test_utils/base/TestBaseTest.py

+"""
+Test TestBase test functions.
+"""
+
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test_event import build_exception
+import six
+
+class TestBuildMethod(Base):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def setUp(self):
+        super().setUp()
+        self._traces = []
+        self.traceAlways = True
+
+    # override the parent trace method
+    def trace(self, *args, **kwargs):
+        io = six.StringIO()
+        print(*args, file=io, **kwargs)
+        self._traces.append(io.getvalue())
+
+    def test_build_fails_helpfully(self):
+        try:
+            self.build(dictionary={"CXX_SOURCES": "nonexisting-file.cpp"})
+        except build_exception.BuildError as e:
+            self.assertIn("nonexisting-file.cpp", str(e))
+        else:
+            self.fail("BuildError not raised!")
+
+    def test_build_logs_traces(self):
+        self.build(dictionary={"CXX_SOURCES": "return0.cpp"})
+        self.assertIn("CXX_SOURCES", self._traces[0])
+        self.assertIn("return0.o", self._traces[1])

lldb/test/API/test_utils/base/return0.cpp

+int main() { return 0; }