Merge lp:~milo/linaro-patchmetrics/bug1012037 into lp:linaro-patchmetrics

[Milo Casagrande]

Hello,

requesting a new merge: with this changes I refactored a little bit the functions used to clone and pull the git repositories, I added a new one for the kill action, and for the new checkout action.

The new functions would be helpful too in the process of fixing bug 1017933.
The addition of a dictionary at the top of the file with the name of the project and the branch name is a temporary solution.

[lp:~milo/linaro-patchmetrics/bug1012037]

370. By Milo Casagrande on 2012-06-28

Added 'master' branch for 'gcc-patches' project.

[Milo Casagrande (milo)]

For the moment I added two repositories in the temporary dictionary: cpufreq and gcc. For cpu-freq we had a confirmation to use the 'next' branch, for gcc we still do not have it, but if we have to change branch later, it will not be a problem, the 'git checkout' operation will work and switch to the new branch.

[Deepti B. Kalakeri (deeptik)]

+++ apps/patchwork/bin/update-committed-patches.py 2012-06-28 08:43:05 +0000
@@ -21,8 +21,12 @@

 from patchwork.models import Project, State
 from patchwork.utils import (
- ensure_source_checkout_for_project, FailedToFetchGitRepo,
- get_diff_for_commit, get_commits_to_parse, get_patches_matching_commit)
+ ensure_source_checkout_for_project,
+ get_commits_to_parse,
+ get_diff_for_commit,
+ get_patches_matching_commit,
+ GitException,
+ )

The comma at the end can be removed.

=== modified file 'apps/patchwork/utils.py'
--- apps/patchwork/utils.py 2012-06-25 15:29:36 +0000
+++ apps/patchwork/utils.py 2012-06-28 08:43:05 +0000
@@ -33,6 +33,25 @@

 # Timeout for git operations.
 GIT_TIMEOUT = 120 * 60
+# XXX This is a temporary solution, a workardound for bug 1017933.
+# Once that bug is fixed or in the process, this should be removed.

FIXME would be more appropriate than XXX

+ # XXX this is a temporary solution, we need to fix bug
+ # https://bugs.launchpad.net/linaro-patchmetrics/+bug/1017933
+ if project.name in REPO_BRANCHES:
+ ensure_on_valid_branch(project, REPO_BRANCHES.get(project.name))
+
Here as well, FIXME would be more appropriate than XXX

The following piece of code repeats in 2 places, it can be made as a function call ?
    wait = 0
    wait_increment = 5

    while wait < timeout:
        proc.poll()
        if proc.returncode is not None:
            break
        wait += wait_increment
        time.sleep(wait_increment)

    if proc.returncode is None:
        kill_git_processes(proc, project)

    stdout, stderr = proc.communicate()

review: Needs Fixing

[lp:~milo/linaro-patchmetrics/bug1012037]

371. By Milo Casagrande on 2012-06-28

Removed duplicated code, PEP8 fixes.

[Milo Casagrande (milo)]

> +++ apps/patchwork/bin/update-committed-patches.py 2012-06-28 08:43:05
>
> The comma at the end can be removed.

Fixed.

> === modified file 'apps/patchwork/utils.py'
> # Timeout for git operations.
> GIT_TIMEOUT = 120 * 60
> +# XXX This is a temporary solution, a workardound for bug 1017933.
> +# Once that bug is fixed or in the process, this should be removed.
>
> FIXME would be more appropriate than XXX

Fixed for both.

> The following piece of code repeats in 2 places, it can be made as a function
> call ?
> wait = 0
> wait_increment = 5
>
> while wait < timeout:
> proc.poll()
> if proc.returncode is not None:
> break
> wait += wait_increment
> time.sleep(wait_increment)
>
> if proc.returncode is None:
> kill_git_processes(proc, project)

I was not completely sure to add a new function for that too, but now that you point it out.
I added a new one, including up to the kill_git_processes call. I preferred to keep the other one out of this new function.

Thanks.

[Deepti B. Kalakeri (deeptik)]

+1 Looks good. I hope we get our Errors fixed now.

Thanks!!!
Deepti.

review: Approve

=== modified file 'apps/patchwork/bin/update-committed-patches.py'
--- apps/patchwork/bin/update-committed-patches.py	2012-01-13 12:48:45 +0000
+++ apps/patchwork/bin/update-committed-patches.py	2012-06-28 11:55:22 +0000
@@ -21,8 +21,12 @@
 
 from patchwork.models import Project, State
 from patchwork.utils import (
-    ensure_source_checkout_for_project, FailedToFetchGitRepo,
-    get_diff_for_commit, get_commits_to_parse, get_patches_matching_commit)
+    ensure_source_checkout_for_project,
+    get_commits_to_parse,
+    get_diff_for_commit,
+    get_patches_matching_commit,
+    GitException
+    )
 
 
 def main():
