Bug 110839

Summary: run-perf-tests fails due to svn_revision not working on a pure git clone
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alancutter, dpranke, eric, esprehn, mithro, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
use --grep=foo rather than --grep foo
none
consolidate log --grep code rniwa: review+

Description Benjamin Poulain 2013-02-25 20:12:20 PST
Make WebKitPy's git_commit_from_svn_revision works for pure Git repository
Comment 1 Benjamin Poulain 2013-02-25 20:18:00 PST
Created attachment 190192 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-26 02:07:50 PST
Comment on attachment 190192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190192&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:311
> +        for i in xrange(25):
> +            git_header = self._run_git(['show', '-s', '--pretty=format:%H%n%b', 'HEAD~%s' % i])

Why 25?  And isn't the other support better when available?
Comment 3 Benjamin Poulain 2013-02-26 11:06:59 PST
> Why 25?

I did that because Git.svn_revision also use the arbitrary limit of 25 (and that's how we find revisions in the first place).
I can change to "while True:", there is really no point at limiting to 25.

> And isn't the other support better when available?

I just don't know a good way to detect if a Git repository is synced with SVN. Any idea?
Comment 4 Eric Seidel (no email) 2013-02-26 11:18:46 PST
I believe git-svn sets a config.  I am on a non-dev machine at the moment, but git config -l should show us.
Comment 5 Dirk Pranke 2013-02-26 13:02:39 PST
I'm not sure what you're trying to accomplish here (in the sense of which commands are broken that you're trying to fix) (and the fact that there are no tests being modified doesn't help).

You can check to see if git-svn is configured by looking for 'svn-remote.svn.url' in the git config.

the svn_revision() command is intended to find the most recent svn revision in the checkout. It is not a command I would recommend be used widely, since it'll fail if you're on branches. I think it was added so rniwa could pull something for the perf tests.

git_commit_from_svn_revision(), on the other hand, can probably be used to refer to *any* svn revision in the tree. Your patch would make it work only for the most recent 25 (git) revisions, and so this probably isn't correct.
Comment 6 Dirk Pranke 2013-02-26 13:03:00 PST
Comment on attachment 190192 [details]
Patch

r-'ing until I can understand better what's going on ...
Comment 7 Benjamin Poulain 2013-02-26 13:12:37 PST
For later then, I don't really have time to fix git.
Comment 8 Ryosuke Niwa 2013-03-06 16:03:29 PST
(In reply to comment #5)
> I'm not sure what you're trying to accomplish here (in the sense of which commands are broken that you're trying to fix) (and the fact that there are no tests being modified doesn't help).

We're trying to fix run-perf-tests on a pure git clone (no svn git).
Comment 9 Eric Seidel (no email) 2013-03-06 16:57:43 PST
Elliot tells me this is blocking him from being able to run-perf-tests. :(
Comment 10 Dirk Pranke 2013-03-06 17:01:21 PST
I'll take a look and suggest a better fix for this particular case. Likely run-perf-tests should just bail out if it can tell if there's no svn checkout, rather than trying to grep the history.
Comment 11 Dirk Pranke 2013-03-06 18:22:14 PST
reopening to fix :).
Comment 12 Dirk Pranke 2013-03-06 19:32:10 PST
Created attachment 191898 [details]
Patch
Comment 13 Eric Seidel (no email) 2013-03-06 19:36:31 PST
Comment on attachment 191898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191898&action=review

LGTM.  Thank you for taking this on.

> Tools/Scripts/webkitpy/common/checkout/scm/scm.py:170
> +        """Returns the latest svn revision found in the checkout."""

This may be the first time I've found docstrings helpful. :p

> Tools/Scripts/webkitpy/common/system/outputcapture.py:32
> +import unittest  # Don't use unittest2 here as the autoinstaller may not have it yet.

Assuming you tested this with py2.5/2.7 sgtm.
Comment 14 WebKit Review Bot 2013-03-06 19:38:05 PST
Attachment 191898 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/checkout/scm/git.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py', u'Tools/Scripts/webkitpy/common/checkout/scm/svn.py', u'Tools/Scripts/webkitpy/common/system/outputcapture.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 152, in main
    patch = host.scm().create_patch(options.git_commit, changed_files=changed_files)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 295, in create_patch
    return self.prepend_svn_revision(self.run(command, decode_output=False, cwd=self.checkout_root))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 273, in prepend_svn_revision
    revision = self.head_svn_revision()
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 167, in head_svn_revision
    return self.svn_revision(self.checkout_root)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 251, in svn_revision
    git_log = self._run_git(['log', '-1', '--grep', 'git-svn-id:', self.find_checkout_root(path)])
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 101, in _run_git
    return self.run(full_command_args, **full_kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 78, in run
    decode_output=decode_output)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 432, in run_command
    (error_handler or self.default_error_handler)(script_error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 350, in default_error_handler
    raise error
webkitpy.common.system.executive.ScriptError: Failed to run "['git', 'log', '-1', '--grep', 'git-svn-id:', u'/mnt/git/webkit-style-queue']" exit_code: 128 cwd: /mnt/git/webkit-style-queue


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Eric Seidel (no email) 2013-03-06 19:39:36 PST
Looks like this patch may have caused the style-queue to barf on itself?
Comment 16 Eric Seidel (no email) 2013-03-06 19:39:58 PST
Comment on attachment 191898 [details]
Patch

style-queue error seems like this may break things.
Comment 17 Dirk Pranke 2013-03-06 19:44:41 PST
check-webkit-style is clean locally.

there must be something wrong with that command in that checkout itself. any chance you can log into the bot and run the command interactively and see what's wrong?
Comment 18 Eric Seidel (no email) 2013-03-06 19:46:29 PST
Alan Cutter is your man.
Comment 19 Alan Cutter 2013-03-06 22:28:48 PST
git svn is not set up by default on the style queue bot.
We have a script to set this up for the bots: http://trac.webkit.org/browser/trunk/Tools/EWSTools/configure-git-svn.sh
You'll want to add this command to the style queue build script: http://trac.webkit.org/browser/trunk/Tools/EWSTools/GoogleComputeEngine/build-feeder-style-sheriffbot.sh

Add the line
    bash configure-git-svn.sh style-queue &&
next to
    bash configure-git-svn.sh sheriff-bot &&
in the build script.


I have run the script on the style-queue bot for you already so it should be safe to resubmit the patch now.
Comment 20 Eric Seidel (no email) 2013-03-06 22:29:58 PST
I thought the whole point of this patch was to handle the case where git-svn wasn't setup?  Or is that a later patch?
Comment 21 Alan Cutter 2013-03-06 22:43:19 PST
(In reply to comment #20)
> I thought the whole point of this patch was to handle the case where git-svn wasn't setup?  Or is that a later patch?

Sorry, was jumping to conclusions based on the error.

This is what I get on the style-queue:

alancutter@gce-feeder-01:/mnt/git/webkit-style-queue$ git log -1 --grep git-svn-id:
fatal: Invalid object name 'git-svn-id'.
alancutter@gce-feeder-01:/mnt/git/webkit-style-queue$ git --version
git version 1.7.0.4


While this is what I get locally:

[17:33:05]~/repos/WebKit$ git log -1 --grep git-svn-id:
commit a7db26f5eab9b33abef5dc6244bf09e0de48fc90
Author: rniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date:   Thu Mar 7 03:03:05 2013 +0000

    Build fix: PageAllocationAligned no longer has executable flag
    https://bugs.webkit.org/show_bug.cgi?id=111659
    
    Patch by Adenilson Cavalcanti <cavalcantii@gmail.com> on 2013-03-06
    Reviewed by Kentaro Hara.
    
    Build fix: use false as parameter to execution flag.
    
    * wtf/PageAllocationAligned.cpp:
    (WTF::PageAllocationAligned::allocate):
    
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@145022 268f45cc-cd09-0410-ab3c-d52691b4dbfc
[17:33:07]~/repos/WebKit$ git --version
git version 1.8.1.3


Looks like a version problem with git. This could be due to the bots still running Lucid. Please let me know what should be done next with the bots.
Comment 22 Dirk Pranke 2013-03-07 12:43:26 PST
Hmm. Yeah, looks like --grep is totally broken in 1.7.0.4.
Comment 23 Dirk Pranke 2013-03-07 13:20:41 PST
ok, turns out --grep isn't totally broken, you just need to use "--grep=foo" rather than "--grep" "foo". handling separate arguments wasn't implemented until 8/21/10, and 1.7.0.4 was released in 3/10. 

New patch coming, no need to change the bots (although we should probably upgrade them to a precise-ish version soon).
Comment 24 Dirk Pranke 2013-03-07 13:21:42 PST
Created attachment 192076 [details]
use --grep=foo rather than --grep foo
Comment 25 Eric Seidel (no email) 2013-03-07 13:24:34 PST
Comment on attachment 192076 [details]
use --grep=foo rather than --grep foo

View in context: https://bugs.webkit.org/attachment.cgi?id=192076&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:251
> +        git_log = self._run_git(['log', '-1', '--grep=git-svn-id:', self.find_checkout_root(path)])

You might add a comment about why we use grep= instead of the standard separate arguments.  Or heck, we could even share these two calls in a helper function.
Comment 26 Dirk Pranke 2013-03-07 13:25:53 PST
Comment on attachment 192076 [details]
use --grep=foo rather than --grep foo

View in context: https://bugs.webkit.org/attachment.cgi?id=192076&action=review

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:251
>> +        git_log = self._run_git(['log', '-1', '--grep=git-svn-id:', self.find_checkout_root(path)])
> 
> You might add a comment about why we use grep= instead of the standard separate arguments.  Or heck, we could even share these two calls in a helper function.

Will do.
Comment 27 Dirk Pranke 2013-03-07 14:25:51 PST
Created attachment 192084 [details]
consolidate log --grep code
Comment 28 Eric Seidel (no email) 2013-03-07 14:27:42 PST
Comment on attachment 192084 [details]
consolidate log --grep code

View in context: https://bugs.webkit.org/attachment.cgi?id=192084&action=review

LGTM.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:252
> +        return self._run_git(['log', '-1', '--grep=' + grep_str, '--date=iso', self.find_checkout_root(path)])

You may want to quote grep_str, but this is OK.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:315
> +        # FIXME: https://bugs.webkit.org/show_bug.cgi?id=111668
> +        # We should change this to run git log --grep 'git-svn-id' instead
> +        # so that we don't require git+svn to be set up.

You might mention _most_recent_log_matching, but you can also do that in the bug.
Comment 29 Dirk Pranke 2013-03-07 14:42:10 PST
Committed r145141: <http://trac.webkit.org/changeset/145141>