Darn.

I would prefer truncating the directory owners part rather than totally removing it, since it's useful in deciding who to contact if you think that something looks unexpected.

I'm also not so sure that the "note to sheriffs" is so useful now... as long as the imports are frequent, reverting by sheriffs is OK. That could probably be replaced with a shorter explanation like: "This is an automatically-generated CL. See <https://some-short-link>. Contact <ecosystem-infra@chromium.org> in case of problems."

We will run into this problem again sooner or later if we simply try to simplify the commit message.

We should use "--message=MESSAGE_FILE" instead.

> We should use "--message=MESSAGE_FILE" instead.

Sorry, "--message-file=MESSAGE_FILE"

Robert's suggestion makes sense. I'm not going to stick around for much longer today, so I'll just cross my fingers and hope everything will be back to normal when I come back tomorrow, otherwise I can look into it next morning :-)

Starting to work on it right now as it's blocking the import.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f1ec6a94694e0b9cdd06337393ff01492e38df01

commit f1ec6a94694e0b9cdd06337393ff01492e38df01
Author: Robert Ma <robertma@chromium.org>
Date: Tue Oct 31 20:37:10 2017

Write CL description into a temporary file

Also add open_text_tempfile() to FileSystem and its mock.

Bug:  780055 
Change-Id: Ib6083639ab3774b468d6b6dc1cb41d818a1b0651
Reviewed-on: https://chromium-review.googlesource.com/747522
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512945}
[modify] https://crrev.com/f1ec6a94694e0b9cdd06337393ff01492e38df01/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem.py
[modify] https://crrev.com/f1ec6a94694e0b9cdd06337393ff01492e38df01/third_party/WebKit/Tools/Scripts/webkitpy/common/system/filesystem_mock.py
[modify] https://crrev.com/f1ec6a94694e0b9cdd06337393ff01492e38df01/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py

Hmm, imports are still failing the same way, so the problem might actually be elsewhere...

Yeah -- the error looks like:

webkitpy.common.system.executive.ScriptError: Failed to run "['git', 'cl', 'upload', '-f', '--gerrit', '--message-file', '/mnt/data/b/rr/tmpt2cvXo/t/tmpz2X7Xt', '--tbrs', u'leon.han@intel.com', '--cc', 'robertma@chromium.org']" exit_code: 1
output: Last 500 characters of output:
a/b/depot_tools/subprocess2.py", line 484, in check_call_out
    out, returncode = communicate(args, **kwargs)

Note that the error is actually in depot_tools -- I suspect it's happening when creating a subprocess somewhere inside git cl. Maybe this is a bug that could be fixed it depot_tools? Maybe it could be reproduced by invoking git cl upload --message-file foo where foo contains a couple pages of text?

Oh well...

Yes, I can reproduce the issue locally with a large commit message in the message file. Here's the full traceback:

Traceback (most recent call last):
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 6132, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 6114, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/usr/local/google/home/robertma/tools/depot_tools/subcommand.py", line 252, in execute
    return command(parser, args[1:])
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 4976, in CMDupload
    return cl.CMDUpload(options, args, orig_args)
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 1665, in CMDUpload
    ret = self.CMDUploadChange(options, git_diff_args, custom_cl_base, change)
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 3008, in CMDUploadChange
    '-m', change_desc.description]).strip()
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 136, in RunGit
    return RunCommand(['git'] + args, **kwargs)
  File "/usr/local/google/home/robertma/tools/depot_tools/git_cl.py", line 124, in RunCommand
    return subprocess2.check_output(args, shell=shell, **kwargs)
  File "/usr/local/google/home/robertma/tools/depot_tools/subprocess2.py", line 524, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File "/usr/local/google/home/robertma/tools/depot_tools/subprocess2.py", line 484, in check_call_out
    out, returncode = communicate(args, **kwargs)
  File "/usr/local/google/home/robertma/tools/depot_tools/subprocess2.py", line 458, in communicate
    proc = Popen(args, **kwargs)
  File "/usr/local/google/home/robertma/tools/depot_tools/subprocess2.py", line 262, in __init__
    % (str(e), kwargs.get('cwd'), args[0]))