@@ -31,7 +35,7 @@
     for project in projects:
         try:
             root = ensure_source_checkout_for_project(project)
-        except FailedToFetchGitRepo, e:
+        except GitException, e:
             sys.stderr.write(str(e) + "\n")
             continue
 

=== modified file 'apps/patchwork/utils.py'
--- apps/patchwork/utils.py	2012-06-25 15:29:36 +0000
+++ apps/patchwork/utils.py	2012-06-28 11:55:22 +0000
@@ -33,6 +33,25 @@
 
 # Timeout for git operations.
 GIT_TIMEOUT = 120 * 60
+# FIXME This is a temporary solution, a workardound for bug 1017933.
+# Once that bug is fixed or in the process, this should be removed.
+REPO_BRANCHES = {'cpufreq': 'next', 'gcc-patches': 'master'}
+
+
+class GitException(Exception):
+    """General class for git exceptions."""
+
+
+class FailedToFetchGitRepo(GitException):
+    """Failed to fetch the git repo of a project."""
+
+
+class FailedToCheckoutGitRepo(GitException):
+    """Failed to perform a checkout operation on a git repository."""
+
+
+class GitTimeout(GitException):
+    """Timeout when fetching the git repo of a project."""
 
 
 def get_patch_ids(d, prefix='patch_id'):
@@ -153,14 +172,6 @@
     return []
 
 
-class FailedToFetchGitRepo(Exception):
-    """Failed to fetch the git repo of a project."""
-
-
-class GitFetchTimeout(FailedToFetchGitRepo):
-    """Timeout when fetching the git repo of a project."""
-
-
 def get_child_pids(pid):
     """Recursively get the PIDs of the given process' children."""
     pids = []
@@ -185,6 +196,14 @@
 # not suitable, whereas its latest version has a bunch of non-packaged
 # dependencies.
 def ensure_source_checkout_for_project(project, timeout=GIT_TIMEOUT):
+    """Ensures that we have the code for the project we are working on.
+
+    If there is no code in the PATCHWORK_GIT_REPOS_DIR, it performs a 'clone'
+    operation, otherwise we try to 'pull' the latest changes.
+
+    :param project: the project we are working on
+    :param timeout: the timeout for the git operation, defaults to GIT_TIMEOUT
+    """
     root = os.path.join(settings.PATCHWORK_GIT_REPOS_DIR, project.linkname)
 
     if os.path.exists(root):
@@ -194,60 +213,109 @@
         proc = subprocess.Popen(['git', 'clone', project.source_tree, root],
                                 stdout=subprocess.PIPE)
 
