infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

This is a reland of https://codereview.chromium.org/2102613002/ .

See https://goto.google.com/ljfzu (Google-internal) for more context. Summary:
- the only builders with chromium checkout affected by this are on chromium.fyi
- we can't clean up old cache directories before switching to new ones

Also see https://goto.google.com/oyxwm why so short directory names are OK.
Similarly https://codereview.chromium.org/2061213002 .

BUG=623575
Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/4f00aa0b495e75c0944fcc4cc3188a9c57efaf7d

[Paweł Hajdan Jr., 2016-06-29 07:24:03]

phajdan.jr@chromium.org changed reviewers:
+ emso@chromium.org, sergiyb@chromium.org, tandrii@chromium.org

[Sergiy Byelozyorov, 2016-06-29 07:41:51]

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
File recipe_modules/infra_paths/path_config.py (right):

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
recipe_modules/infra_paths/path_config.py:39: c.base_paths['cache'] = ('/', 'b',
'c')
Although I understand the reason behind this patch and lgtm it, I worry that it
will be harder to understand what are these directories on the bot for those
unfamiliar with this code. Also I am not sure if it's a good long-term solution
as it only shaves off a few characters. If a new directory with long name is
added to Chromium, it will break again. Can you please explain what other
alternative solutions did you consider (passing params in a file?) and why did
you choose this one in particular? Finally, please document these directories in
the Trooper Playbook.

[Paweł Hajdan Jr., 2016-06-29 07:44:18]

Description was changed from

==========
infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

This is a reland of https://codereview.chromium.org/2102613002/ .

See https://goto.google.com/ljfzu (Google-internal) for more context. Summary:
- the only builders with chromium checkout affected by this are on chromium.fyi
- we can't clean up old cache directories before switching to new ones

BUG=623575
==========

to

==========
infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

This is a reland of https://codereview.chromium.org/2102613002/ .

See https://goto.google.com/ljfzu (Google-internal) for more context. Summary:
- the only builders with chromium checkout affected by this are on chromium.fyi
- we can't clean up old cache directories before switching to new ones

Also see https://goto.google.com/oyxwm why so short directory names are OK.
Similarly https://codereview.chromium.org/2061213002 .

BUG=623575
==========

[Paweł Hajdan Jr., 2016-06-29 07:49:26]

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
File recipe_modules/infra_paths/path_config.py (right):

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
recipe_modules/infra_paths/path_config.py:39: c.base_paths['cache'] = ('/', 'b',
'c')
On 2016/06/29 at 07:41:51, Sergiy Byelozyorov wrote:
> Although I understand the reason behind this patch and lgtm it, I worry that
it will be harder to understand what are these directories on the bot for those
unfamiliar with this code.

It is a valid concern. My understanding is people are fine with that tradeoff.
Added even more context to the commit message - we already do similar things
e.g. for remote_run.

> Also I am not sure if it's a good long-term solution as it only shaves off a
few characters. If a new directory with long name is added to Chromium, it will
break again.

It solves an actual problem that these _new_ paths were too long for _existing_
builds that work with other, shorter paths. Above is just as true for the
current situation, and I agree being close to path length limit is not ideal.

> Can you please explain what other alternative solutions did you consider
(passing params in a file?) and why did you choose this one in particular?

Feel free to suggest an alternative patch for
https://bugs.chromium.org/p/chromium/issues/detail?id=623575 .

> Finally, please document these directories in the Trooper Playbook.

I think at most we can link to this code. I'm not sure how useful is
documentation which just repeats the code and may get out of sync with it. Do
you have any suggestions which section of the playbook this might be a good fit
for?

[Paweł Hajdan Jr., 2016-06-29 07:56:33]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2016-06-29 07:56:37]

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, 2016-06-29 08:00:13]

Description was changed from

==========
infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

This is a reland of https://codereview.chromium.org/2102613002/ .

See https://goto.google.com/ljfzu (Google-internal) for more context. Summary:
- the only builders with chromium checkout affected by this are on chromium.fyi
- we can't clean up old cache directories before switching to new ones

Also see https://goto.google.com/oyxwm why so short directory names are OK.
Similarly https://codereview.chromium.org/2061213002 .

BUG=623575
==========

to

==========
infra_paths: keep cache paths super short to avoid long path issues (attempt #2)

This is a reland of https://codereview.chromium.org/2102613002/ .

See https://goto.google.com/ljfzu (Google-internal) for more context. Summary:
- the only builders with chromium checkout affected by this are on chromium.fyi
- we can't clean up old cache directories before switching to new ones

Also see https://goto.google.com/oyxwm why so short directory names are OK.
Similarly https://codereview.chromium.org/2061213002 .

BUG=623575

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/4f00aa0b495e75...
==========

[commit-bot: I haz the power, 2016-06-29 08:00:14]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/4f00aa0b495e75...

[Sergiy Byelozyorov, 2016-06-29 08:31:48]

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
File recipe_modules/infra_paths/path_config.py (right):

https://codereview.chromium.org/2102403002/diff/1/recipe_modules/infra_paths/...
recipe_modules/infra_paths/path_config.py:39: c.base_paths['cache'] = ('/', 'b',
'c')
On 2016/06/29 07:49:26, Paweł Hajdan Jr. wrote:
> On 2016/06/29 at 07:41:51, Sergiy Byelozyorov wrote:
> > Although I understand the reason behind this patch and lgtm it, I worry that
> it will be harder to understand what are these directories on the bot for
those
> unfamiliar with this code.
> 
> It is a valid concern. My understanding is people are fine with that tradeoff.
> Added even more context to the commit message - we already do similar things
> e.g. for remote_run.

I've replied on the thread.

> 
> > Also I am not sure if it's a good long-term solution as it only shaves off a
> few characters. If a new directory with long name is added to Chromium, it
will
> break again.
> 
> It solves an actual problem that these _new_ paths were too long for
_existing_
> builds that work with other, shorter paths. Above is just as true for the
> current situation, and I agree being close to path length limit is not ideal.

Agreed, we need to fix it now. But eventually we'll hit the problem again. I
suggest filing a bug to find a long-term solution with long paths and adding a
TODO here to rename directories back once we've solved this.

> > Can you please explain what other alternative solutions did you consider
> (passing params in a file?) and why did you choose this one in particular?
> 
> Feel free to suggest an alternative patch for
> https://bugs.chromium.org/p/chromium/issues/detail?id=623575 .

I've found an alternative solution and posted it to the bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=623575#c15.

> > Finally, please document these directories in the Trooper Playbook.
> 
> I think at most we can link to this code. I'm not sure how useful is
> documentation which just repeats the code and may get out of sync with it. Do
> you have any suggestions which section of the playbook this might be a good
fit
> for?

When a trooper sees a directory C:\b\c\b or /b/c/b, he doesn't know where to
look other than Trooper Playbook. Finding this particular piece of code is non
trivial because the path is not explicitly mentioned and search simply for "b"
or "c" is going to return too many matches. I suggest creating a section
"remote_run" in Handling failures and adding there something like this:

Some directories on the bots have shorter names to avoid long-path problem on
Windows. The following directories exist under b-dir:
 - c - cache
 - c/b - build_cache