webkitpy: Override TEMPDIR for Xvfb.

webkitpy: Override TEMPDIR for Xvfb.

Hard-code TEMPDIR/TMPDIR for "Xvfb" to "/tmp" in webkitpy's Linux layout
test port. The Xvfb tool's "xkb" library compiles in "/tmp" in several
places, and hard-linking expectations get violated if TEMPDIR and "/tmp"
are on different filesystems.

BUG=chromium:715848
TEST=local
  - Ran without patch and TEMPDIR/TMPDIR overrides, verified hardlink
    failure from bug.
  - Ran with patch, no more hardlink failure!

Review-Url: https://codereview.chromium.org/2859703002
Cr-Commit-Position: refs/heads/master@{#469378}
Committed: https://chromium.googlesource.com/chromium/src/+/9cc81c66fbe4e06f84a5d8647faf1a2fab5ecf38

[dnj, 2017-05-02 22:25:32]

dnj@chromium.org changed reviewers:
+ dpranke@google.com, iannucci@chromium.org, tansell@chromium.org

[dnj, 2017-05-02 22:25:33]

PTAL

[iannucci, 2017-05-03 01:17:19]

lgtm

[mithro, 2017-05-03 01:58:34]

Couple of small nits.

Are you able to actually add a test to the
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux_unittest.py
file?

I'm unsure if the MockExecutive lets you check the environment for a command?

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
(right):

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:166: #
process.
Don't need this second paragraph.

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:168: # See:
crbug.com/715848
https://crbug.com/XXXXX

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:170:
env['TEMPDIR'] = env['TMPDIR'] = '/tmp'
Can you log a warning if 'TEMPDIR' is set in the environment and not equal to
'/tmp'

[iannucci, 2017-05-03 02:06:37]

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
(right):

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:170:
env['TEMPDIR'] = env['TMPDIR'] = '/tmp'
On 2017/05/03 01:58:34, mithro wrote:
> Can you log a warning if 'TEMPDIR' is set in the environment and not equal to
> '/tmp'

Actually... Why set TEMPDIR? That's only used on windows.

[dnj, 2017-05-03 03:36:37]

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
(right):

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:166: #
process.
On 2017/05/03 01:58:34, mithro wrote:
> Don't need this second paragraph.

Done.

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:168: # See:
crbug.com/715848
On 2017/05/03 01:58:34, mithro wrote:
> https://crbug.com/XXXXX

Done.

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:170:
env['TEMPDIR'] = env['TMPDIR'] = '/tmp'
On 2017/05/03 01:58:34, mithro wrote:
> Can you log a warning if 'TEMPDIR' is set in the environment and not equal to
> '/tmp'

I can - however, this will generally not be set on developer systems, and will
always be set to not "/tmp" on bots. I'll log a warning if it's set and not
"/tmp".

https://codereview.chromium.org/2859703002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:170:
env['TEMPDIR'] = env['TMPDIR'] = '/tmp'
On 2017/05/03 02:06:36, iannucci wrote:
> On 2017/05/03 01:58:34, mithro wrote:
> > Can you log a warning if 'TEMPDIR' is set in the environment and not equal
to
> > '/tmp'
> 
> Actually... Why set TEMPDIR? That's only used on windows.

I was overriding everything I saw in the log vars, but good to know - I'll just
set TMPDIR.

[dnj, 2017-05-03 19:09:19]

(friendly ping)

[Dirk Pranke, 2017-05-03 20:26:06]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2017-05-03 20:26:06]

lgtm. I agree with mithro@ that a test might be ideal, but I also don't think
it's critical for this.

https://codereview.chromium.org/2859703002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
(right):

https://codereview.chromium.org/2859703002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:167:
_log.info('Overriding TMPDIR to "/tmp" for Xvfb, was: %s', env['TMPDIR'])
Nit: use _log.debug() here.

[dnj, 2017-05-03 22:02:21]

