Launchpad itself

Merge lp:~jelmer/launchpad/bug499115 into lp:launchpad

  1. bug499115
  2. Merge into devel
Proposed by Jelmer Vernooij
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~jelmer/launchpad/bug499115
Merge into: lp:launchpad
Diff against target: 130 lines (+56/-18)
2 files modified
lib/lp/buildmaster/model/buildbase.py (+27/-18)
lib/lp/buildmaster/tests/test_buildbase.py (+29/-0)
To merge this branch: bzr merge lp:~jelmer/launchpad/bug499115
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+20465@code.launchpad.net

Commit message

This trivial change prevents the buildd master from falling over if a container directory was created but no log file uploaded.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This is a resubmission of https://code.edge.launchpad.net/~jelmer/launchpad/bug499115/+merge/19328 with Henning's comments addressed.

I'm submitting a new merge proposal since I would like to actually target devel rather than db-devel.

Revision history for this message
Brad Crittenden (bac) wrote :

Hi Jelmer,

Thanks for the branch. I have two suggestions:

1) UPLOADLOG_FILENAME -> UPLOAD_LOG_FILENAME

2) You use try/finally for resource management in a couple of places. It might be nice to use 'with' as Barry discussed on the mailing list a few weeks ago. That's totally up to you, though.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/buildmaster/model/buildbase.py'
2--- lib/lp/buildmaster/model/buildbase.py 2010-02-16 02:27:24 +0000
3+++ lib/lp/buildmaster/model/buildbase.py 2010-03-05 23:33:34 +0000
4@@ -3,6 +3,8 @@
5
6 # pylint: disable-msg=E0211,E0213
7
8+from __future__ import with_statement
9+
10 """Common build base classes."""
11
12 __metaclass__ = type
13@@ -32,6 +34,9 @@
14 from lp.soyuz.model.buildqueue import BuildQueue
15
16
17+UPLOAD_LOG_FILENAME = 'uploader.log'
18+
19+
20 class BuildBase:
21 """A mixin class providing functionality for farm jobs that build a
22 package.
23@@ -96,6 +101,25 @@
24 return None
25 return self._getProxiedFileURL(self.buildlog)
26
27+ def getUploadLogContent(self, root, leaf):
28+ """Retrieve the upload log contents.
29+
30+ :param root: Root directory for the uploads
31+ :param leaf: Leaf for this particular upload
32+ :return: Contents of log file or message saying no log file was found.
33+ """
34+ # Retrieve log file content.
35+ possible_locations = (
36+ 'failed', 'failed-to-move', 'rejected', 'accepted')
37+ for location_dir in possible_locations:
38+ log_filepath = os.path.join(root, location_dir, leaf,
39+ UPLOAD_LOG_FILENAME)
40+ if os.path.exists(log_filepath):
41+ with open(log_filepath, 'r') as uploader_log_file:
42+ return uploader_log_file.read()
43+ else:
44+ return 'Could not find upload log file'
45+
46 @property
47 def upload_log_url(self):
48 """See `IBuildBase`."""
49@@ -161,7 +185,7 @@
50 out_file = open(out_file_name, "wb")
51 copy_and_close(slave_file, out_file)
52
53- uploader_logfilename = os.path.join(upload_dir, 'uploader.log')
54+ uploader_logfilename = os.path.join(upload_dir, UPLOAD_LOG_FILENAME)
55 uploader_command = self.getUploaderCommand(
56 upload_leaf, uploader_logfilename)
57 logger.debug("Saving uploader log at '%s'" % uploader_logfilename)
58@@ -239,23 +263,8 @@
59 not self.verifySuccessfulUpload()):
60 logger.debug("Build %s upload failed." % self.id)
61 self.buildstate = BuildStatus.FAILEDTOUPLOAD
62- # Retrieve log file content.
63- possible_locations = (
64- 'failed', 'failed-to-move', 'rejected', 'accepted')
65- for location_dir in possible_locations:
66- upload_final_location = os.path.join(
67- root, location_dir, upload_leaf)
68- if os.path.exists(upload_final_location):
69- log_filepath = os.path.join(
70- upload_final_location, 'uploader.log')
71- uploader_log_file = open(log_filepath)
72- try:
73- uploader_log_content = uploader_log_file.read()
74- finally:
75- uploader_log_file.close()
76- break
77- else:
78- uploader_log_content = 'Could not find upload log file'
79+ uploader_log_content = self.getUploadLogContent(root,
80+ upload_leaf)
81 # Store the upload_log_contents in librarian so it can be
82 # accessed by anyone with permission to see the build.
83 self.storeUploadLog(uploader_log_content)
84
85=== modified file 'lib/lp/buildmaster/tests/test_buildbase.py'
86--- lib/lp/buildmaster/tests/test_buildbase.py 2010-02-03 16:21:57 +0000
87+++ lib/lp/buildmaster/tests/test_buildbase.py 2010-03-05 23:33:34 +0000
88@@ -1,6 +1,8 @@
89 # Copyright 2010 Canonical Ltd. This software is licensed under the
90 # GNU Affero General Public License version 3 (see the file LICENSE).
91
92+from __future__ import with_statement
93+
94 """Tests for `IBuildBase`."""
95
96 __metaclass__ = type
97@@ -45,6 +47,33 @@
98
99 layer = DatabaseFunctionalLayer
100
101+ def test_getUploadLogContent_nolog(self):
102+ """If there is no log file there, a string explaining that is returned.
103+ """
104+ self.useTempDir()
105+ build_base = BuildBase()
106+ self.assertEquals('Could not find upload log file',
107+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
108+
109+ def test_getUploadLogContent_only_dir(self):
110+ """If there is a directory but no log file, expect the error string,
111+ not an exception."""
112+ self.useTempDir()
113+ os.makedirs("accepted/myleaf")
114+ build_base = BuildBase()
115+ self.assertEquals('Could not find upload log file',
116+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
117+
118+ def test_getUploadLogContent_readsfile(self):
119+ """If there is a log file, return its contents."""
120+ self.useTempDir()
121+ os.makedirs("accepted/myleaf")
122+ with open('accepted/myleaf/uploader.log', 'w') as f:
123+ f.write('foo')
124+ build_base = BuildBase()
125+ self.assertEquals('foo',
126+ build_base.getUploadLogContent(os.getcwd(), "myleaf"))
127+
128 def test_getUploaderCommand(self):
129 build_base = BuildBase()
130 upload_leaf = self.factory.getUniqueString('upload-leaf')