Bug 110839 – run-perf-tests fails due to svn_revision not working on a pure git clone

• Component: New Bugs
• Status: RESOLVED FIXED

[Benjamin Poulain]

Make WebKitPy's git_commit_from_svn_revision works for pure Git repository

[Benjamin Poulain]

Created attachment 190192 [details]
Patch

[Eric Seidel (no email)]

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?

[Benjamin Poulain]

> 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?

[Eric Seidel (no email)]

I believe git-svn sets a config.  I am on a non-dev machine at the moment, but git config -l should show us.

[Dirk Pranke]

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.

[Dirk Pranke]

Comment on attachment 190192 [details]
Patch

r-'ing until I can understand better what's going on ...

[Benjamin Poulain]

For later then, I don't really have time to fix git.

[Ryosuke Niwa]

(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).

[Eric Seidel (no email)]

Elliot tells me this is blocking him from being able to run-perf-tests. :(

[Dirk Pranke]

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.

[Dirk Pranke]

reopening to fix :).

[Dirk Pranke]

Created attachment 191898 [details]
Patch

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Looks like this patch may have caused the style-queue to barf on itself?

[Eric Seidel (no email)]

Comment on attachment 191898 [details]
Patch

style-queue error seems like this may break things.

[Dirk Pranke]

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?

[Eric Seidel (no email)]

Alan Cutter is your man.

[Alan Cutter]

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.

[Eric Seidel (no email)]

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?

[Alan Cutter]

(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: [e-mail] <[e-mail]@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 <[e-mail]> 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.

[Dirk Pranke]

Hmm. Yeah, looks like --grep is totally broken in 1.7.0.4.

[Dirk Pranke]

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).

[Dirk Pranke]

Created attachment 192076 [details]
use --grep=foo rather than --grep foo

[Eric Seidel (no email)]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 192084 [details]
consolidate log --grep code

[Eric Seidel (no email)]

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.

[Dirk Pranke]

Committed r145141: <http://trac.webkit.org/changeset/145141>