Hey - PTAL again. I added a test and updated "MockExecutive" to track "env". I'm
not sure how far the rabbit hole I should go here, so this is a fairly
non-invasive approach. LMK what you think.

[Dirk Pranke, 2017-05-04 00:05:48]

Thanks for adding the test, still lgtm.

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
(right):

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
return get_args(self.full_calls)
Can't this just be:

  @property
  def calls(self):
    return [fc.args for fc in self.full_calls]

If full_calls isn't a list of MockCalls, you'll get some sort of assertion here,
and isn't that good enough?

[dnj, 2017-05-04 00:20:38]

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
(right):

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
return get_args(self.full_calls)
On 2017/05/04 00:05:48, Dirk Pranke (slow until May 4) wrote:
> Can't this just be:
> 
>   @property
>   def calls(self):
>     return [fc.args for fc in self.full_calls]
> 
> If full_calls isn't a list of MockCalls, you'll get some sort of assertion
here,
> and isn't that good enough?

No, b/c "run_in_parallel" adds lists of MockCall to the "full_calls" list, so
it's at least one level recursive.

[Dirk Pranke, 2017-05-04 00:43:46]

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
(right):

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
return get_args(self.full_calls)
On 2017/05/04 00:20:37, dnj wrote:
> On 2017/05/04 00:05:48, Dirk Pranke (slow until May 4) wrote:
> > Can't this just be:
> > 
> >   @property
> >   def calls(self):
> >     return [fc.args for fc in self.full_calls]
> > 
> > If full_calls isn't a list of MockCalls, you'll get some sort of assertion
> here,
> > and isn't that good enough?
> 
> No, b/c "run_in_parallel" adds lists of MockCall to the "full_calls" list, so
> it's at least one level recursive.

Ah, I see. That's ugly. Can you add a TODO and/or file a bug so we can get rid
of that?

[dnj, 2017-05-04 01:13:17]

On 2017/05/04 00:43:46, Dirk Pranke (slow until May 4) wrote:
>
https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
> (right):
> 
>
https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
> return get_args(self.full_calls)
> On 2017/05/04 00:20:37, dnj wrote:
> > On 2017/05/04 00:05:48, Dirk Pranke (slow until May 4) wrote:
> > > Can't this just be:
> > > 
> > >   @property
> > >   def calls(self):
> > >     return [fc.args for fc in self.full_calls]
> > > 
> > > If full_calls isn't a list of MockCalls, you'll get some sort of assertion
> > here,
> > > and isn't that good enough?
> > 
> > No, b/c "run_in_parallel" adds lists of MockCall to the "full_calls" list,
so
> > it's at least one level recursive.
> 
> Ah, I see. That's ugly. Can you add a TODO and/or file a bug so we can get rid
> of that?

Agreed that it is ugly. Not sure what the TODO / BUG would say, since I'm not
sure it's undesired or a bug :) If you say it is and give me some text, I can
include it for you.

[Dirk Pranke, 2017-05-04 01:24:55]

On 2017/05/04 01:13:17, dnj wrote:
> On 2017/05/04 00:43:46, Dirk Pranke (slow until May 4) wrote:
> >
>
https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
> > File
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
> >
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
> > return get_args(self.full_calls)
> > On 2017/05/04 00:20:37, dnj wrote:
> > > On 2017/05/04 00:05:48, Dirk Pranke (slow until May 4) wrote:
> > > > Can't this just be:
> > > > 
> > > >   @property
> > > >   def calls(self):
> > > >     return [fc.args for fc in self.full_calls]
> > > > 
> > > > If full_calls isn't a list of MockCalls, you'll get some sort of
assertion
> > > here,
> > > > and isn't that good enough?
> > > 
> > > No, b/c "run_in_parallel" adds lists of MockCall to the "full_calls" list,
> so
> > > it's at least one level recursive.
> > 
> > Ah, I see. That's ugly. Can you add a TODO and/or file a bug so we can get
rid
> > of that?
> 
> Agreed that it is ugly. Not sure what the TODO / BUG would say, since I'm not
> sure it's undesired or a bug :) If you say it is and give me some text, I can
> include it for you.

