Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 5 months ago by Paweł Hajdan Jr. Modified:
4 years, 5 months ago Reviewers:
Sergiy Byelozyorov, emso, tandrii(chromium) CC:
chromium-reviews, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Target Ref:
refs/heads/master Project:
depot_tools Visibility:
Public. |
Descriptioninfra_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
Patch Set 1 #
Total comments: 3
Created: 4 years, 5 months ago
Messages
Total messages: 10 (4 generated)
phajdan.jr@chromium.org changed reviewers: + emso@chromium.org, sergiyb@chromium.org, tandrii@chromium.org
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.
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 ==========
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?
The CQ bit was checked by phajdan.jr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/tools/depot_tools/+/4f00aa0b495e75...
Message was sent while issue was closed.
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
