Page MenuHomePhabricator

[lldb/test] Print build commands in trace mode
ClosedPublic

Authored by labath on Oct 21 2021, 3:47 AM.

Details

Summary

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

Diff Detail

Event Timeline

labath requested review of this revision.Oct 21 2021, 3:47 AM
labath created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 3:47 AM
labath added inline comments.Oct 21 2021, 3:49 AM
lldb/packages/Python/lldbsuite/test/lldbtest.py
1429

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

DavidSpickett added a subscriber: DavidSpickett.
DavidSpickett added inline comments.
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 updated this revision to Diff 381223.Oct 21 2021, 5:24 AM

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 added inline comments.Oct 21 2021, 5:46 AM
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.

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)

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 updated this revision to Diff 381500.Oct 22 2021, 3:26 AM
  • support single command only
  • add a test

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.

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

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.

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?

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.

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.

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.

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 accepted this revision.Thu, Oct 28, 7:12 PM

Thanks everyone. LGTM!

This revision is now accepted and ready to land.Thu, Oct 28, 7:12 PM
labath marked 2 inline comments as done.Fri, Oct 29, 2:32 AM
labath added inline comments.
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.

This revision was landed with ongoing or failed builds.Fri, Oct 29, 2:34 AM
This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.