I'm pretty sure I wrote that code and it may have been intentional at the time,
but I'm 
regretting it now :)

Something like 

# TODO(crbug.com/XYZ): Make self.calls always be an array of arrays of
MockCalls, rather than a union type.

would be better.

Thanks for working on this and bearing with the feedback, I know this code isn't
your usual stomping grounds :).

[mithro, 2017-05-04 05:24:17]

The CQ bit was checked by tansell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 05:24:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 06:49:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 06:49:52]

Dry run: This issue passed the CQ dry run.

[dnj, 2017-05-04 16:07:54]

Done. NP, is fun to play with other code bases from time to time :)

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
(right):

https://codereview.chromium.org/2859703002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py:222:
return get_args(self.full_calls)
On 2017/05/04 00:43:46, Dirk Pranke (slow until May 4) wrote:
> On 2017/05/04 00:20:37, dnj wrote:
> > On 2017/05/04 00:05:48, Dirk Pranke (slow until May 4) wrote:
> > > Can't this just be:
> > > 
> > >   @property
> > >   def calls(self):
> > >     return [fc.args for fc in self.full_calls]
> > > 
> > > If full_calls isn't a list of MockCalls, you'll get some sort of assertion
> > here,
> > > and isn't that good enough?
> > 
> > No, b/c "run_in_parallel" adds lists of MockCall to the "full_calls" list,
so
> > it's at least one level recursive.
> 
> Ah, I see. That's ugly. Can you add a TODO and/or file a bug so we can get rid
> of that?

Done.

[dnj, 2017-05-04 16:08:54]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-05-04 16:08:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2859703002/#ps40001
(title: "track mock call env, add test")

[dnj, 2017-05-04 16:09:02]

The CQ bit was unchecked by dnj@chromium.org

[dnj, 2017-05-04 16:14:09]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-05-04 16:14:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2859703002/#ps60001
(title: "todo comment")

[commit-bot: I haz the power, 2017-05-04 16:14:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 17:48:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493914449771100,
"parent_rev": "66a2016758be5a47057b8580f9d0432b7de570f8", "commit_rev":
"9cc81c66fbe4e06f84a5d8647faf1a2fab5ecf38"}

[commit-bot: I haz the power, 2017-05-04 17:48:50]

Description was changed from

==========
webkitpy: Override TEMPDIR for Xvfb.

Hard-code TEMPDIR/TMPDIR for "Xvfb" to "/tmp" in webkitpy's Linux layout
test port. The Xvfb tool's "xkb" library compiles in "/tmp" in several
places, and hard-linking expectations get violated if TEMPDIR and "/tmp"
are on different filesystems.

BUG=chromium:715848
TEST=local
  - Ran without patch and TEMPDIR/TMPDIR overrides, verified hardlink
    failure from bug.
  - Ran with patch, no more hardlink failure!
==========

to

==========
webkitpy: Override TEMPDIR for Xvfb.

Hard-code TEMPDIR/TMPDIR for "Xvfb" to "/tmp" in webkitpy's Linux layout
test port. The Xvfb tool's "xkb" library compiles in "/tmp" in several
places, and hard-linking expectations get violated if TEMPDIR and "/tmp"
are on different filesystems.

BUG=chromium:715848
TEST=local
  - Ran without patch and TEMPDIR/TMPDIR overrides, verified hardlink
    failure from bug.
  - Ran with patch, no more hardlink failure!

Review-Url: https://codereview.chromium.org/2859703002
Cr-Commit-Position: refs/heads/master@{#469378}
Committed:
https://chromium.googlesource.com/chromium/src/+/9cc81c66fbe4e06f84a5d8647faf...
==========

[commit-bot: I haz the power, 2017-05-04 17:48:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9cc81c66fbe4e06f84a5d8647faf...