New issue
Advanced search Search tips

Issue 839183 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug

Blocking:
issue 831847



Sign in to add a comment

Shorten virtual test names

Project Member Reported by martiniss@chromium.org, May 3 2018

Issue description

Currently, "virtual" layout tests can sometimes have longer than the standard 150 character limit for web platform tests.

This creates problems on windows, since there's a max path limit on windows.

 bug 831847  is also related.
 
Owner: robertma@chromium.org
Status: Assigned (was: Unconfirmed)
Summary: Shorten virtual test names (was: Change virtual test names to )

Comment 2 by kbr@chromium.org, May 4 2018

Blockedon: 831847
Blockedon: -831847
Blocking: 831847
Components: Blink>Infra
Labels: -Pri-3 Pri-2
I this is blocking, not blocked on,  issue 831847 , right?

@martiniss I'm trying to figure out the maximum length of prefixes (names) for virtual test suites. One problem is that the current path prefix on Swarming is way too long:

c:\\users\\chrome~1\\appdata\\local\\temp\\tmpf8qo2a\\11\\layout-test-results

This is 70+ characters, which is a large portion of the 260-character length limit on Windows. And WPT itself (the part after external/wpt) can take up to 150, so I'm a bit hand tied...

Is it possible to write the layout test results to some other, shorter path instead?
Labels: -OS-Linux OS-Windows
Cc: mar...@chromium.org
maruel@, is there any way to shorten the swarming temp location?

That's the default windows 7 temp directory, I think (that's what a quick google search was). 
The Swarming bot overrides the %TEMP% environment variable to a short name. Please use it. Python's tempfile should do the right thing by default.

https://cs.chromium.org/chromium/infra/luci/client/run_isolated.py?l=390

I'm going to look at this
I think this is breaking on buildbot, not swarming. So it doesn't have the same temp directory that swarming does. :/
Yeah, https://logs.chromium.org/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2Fwin7_chromium_rel_ng%2F162837%2F%2B%2Frecipes%2Fsteps%2Fwebkit_layout_tests__with_patch__experimental_%2F0%2Fstdout is the failure we're seeing. This is running on buildbot

This might be fixed on LUCI? Since the bots are running on swarming and not buildbot. I'll look and see if that's the case today.
It is better on LUCI. LUCI command arguments include

 '--task-output-dir',
 'c:\\b\\swarming\\w\\ir\\tmp\\tmp513sza',


Buildbot is

 '--task-output-dir',
 'c:\\users\\chrome~1\\appdata\\local\\temp\\tmp6prls5',

That's about 14 characters, which is something. I'm switching the bot to LUCI, but we're running into capacity issues which means it might not be super quick to do. 
So, here are the things we can change I think:

* Shorten the LUCI command arguments.
  It's already pretty short, although I don't know why it's \\b\\swarming and not \\b\s. Other than that I don't think we can shorten it more.
* Shorten '\\10\\layout-test-results\\'
  This is in between the task output directory and the test name. Theoretically we could shorten it to 'LTR', 'results', something like that.
Cc: dpranke@chromium.org
Thanks for the investigation, martiniss@!

It'd be great if we could shorten \b\swarming to \b\s, remove the \tmp\ component from the path, or use a much shorter tmp path outside of the Swarming directory, etc. We don't have much safety margin here, so each character counts. Yet I understand this might be hard as some modifications in LUCI/Swarming might be needed to properly upload and clean up the results.

As for \layout-test-results\, this directory can be changed by changing the --results-directory option given to r_w_t, but I suspect we might have accidentally hard-coded this path in some places and would need to at least examine related recipes, blinkpy & test-results server. +dpranke , is there any other concern for shortening this directory name?
Every year, we reduce the path length on the bots, every year, the layout test consumes these characters.

This is in fact much worse than that, because right now this effectively makes it impossible for developers running on Windows to run layout tests locally. While I'm happy to have the bots have extremely short path, the end story is still that developers won't override their %TEMP% to run layout tests.
We shouldn't need to shorten \b\swarming to \b\s. IIRC, We already recommend devs set TEMP to something under c:\src. The failure here is that we're not limiting directory lengths for virtual test suites, which should be pretty easy to rename.

That said, maruel@ raises some good points, particularly about devs having issues w/ running locally.

If we're fairly confident that the problem only shows up in blinkpy code, then I would be okay with investigating maruel's python hack. I don't really understand it myself (I haven't looked at it), so it's hard for me to assess how safe/reliable it will be.
I'm fine playing semi whack-a-mole with path length stuff; I'm ok landing a few CLs to fix it.

The python hack isn't exactly a hack; https://msdn.microsoft.com/en-us/library/aa365247.aspx#maxpath says that it's officially supported. It's a bit hard to make it work everywhere. I think the CL I originally posted should work for blinkpy code, although it might take a bit to make it fully work.
Okay, having looked at the python change (I apologize for calling it a hack), I think we could probably solve this much more easily just in the context of blinkpy, since all of the i/o should be happening through blinkpy.common.system.filesystem , as per martiniss' CL in https://crrev.com/c/1038431 .
Ping from your friendly neighbourhood ecosystem infra rotation. robertma@chromium.org and dpranke@chromium.org I noticed that the above-linked CL was abandoned. Is this affecting a sizable number of tests on Windows?
Ping again robertma@, should this really be a P2, is anything bad happening?

Sign in to add a comment