+    git_process_poll(proc, project, timeout)
+
+    stdout, stderr = proc.communicate()
+    if proc.returncode != 0:
+        raise FailedToFetchGitRepo(
+            "Failed to fetch %s (%s - %s).\n stdout: %s\n stderr: %s" % (
+                project.source_tree, project.name, project.linkname, stdout,
+                stderr))
+
+    # FIXME this is a temporary solution, we need to fix bug
+    # https://bugs.launchpad.net/linaro-patchmetrics/+bug/1017933
+    if project.name in REPO_BRANCHES:
+        ensure_on_valid_branch(project, REPO_BRANCHES.get(project.name))
+
+    return root
+
+
+def ensure_on_valid_branch(project, branch, timeout=GIT_TIMEOUT):
+    """Ensures that we are on a valid git branch.
+
+    Performs a git checkout on the provided branch name, no check is done on
+    the name, it has to be a valid git branch. If we are already on that branch
+    nothing happens, git will return 0 as normal.
+
+    :param project: the project we are working on
+    :param branch: the name of the branch to checkout
+    :param timeout: the max timeout for git operation, defaults to GIT_TIMEOUT
+    """
+    cwd = os.path.join(settings.PATCHWORK_GIT_REPOS_DIR, project.linkname)
+    proc = subprocess.Popen(['git', 'checkout', branch], cwd=cwd,
+                            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+    git_process_poll(proc, project, timeout)
+
+    stdout, stderr = proc.communicate()
+    if proc.returncode != 0:
+        raise FailedToCheckoutGitRepo(
+            "Failed to checkout branch '%s' of %s.\n stdout: %s"
+            "\n stderr: %s" % (branch, project.name, stdout, stderr))
+
+
+def git_process_poll(process, project, timeout):
+    """Check the status of the process.
+
+    :param process: the process to watch
+    :param project: the project we are working on
+    :param timeout: the max timeout for the process
+    """
     wait = 0
     wait_increment = 5
 
     while wait < timeout:
-        proc.poll()
-        if proc.returncode is not None:
+        process.poll()
+        if process.returncode is not None:
             break
         wait += wait_increment
         time.sleep(wait_increment)
 
-    if proc.returncode is None:
-        msg = "Timeout (%d/%d seconds) when fetching '%s' (%s).\n" % \
-        (wait, timeout, project.name, project.linkname)
-        msg += " Project path: %s\n Project tree: %s\n" % \
-        (root, project.source_tree)
-
-        # Kill the subprocess we started and all its children. This is
-        # necessary because 'git pull' will actually run a shell script
-        # as a subprocess, with the script firing a 'git fetch' subprocess,
-        # and if we don't kill them all individually they will be left around
-        # and some times they will never finish.
-        pids = [str(proc.pid)]
-        # We traverse the child processes recursively here instead of querying
-        # all processes under the same process group because on production
-        # this will be run as a cronjob and thus the process group will
-        # include the shell process started by cron, which is the parent of
-        # this very process and thus we don't want to kill -- we only want to
-        # kill the subprocess we started above and all its children.
-        pids.extend(get_child_pids(proc.pid))
-        msg += " Killing processes %s and moving on." % pids
-        proc = subprocess.Popen(
-            ['kill', '-KILL'] + pids,
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        kill_stdout, kill_stderr = proc.communicate()
-        msg += " Output of the kill command: (%s, %s)." % (
-            kill_stdout, kill_stderr)
-
-        # Poll the subprocess we started to ensure it's not left behind as a
-        # zombie, but first sleep a couple seconds to make sure it's finished
-        # when we poll.
-        time.sleep(2)
-        if proc.poll() is None:
-            msg += (" Oops, we failed to kill the git process (pid=%d) that "
-                    "timed out." % proc.pid)
-        raise GitFetchTimeout(msg)
-
-    stdout, stderr = proc.communicate()
-    if proc.returncode != 0:
-        raise FailedToFetchGitRepo(
-            "Failed to fetch %s (%s - %s).\n stdout: %s\n stderr: %s" % (
-                project.source_tree, project.name, project.linkname, stdout,
-                stderr))
-
-    return root
+    if process.returncode is None:
+        kill_git_processes(process, project)
+
+
+def kill_git_processes(process, project):
+    """Kills the process of the git operation, and its subprocesses.
+
+    :param process: the parent process to kill
+    :param project: the project we are working on
+    """
+    cwd = os.path.join(settings.PATCHWORK_GIT_REPOS_DIR, project.linkname)
+    msg = ("Timeout when performing git operation on '%s' (%s).\n" %
+            (project.name, project.linkname))
+    msg += " Project path: %s\n Project tree: %s\n" % \
+    (cwd, project.source_tree)
+
+    # Kill the subprocess we started and all its children. This is
+    # necessary because 'git pull' will actually run a shell script
+    # as a subprocess, with the script firing a 'git fetch' subprocess,
+    # and if we don't kill them all individually they will be left around
+    # and some times they will never finish.
+    pids = [str(process.pid)]
+    # We traverse the child processes recursively here instead of querying
+    # all processes under the same process group because on production
+    # this will be run as a cronjob and thus the process group will
+    # include the shell process started by cron, which is the parent of
+    # this very process and thus we don't want to kill -- we only want to
+    # kill the subprocess we started above and all its children.
+    pids.extend(get_child_pids(process.pid))
+    msg += " Killing processes %s and moving on." % pids
+    proc = subprocess.Popen(
+        ['kill', '-KILL'] + pids,
+        stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+    kill_stdout, kill_stderr = proc.communicate()
+    msg += " Output of the kill command: (%s, %s)." % (
+        kill_stdout, kill_stderr)
+
+    # Poll the subprocess we started to ensure it's not left behind as a
+    # zombie, but first sleep a couple seconds to make sure it's finished
+    # when we poll.
+    time.sleep(2)
+    if proc.poll() is None:
+        msg += (" Oops, we failed to kill the git process (pid=%d) that "
+                "timed out." % proc.pid)
+    raise GitTimeout(msg)
 
 
 def get_commits_to_parse(root, start_at):