Increase max WPT import path length to 160.

Increase max WPT import path length to 160.

Review-Url: https://codereview.chromium.org/2708883004
Cr-Commit-Position: refs/heads/master@{#453281}
Committed: https://chromium.googlesource.com/chromium/src/+/a3547c0e56d617c5a82dcd0d8a128f4c2c3f921f

[qyearsley, 2017-02-21 23:29:09]

qyearsley@chromium.org changed reviewers:
+ foolip@chromium.org

[qyearsley, 2017-02-21 23:29:10]

I noticed that in the recent web-platform-tests imports, many or most of the
tests in wpt/referrer-policy/ are skipped due to long paths (Example logs:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fw3...).

I was going to skip these tests or suggest renaming them, but I didn't see any
really obvious names to shorten, so I decided to check whether it might be
reasonable to increase the max path length.

Based on the fact that the Chromium repo base dir on "WebKit Win7" is just
"E:\b\c\b\win_layout\src", it looks like we might have up to 180 characters of
slack to allow for WPT test paths. Changing it to 160 characters for now, which
should allow all of the referrer-policy tests to be imported.

[qyearsley, 2017-02-21 23:29:32]

Description was changed from

==========
Increase max import path length to 160.
==========

to

==========
Increase max WPT import path length to 160.
==========

[qyearsley, 2017-02-21 23:29:37]

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

[commit-bot: I haz the power, 2017-02-21 23:31:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-02-21 23:45:03]

On 2017/02/21 at 23:29:10, qyearsley wrote:
> I noticed that in the recent web-platform-tests imports, many or most of the
tests in wpt/referrer-policy/ are skipped due to long paths (Example logs:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fw3...).
> 
> I was going to skip these tests or suggest renaming them, but I didn't see any
really obvious names to shorten, so I decided to check whether it might be
reasonable to increase the max path length.
> 
> Based on the fact that the Chromium repo base dir on "WebKit Win7" is just
"E:\b\c\b\win_layout\src", it looks like we might have up to 180 characters of
slack to allow for WPT test paths. Changing it to 160 characters for now, which
should allow all of the referrer-policy tests to be imported.

According to https://bugs.chromium.org/p/chromium/issues/detail?id=535615#c2,
"E:\b\build\slave\windows-content-browser-drm-full-6\build\src" caused the
problem.  What's the longest Windows bot name nowadays?

[qyearsley, 2017-02-21 23:54:55]

qyearsley@chromium.org changed reviewers:
+ tkent@chromium.org

[qyearsley, 2017-02-22 00:11:35]

On 2017/02/21 at 23:45:03, tkent wrote:
> On 2017/02/21 at 23:29:10, qyearsley wrote:
> > I noticed that in the recent web-platform-tests imports, many or most of the
tests in wpt/referrer-policy/ are skipped due to long paths (Example logs:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fw3...).
> > 
> > I was going to skip these tests or suggest renaming them, but I didn't see
any really obvious names to shorten, so I decided to check whether it might be
reasonable to increase the max path length.
> > 
> > Based on the fact that the Chromium repo base dir on "WebKit Win7" is just
"E:\b\c\b\win_layout\src", it looks like we might have up to 180 characters of
slack to allow for WPT test paths. Changing it to 160 characters for now, which
should allow all of the referrer-policy tests to be imported.
> 
> According to https://bugs.chromium.org/p/chromium/issues/detail?id=535615#c2,
"E:\b\build\slave\windows-content-browser-drm-full-6\build\src" caused the
problem. What's the longest Windows bot name nowadays?

Ah, right. By default the checkout dir is related to the builder name, but it
may be configured to be something else
(https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_...).

There are some pretty long builder names listed in
https://chrome-internal.googlesource.com/infradata/hosts/+/master/buildermap.md#,
but for the chromium.webkit and tryserver.blink masters, the checkout dirs are
set to be shorter.

Do you think it might not be a good idea to to change this?

Also, jochen@/estark@: do you know if you might be able to shorten some paths in
wpt/referrer-policy (e.g.
"wpt/referrer-policy/strict-origin-when-cross-origin/http-rp/same-origin/http-https/fetch-request/upgrade-protocol.keep-origin-redirect.http.html.headers")?

[tkent, 2017-02-22 00:21:26]

> > According to
https://bugs.chromium.org/p/chromium/issues/detail?id=535615#c2,
"E:\b\build\slave\windows-content-browser-drm-full-6\build\src" caused the
problem. What's the longest Windows bot name nowadays?
> 
> Ah, right. By default the checkout dir is related to the builder name, but it
may be configured to be something else
(https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium_...).
> 
> There are some pretty long builder names listed in
https://chrome-internal.googlesource.com/infradata/hosts/+/master/buildermap.md#,
but for the chromium.webkit and tryserver.blink masters, the checkout dirs are
set to be shorter.
> 
> Do you think it might not be a good idea to to change this?

I'm not sure.  We need to make sure what's the longest Windows builder name
which checks out chromium src repository.  The issue affects non-layouttest
builders.

[commit-bot: I haz the power, 2017-02-22 01:38:12]

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

[commit-bot: I haz the power, 2017-02-22 01:38:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[foolip, 2017-02-23 02:38:07]

I'll leave this review to tkent since you've already started the discussion.

[qyearsley, 2017-02-23 19:38:20]

On 2017/02/23 at 02:38:07, foolip wrote:
> I'll leave this review to tkent since you've already started the discussion.

Alright - There's nothing really urgent here, the only problem is that there are
now many tests in wpt/referrer-policy that are skipped because of long paths.

Unless we can determine for sure what the longest windows builder name is for
any builder that may run layout tests, we could:

 1. Do nothing, continue skipping tests with long paths on each import
 2. Increase the limit and observe if problems occur (increasing to 150 would
allow those current referrer-policy tests to get imported). If problems occur,
we could change the checkout dir for any builders that have problems.
 3. Rename directories/files to decrease path length.
 4. Explicitly skip directories in NeverFixTests and file a bug, to make it
clear which tests are never imported.

I'm currently a bit in favor of option 2; but any of these is OK. tkent@, what
do you think?

[foolip, 2017-02-23 19:41:00]

Is it tricky to figure out how long the relevant builder names are before making
the change? If the new limit is too high, will everything blow up?

[qyearsley, 2017-02-23 19:51:58]

On 2017/02/23 at 19:41:00, foolip wrote:
> Is it tricky to figure out how long the relevant builder names are before
making the change? If the new limit is too high, will everything blow up?

It's probably not tricky, I'm just not 100% sure whether I'm missing anything.

The builders that I know for sure are running layout tests are:
 - continuous: chromium.webkit
 - try server: tryserver.blink
 - a couple builders on chromium.fyi
 - and the builders linux_chromium_rel_ng, mac_chromium_rel_ng,
win_chromium_rel_ng, android_blink_rel, and v8_linux_blink_rel.

A limit of 160 shouldn't be a problem for all of these.

If the new limit is too high and there's some builder somewhere that I don't
know about that has a super long name that runs layout tests, then it could
start failing and theoretically it might be hard for us to find out about it.

[tkent, 2017-02-24 00:43:23]

On 2017/02/23 at 19:38:20, qyearsley wrote:
> On 2017/02/23 at 02:38:07, foolip wrote:
> > I'll leave this review to tkent since you've already started the discussion.
> 
> Alright - There's nothing really urgent here, the only problem is that there
are now many tests in wpt/referrer-policy that are skipped because of long
paths.
> 
> Unless we can determine for sure what the longest windows builder name is for
any builder that may run layout tests, we could:
> 
>  1. Do nothing, continue skipping tests with long paths on each import
>  2. Increase the limit and observe if problems occur (increasing to 150 would
allow those current referrer-policy tests to get imported). If problems occur,
we could change the checkout dir for any builders that have problems.
>  3. Rename directories/files to decrease path length.
>  4. Explicitly skip directories in NeverFixTests and file a bug, to make it
clear which tests are never imported.
> 
> I'm currently a bit in favor of option 2; but any of these is OK. tkent@, what
do you think?

Re: 2
I wont't stop trying it.  I recommend adding a dummy file with long name rather
than landing this CL.
However, please don't forget this issue affects all Windows bots checking out
chromium repository.  If we have too long file names in chromium repository,
|git checkout/pull| will fail.

Re: 3
Now we are discussing moving third_party/WebKit to elsewhere.
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...

[foolip, 2017-02-24 04:39:46]

On 2017/02/24 00:43:23, tkent wrote:
> On 2017/02/23 at 19:38:20, qyearsley wrote:
> > On 2017/02/23 at 02:38:07, foolip wrote:
> > > I'll leave this review to tkent since you've already started the
discussion.
> > 
> > Alright - There's nothing really urgent here, the only problem is that there
> are now many tests in wpt/referrer-policy that are skipped because of long
> paths.
> > 
> > Unless we can determine for sure what the longest windows builder name is
for
> any builder that may run layout tests, we could:
> > 
> >  1. Do nothing, continue skipping tests with long paths on each import
> >  2. Increase the limit and observe if problems occur (increasing to 150
would
> allow those current referrer-policy tests to get imported). If problems occur,
> we could change the checkout dir for any builders that have problems.
> >  3. Rename directories/files to decrease path length.
> >  4. Explicitly skip directories in NeverFixTests and file a bug, to make it
> clear which tests are never imported.
> > 
> > I'm currently a bit in favor of option 2; but any of these is OK. tkent@,
what
> do you think?
> 
> Re: 2
> I wont't stop trying it.  I recommend adding a dummy file with long name
rather
> than landing this CL.
> However, please don't forget this issue affects all Windows bots checking out
> chromium repository.  If we have too long file names in chromium repository,
> |git checkout/pull| will fail.

Do you mean to add a simple dummy test somewhere in LayoutTests/external/wpt (or
elsewhere) and see how long the filename can be made without failing some bot in
dry runs, and then using that as the limit? That sounds safer than committing
and waiting for a wpt import to see breakage, but I suppose non-CQ bots could
still have problems. Did you mean to land and keep such a dummy test, as a
protection against some bots growing a longer name and breaking things?

> Re: 3
> Now we are discussing moving third_party/WebKit to elsewhere.
>
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...

This is presumably more than a few weeks away, if we can get additional wpt
coverage in the interim with a small amount of work I think that seems
worthwhile.

[tkent, 2017-02-24 05:07:42]

On 2017/02/24 at 04:39:46, foolip wrote:
> Do you mean to add a simple dummy test somewhere in LayoutTests/external/wpt
(or elsewhere) and see how long the filename can be made without failing some
bot in dry runs, and then using that as the limit? That sounds safer than
committing and waiting for a wpt import to see breakage, but I suppose non-CQ
bots could still have problems. Did you mean to land and keep such a dummy test,
as a protection against some bots growing a longer name and breaking things?

I mean landing a dummy file. Try bots can't catch the issue because their names
are not long.

> > Re: 3
> > Now we are discussing moving third_party/WebKit to elsewhere.
> >
https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-...
> 
> This is presumably more than a few weeks away, if we can get additional wpt
coverage in the interim with a small amount of work I think that seems
worthwhile.

Yeah, even if we get agreement, actual movement would happen in Q2 or later.

[foolip, 2017-02-24 05:10:25]

On 2017/02/24 05:07:42, tkent wrote:
> On 2017/02/24 at 04:39:46, foolip wrote:
> > Do you mean to add a simple dummy test somewhere in LayoutTests/external/wpt
> (or elsewhere) and see how long the filename can be made without failing some
> bot in dry runs, and then using that as the limit? That sounds safer than
> committing and waiting for a wpt import to see breakage, but I suppose non-CQ
> bots could still have problems. Did you mean to land and keep such a dummy
test,
> as a protection against some bots growing a longer name and breaking things?
> 
> I mean landing a dummy file. Try bots can't catch the issue because their
names
> are not long.

So, if we get it wrong, there will still some red in the waterfall. I guess
doing this and watching the waterfall would work. WDYT, qyearsley@?

[qyearsley, 2017-02-24 19:06:50]

On 2017/02/24 at 05:10:25, foolip wrote:
> On 2017/02/24 05:07:42, tkent wrote:
> > On 2017/02/24 at 04:39:46, foolip wrote:
> > > Do you mean to add a simple dummy test somewhere in
LayoutTests/external/wpt
> > (or elsewhere) and see how long the filename can be made without failing
some
> > bot in dry runs, and then using that as the limit? That sounds safer than
> > committing and waiting for a wpt import to see breakage, but I suppose
non-CQ
> > bots could still have problems. Did you mean to land and keep such a dummy
test,
> > as a protection against some bots growing a longer name and breaking things?
> > 
> > I mean landing a dummy file. Try bots can't catch the issue because their
names
> > are not long.
> 
> So, if we get it wrong, there will still some red in the waterfall. I guess
doing this and watching the waterfall would work. WDYT, qyearsley@?

That sounds reasonable. CL: https://codereview.chromium.org/2718733002

It might be good to try to commit that when I'm certain to be around watching
the waterfall and there are not many other commits. Would it be OK to try that
on Saturday (if I can be watching at the time of commit?)

[foolip, 2017-02-26 14:54:31]

On 2017/02/24 19:06:50, qyearsley wrote:
> On 2017/02/24 at 05:10:25, foolip wrote:
> > On 2017/02/24 05:07:42, tkent wrote:
> > > On 2017/02/24 at 04:39:46, foolip wrote:
> > > > Do you mean to add a simple dummy test somewhere in
> LayoutTests/external/wpt
> > > (or elsewhere) and see how long the filename can be made without failing
> some
> > > bot in dry runs, and then using that as the limit? That sounds safer than
> > > committing and waiting for a wpt import to see breakage, but I suppose
> non-CQ
> > > bots could still have problems. Did you mean to land and keep such a dummy
> test,
> > > as a protection against some bots growing a longer name and breaking
things?
> > > 
> > > I mean landing a dummy file. Try bots can't catch the issue because their
> names
> > > are not long.
> > 
> > So, if we get it wrong, there will still some red in the waterfall. I guess
> doing this and watching the waterfall would work. WDYT, qyearsley@?
> 
> That sounds reasonable. CL: https://codereview.chromium.org/2718733002
> 
> It might be good to try to commit that when I'm certain to be around watching
> the waterfall and there are not many other commits. Would it be OK to try that
> on Saturday (if I can be watching at the time of commit?)

It's Sunday now, but that sounds like a good idea.

[qyearsley, 2017-02-27 17:41:57]

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

[commit-bot: I haz the power, 2017-02-27 17:42:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2017-02-27 17:43:54]

On 2017/02/26 at 14:54:31, foolip wrote:
> On 2017/02/24 19:06:50, qyearsley wrote:
> > On 2017/02/24 at 05:10:25, foolip wrote:
> > > On 2017/02/24 05:07:42, tkent wrote:
> > > > On 2017/02/24 at 04:39:46, foolip wrote:
> > > > > Do you mean to add a simple dummy test somewhere in
> > LayoutTests/external/wpt
> > > > (or elsewhere) and see how long the filename can be made without failing
> > some
> > > > bot in dry runs, and then using that as the limit? That sounds safer
than
> > > > committing and waiting for a wpt import to see breakage, but I suppose
> > non-CQ
> > > > bots could still have problems. Did you mean to land and keep such a
dummy
> > test,
> > > > as a protection against some bots growing a longer name and breaking
things?
> > > > 
> > > > I mean landing a dummy file. Try bots can't catch the issue because
their
> > names
> > > > are not long.
> > > 
> > > So, if we get it wrong, there will still some red in the waterfall. I
guess
> > doing this and watching the waterfall would work. WDYT, qyearsley@?
> > 
> > That sounds reasonable. CL: https://codereview.chromium.org/2718733002
> > 
> > It might be good to try to commit that when I'm certain to be around
watching
> > the waterfall and there are not many other commits. Would it be OK to try
that
> > on Saturday (if I can be watching at the time of commit?)
> 
> It's Sunday now, but that sounds like a good idea.

Alright, on the weekend I landed the dummy file
(https://codereview.chromium.org/2718733002), and watched the waterfall (just
the builders shown by default on https://build.chromium.org/), and I didn't find
any failures caused by the long path name, so I believe that this change should
be safe to commit.

Might there be some builders I forgot to check? Otherwise, is this CL OK to
commit?

[foolip, 2017-02-27 18:23:34]

On 2017/02/27 17:43:54, qyearsley wrote:
> On 2017/02/26 at 14:54:31, foolip wrote:
> > On 2017/02/24 19:06:50, qyearsley wrote:
> > > On 2017/02/24 at 05:10:25, foolip wrote:
> > > > On 2017/02/24 05:07:42, tkent wrote:
> > > > > On 2017/02/24 at 04:39:46, foolip wrote:
> > > > > > Do you mean to add a simple dummy test somewhere in
> > > LayoutTests/external/wpt
> > > > > (or elsewhere) and see how long the filename can be made without
failing
> > > some
> > > > > bot in dry runs, and then using that as the limit? That sounds safer
> than
> > > > > committing and waiting for a wpt import to see breakage, but I suppose
> > > non-CQ
> > > > > bots could still have problems. Did you mean to land and keep such a
> dummy
> > > test,
> > > > > as a protection against some bots growing a longer name and breaking
> things?
> > > > > 
> > > > > I mean landing a dummy file. Try bots can't catch the issue because
> their
> > > names
> > > > > are not long.
> > > > 
> > > > So, if we get it wrong, there will still some red in the waterfall. I
> guess
> > > doing this and watching the waterfall would work. WDYT, qyearsley@?
> > > 
> > > That sounds reasonable. CL: https://codereview.chromium.org/2718733002
> > > 
> > > It might be good to try to commit that when I'm certain to be around
> watching
> > > the waterfall and there are not many other commits. Would it be OK to try
> that
> > > on Saturday (if I can be watching at the time of commit?)
> > 
> > It's Sunday now, but that sounds like a good idea.
> 
> Alright, on the weekend I landed the dummy file
> (https://codereview.chromium.org/2718733002), and watched the waterfall (just
> the builders shown by default on https://build.chromium.org/), and I didn't
find
> any failures caused by the long path name, so I believe that this change
should
> be safe to commit.
> 
> Might there be some builders I forgot to check? Otherwise, is this CL OK to
> commit?

Seems like enough caution to me, LGTM and good luck!

[qyearsley, 2017-02-27 18:50:30]

The CQ bit was unchecked by qyearsley@chromium.org

[qyearsley, 2017-02-27 18:50:32]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2017-02-27 18:50:56]

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-02-27 19:08:34]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1488221432126730, "parent_rev":
"56ce72b787bcd211e206ec4d49bb06f07590ca3f", "commit_rev":
"a3547c0e56d617c5a82dcd0d8a128f4c2c3f921f"}

[commit-bot: I haz the power, 2017-02-27 19:09:14]

Description was changed from

==========
Increase max WPT import path length to 160.
==========

to

==========
Increase max WPT import path length to 160.

Review-Url: https://codereview.chromium.org/2708883004
Cr-Commit-Position: refs/heads/master@{#453281}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3547c0e56d617c5a82dcd0d8a12...
==========

[commit-bot: I haz the power, 2017-02-27 19:09:15]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/a3547c0e56d617c5a82dcd0d8a12...

[tkent, 2017-02-28 04:30:59]

WPT lint tool rejects 150+ path names. We don't need MAX_PATH_LENGTH check
entirely now.
https://github.com/w3c/wpt-tools/blob/95b0f836657b5bab6064850bdb6f92b4a71f574...

We can remove the section of crbug.com/535615 from W3CImportExceptions.

[foolip, 2017-02-28 04:58:43]

On 2017/02/28 04:30:59, tkent wrote:
> WPT lint tool rejects 150+ path names. We don't need MAX_PATH_LENGTH check
> entirely now.
>
https://github.com/w3c/wpt-tools/blob/95b0f836657b5bab6064850bdb6f92b4a71f574...
> 
> We can remove the section of crbug.com/535615 from W3CImportExceptions.

Wow, that's great! And it looks like the lint check is actually working, the
longest path is now 149:

referrer-policy/strict-origin-when-cross-origin/http-rp/cross-origin/http-https/fetch-request/upgrade-protocol.swap-origin-redirect.http.html.headers