OSError: Execution failed with error: [Errno 7] Argument list too long.
Check that None or git exist and have execution permission.

Seems like depot_toos simply loads the content of the message file, after which it's just the same as -m:
https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?l=4963&rcl=47b67c426b1d7120803e09f570e1cca312b34249
https://cs.chromium.org/chromium/tools/depot_tools/git_cl.py?l=3008&rcl=47b67c426b1d7120803e09f570e1cca312b34249

So I think, on one hand, this is definitely a bug in depot_tools -- usually the primary motivation of passing a path instead of content in the argument is to work around the command line length limit -- and the current implementation of depot_tools defeats that goal. (And hence I think we should still keep using the temporary file, even though it doesn't currently fix the bug.) On the other hand, let's cut down the message length for now to unblock the import.

Fixing git_cl.py may take a little bit longer as you said; I've sent https://chromium-review.googlesource.com/c/chromium/src/+/749062 to shorten our CL message and work around this specific bug for now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4229d6df7bed6aa32ad57a98afd9a6f1918c468

commit d4229d6df7bed6aa32ad57a98afd9a6f1918c468
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Wed Nov 01 12:20:20 2017

WPT import: Shorten CL commit message.

We are currently hitting an "argument list too long" error in git-cl upload
that is blocking imports.

At least temporarily, stop mentioning the owners for the directories being
changed, and trim the "note to sheriffs" so that the CL message gets
shorter.

Bug:  780055 
Change-Id: Icf65268c5f8429c1b18d327d320a5731b1bd0784
Reviewed-on: https://chromium-review.googlesource.com/749062
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513109}
[modify] https://crrev.com/d4229d6df7bed6aa32ad57a98afd9a6f1918c468/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/d4229d6df7bed6aa32ad57a98afd9a6f1918c468/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Actually, I now wonder if that was the right fix. Since the last import, https://github.com/w3c/web-platform-tests/pull/8011 was merged, at it renames 12k+ files. I will do a manual import today, since I'm pretty sure a bunch of adjustments will be needed.

You're right after all.

I've done a manual import locally, and can reproduce the upload failure locally. It turns out that the "argument list too long" error comes from git_cl running the presubmit hooks: one of the hooks calls "//tools/checkperms/checkperms.py --root [...] --file [...]", with --file <file> being repeated for each file in the commit. That's around 263000 characters!

Additionally, the imports are taking so long to fail because the watchlist processing also seems to be taking an awful lot of time.

For now, I'll revert the CL above since it doesn't fix anything.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25df2b1af2725ae7f88034cf0fbce04b6ccfe529

commit 25df2b1af2725ae7f88034cf0fbce04b6ccfe529
Author: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Date: Wed Nov 01 15:09:48 2017

Revert "WPT import: Shorten CL commit message."

This reverts commit d4229d6df7bed6aa32ad57a98afd9a6f1918c468.

Reason for revert: does not fix the underlying issue.

Original change's description:
> WPT import: Shorten CL commit message.
> 
> We are currently hitting an "argument list too long" error in git-cl upload
> that is blocking imports.
> 
> At least temporarily, stop mentioning the owners for the directories being
> changed, and trim the "note to sheriffs" so that the CL message gets
> shorter.
> 
> Bug:  780055 
> Change-Id: Icf65268c5f8429c1b18d327d320a5731b1bd0784
> Reviewed-on: https://chromium-review.googlesource.com/749062
> Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
> Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#513109}

TBR=qyearsley@chromium.org,raphael.kubo.da.costa@intel.com,foolip@chromium.org,robertma@chromium.org

