[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Closed by commit rGeee887e03551: [lldb/test] Print build commands in trace mode 
(authored by labath).

Changed prior to commit:
  https://reviews.llvm.org/D112212?vs=381500&id=383269#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py
  lldb/test/API/test_utils/base/Makefile
  lldb/test/API/test_utils/base/TestBaseTest.py
  lldb/test/API/test_utils/base/return0.cpp

Index: lldb/test/API/test_utils/base/return0.cpp
===
--- /dev/null
+++ lldb/test/API/test_utils/base/return0.cpp
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/test_utils/base/TestBaseTest.py
===
--- /dev/null
+++ lldb/test/API/test_utils/base/TestBaseTest.py
@@ -0,0 +1,35 @@
+"""
+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])
Index: lldb/test/API/test_utils/base/Makefile
===
--- /dev/null
+++ lldb/test/API/test_utils/base/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -1,10 +1,11 @@
+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", "")
-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)
@@ -12,4 +13,4 @@
 @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)
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -45,6 +45,7 @@
 import re
 import shutil
 import signal
+import shlex
 from subprocess import *
 import sys
 import time
@@ -68,6 +69,7 @@
 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.
@@ -470,61 +472,6 @@
 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 roo

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.



Comment at: lldb/test/API/test_utils/TestBaseTest.py:1
+"""
+Test TestBase test functions.

teemperor wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Thanks everyone. LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.




Comment at: lldb/test/API/test_utils/TestBaseTest.py:1
+"""
+Test TestBase test functions.

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).



Comment at: lldb/test/API/test_utils/TestBaseTest.py:18
+
+def trace(self, *args, **kwargs):
+io = six.StringIO()

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...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I agree with what Raphael said.

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




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1422
+def runBuildCommands(self, commands):
+for cmd in commands:
+self.trace(shlex.join(cmd))

DavidSpickett wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 381500.
labath added a comment.

- support single command only
- add a test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py
  lldb/test/API/test_utils/Makefile
  lldb/test/API/test_utils/TestBaseTest.py
  lldb/test/API/test_utils/return0.cpp

Index: lldb/test/API/test_utils/return0.cpp
===
--- /dev/null
+++ lldb/test/API/test_utils/return0.cpp
@@ -0,0 +1 @@
+int main() { return 0; }
Index: lldb/test/API/test_utils/TestBaseTest.py
===
--- /dev/null
+++ lldb/test/API/test_utils/TestBaseTest.py
@@ -0,0 +1,34 @@
+"""
+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
+
+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])
Index: lldb/test/API/test_utils/Makefile
===
--- /dev/null
+++ lldb/test/API/test_utils/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -1,10 +1,11 @@
+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", "")
-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)
@@ -12,4 +13,4 @@
 @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)
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -45,6 +45,7 @@
 import re
 import shutil
 import signal
+import shlex
 from subprocess import *
 import sys
 import time
@@ -68,6 +69,7 @@
 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.
@@ -469,61 +471,6 @@
 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_existe

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1422
+def runBuildCommands(self, commands):
+for cmd in commands:
+self.trace(shlex.join(cmd))

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.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1284
 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"])
+m = re.search(b'version ([0-9.]+)', version_output)

labath wrote:
> DavidSpickett wrote:
> > 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.
> 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.)
> and does not throw an exception on non-utf output

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1284
 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"])
+m = re.search(b'version ([0-9.]+)', version_output)

DavidSpickett wrote:
> 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.
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.)



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1425
+try:
+proc = run(cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT,
+check=True)

DavidSpickett wrote:
> labath wrote:
> > PSA: this function is available from python-3.5 onwards.
> 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)
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 381223.
labath added a comment.

Respond to reviewer comments (thanks).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py

Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -1,10 +1,11 @@
+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", "")
-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)
@@ -12,4 +13,4 @@
 @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)
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -45,6 +45,7 @@
 import re
 import shutil
 import signal
+import shlex
 from subprocess import *
 import sys
 import time
@@ -68,6 +69,7 @@
 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.
@@ -469,61 +471,6 @@
 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,
@@ -1334,11 +1281,10 @@
 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)
+if m:
+return m.group(1)
 return 'unknown'
 
 def getDwarfVersion(self):
@@ -1465,9 +1411,2

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: DavidSpickett.
DavidSpickett added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1284
 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"])
+m = re.search(b'version ([0-9.]+)', version_output)

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.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1425
+try:
+proc = run(cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT,
+check=True)

labath wrote:
> PSA: this function is available from python-3.5 onwards.
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1425
+try:
+proc = run(cmd, stdin=DEVNULL, stdout=PIPE, stderr=STDOUT,
+check=True)

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112212/new/

https://reviews.llvm.org/D112212

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: teemperor, JDevlieghere.
labath requested review of this revision.
Herald added a project: LLDB.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112212

Files:
  lldb/packages/Python/lldbsuite/test/builders/builder.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py

Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -1,10 +1,11 @@
+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", "")
-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)
Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -45,6 +45,7 @@
 import re
 import shutil
 import signal
+import shlex
 from subprocess import *
 import sys
 import time
@@ -68,6 +69,7 @@
 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.
@@ -469,61 +471,6 @@
 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,
@@ -1334,11 +1281,10 @@
 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"])
+m = re.search(b'version ([0-9.]+)', version_output)
+if m:
+return m.group(1).decode(errors="replace")
 return 'unknown'
 
 def getDwarfVersion(self):
@@ -1465,9 +1411,23 @@
 testname = self.getBuildDirBasename()
 
 module = builder_module()
-if not module.build(debug_info, architecture, compiler, dictionary,
-testdir, testname):
+c