Change-Id: I4b4141acad7380aaecf16f63e627eb8777202578
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  780055 
Reviewed-on: https://chromium-review.googlesource.com/749183
Reviewed-by: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#513129}
[modify] https://crrev.com/25df2b1af2725ae7f88034cf0fbce04b6ccfe529/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/25df2b1af2725ae7f88034cf0fbce04b6ccfe529/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Regarding comment 14, that sounds like a different failure. My local reproduction passed presubmit checks successfully but failed at commit-tree.

Could you paste the full error message here?

I ran git-cl upload with -vv, so I got a 6.2M file in the end. I'm attaching a compressed version here.

This is what I ran after creating a local import commit:
python -u /data/src/depot_tools/git_cl.py upload -vv -f --gerrit --message-file /tmp/tmpMF09ce --tbrs foolip --cc robertma@chromium.org --bypass-watchlist

The only difference compared to what test_import.py does is --bypass-watchlist. Like I said, the watchlist stuff was taking an insane amount of time before I even got to the point where I'd get the same failure the bot's getting.

Deleted: upload.log.xz 139 KB

upload.log.xz 139 KB Download

Thanks, Raphael.

Comparing the two stack trace, we are failing even earlier now. In CMDUpload, the previous failure was near the end, when actually trying to upload and the commit message got appended to the command line.

Now, we are crashing at RunHook where the full list of changed files is somehow thrown into command line... 

There are two bugs so far:
1. A presubmit hook (checkperm) takes the full list of changed files via command line
2. Commit message is always passed via command line under the hood, no matter whether we use -m or --message-file.

After fixing 1, we still need to cut down the length of commit messages (as in comment 12).

Filed two new bugs for the two separate underlying issues (one in depot_tools, one in chromium/src/PRESUBMIT.py).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/ae97943bbcac71a0d99efcc340bf85ace0cc9d8e

commit ae97943bbcac71a0d99efcc340bf85ace0cc9d8e
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Thu Nov 02 19:47:06 2017

watchlists: Precompile filepath regular expressions before using them.

Instead of calling re.search() for every entry in WATCHLIST_DEFINITIONS for
every file being processed, create the regular expressions object beforehand
when parsing the watchlist file.

Processing a Chromium commit with 17k files went down from about 25 minutes
to 10 seconds with this change.

Bug:  780055 
Change-Id: I6493971b67a7466ce8e1e3b28537018a724bbf47
Reviewed-on: https://chromium-review.googlesource.com/751463
Reviewed-by: Aaron Gable <agable@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>

[modify] https://crrev.com/ae97943bbcac71a0d99efcc340bf85ace0cc9d8e/watchlists.py

Demoting to P-2, WPT imports are no longer blocked on this (see  issue 780700 ).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6ff391dca136c9b85c7c78b4680103110bf8bb53

commit 6ff391dca136c9b85c7c78b4680103110bf8bb53
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Nov 13 16:43:39 2017

checkperms: Add a --file-list option.

And use it from //PRESUBMIT.py instead of passing --file <file> for each
file that needs to be processed.

The idea is to read the list of files we want to process from a text file
instead of reading everything from the command-line.

web-platform-test imports were hitting an "argument list too long" error
because there's a particularly huge import with several directories being
renamed; at one point, we ended up with a command-line with ~260k
characters.

Bug:  780055 ,  780629 
Change-Id: Ib1c8b902c42ade77ccbd2662905301d7f766f9d9
Reviewed-on: https://chromium-review.googlesource.com/751504
Reviewed-by: Aaron Gable <agable@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Cr-Commit-Position: refs/heads/master@{#515960}
[modify] https://crrev.com/6ff391dca136c9b85c7c78b4680103110bf8bb53/PRESUBMIT.py
[modify] https://crrev.com/6ff391dca136c9b85c7c78b4680103110bf8bb53/tools/checkperms/checkperms.py

Both underlying issues have been fixed! Everything should be good in theory now, unless we run into something else unexpected next time :)

Huge thanks to Raphael!