Merge lp:~abentley/launchpad/no-write-cwd into lp:launchpad

Proposed by Aaron Bentley
Status: Merged
Approved by: Robert Collins
Approved revision: no longer in the source branch.
Merged at revision: 11172
Proposed branch: lp:~abentley/launchpad/no-write-cwd
Merge into: lp:launchpad
Diff against target: 271 lines (+69/-41)
4 files modified
lib/lp/archiveuploader/dscfile.py (+32/-28)
lib/lp/archiveuploader/nascentupload.py (+7/-0)
lib/lp/archiveuploader/tests/test_dscfile.py (+29/-12)
lib/lp/archiveuploader/uploadprocessor.py (+1/-1)
To merge this branch: bzr merge lp:~abentley/launchpad/no-write-cwd
Reviewer Review Type Date Requested Status
Robert Collins (community) Needs Fixing
Julian Edwards (community) Approve
Review via email: mp+29965@code.launchpad.net

Commit message

Avoid writing to the to the CWD.

Description of the change

= Summary =
Avoid writing to CWD in the archive uploader.

== Proposed fix ==
Introduce a DSCFile.cleanUp method, so that we can have a temp directory that
lives between unpackAndCheckSource and storeInDatabase.

== Pre-implementation notes ==
Mid-implementation discussion with bigjools

== Implementation details ==
None

== Tests ==
bin/test -t nascent -t archiveuploader

== Demo and Q/A ==
Try doing a sourcepackagerecipe build on staging. It should not die like this:
http://staging.launchpadlibrarian.net/51599092/aRg8XXIxzd43E3UO4chC78epwXK.txt

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/nascentupload.py
  lib/lp/archiveuploader/uploadprocessor.py
  lib/lp/archiveuploader/dscfile.py
  lib/lp/archiveuploader/tests/test_dscfile.py

/home/abentley/launchpad/no-write-cwd/bin/lint.sh: line 161: pocketlint: command not found

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Looks good! Just a couple of small things:

1. change the indentation at line 41 of the diff as we discussed.
2. from lp.archiveuploader.tests import datadir
  then remove the hardcoded /lib/lp/archiveuploaders/tests/data

Thanks for making the change.

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

Looks like this is in tweak mode.

review: Needs Fixing
Revision history for this message
Aaron Bentley (abentley) wrote :

> Looks like this is in tweak mode.

If you're going to say that, please say a bit more.

Revision history for this message
Robert Collins (lifeless) wrote :

On Mon, Jul 19, 2010 at 4:38 PM, Aaron Bentley <email address hidden> wrote:
>> Looks like this is in tweak mode.
>
> If you're going to say that, please say a bit more.

Another reviewer has asked for some changes and for the branch to be
landed after that - what I meant by 'it is in tweak mode' is that it
should have its status set accordingly: it no longer needs review,
rather it needs those changes made and the branch to be landed.

Revision history for this message
Aaron Bentley (abentley) wrote :

> Another reviewer has asked for some changes and for the branch to be
> landed after that - what I meant by 'it is in tweak mode' is that it
> should have its status set accordingly: it no longer needs review,
> rather it needs those changes made and the branch to be landed.

The convention in Launchpad reviews is that approve reviews may be conditional, based on the text of the review. A subsequent "needs fixing" review like yours creates confusion.

Revision history for this message
Robert Collins (lifeless) wrote :

On Mon, Jul 19, 2010 at 5:37 PM, Aaron Bentley <email address hidden> wrote:
>> Another reviewer has asked for some changes and for the branch to be
>> landed after that - what I meant by 'it is in tweak mode' is that it
>> should have its status set accordingly: it no longer needs review,
>> rather it needs those changes made and the branch to be landed.
>
> The convention in Launchpad reviews is that approve reviews may be conditional, based on the text of the review.  A subsequent "needs fixing" review like yours creates confusion.

I'll try to avoid confusion in future. I was perhaps getting a bit
brief - we had a significant number of confusing-status merge
proposals in the queue. Sorry.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archiveuploader/dscfile.py'
2--- lib/lp/archiveuploader/dscfile.py 2010-06-25 21:35:15 +0000
3+++ lib/lp/archiveuploader/dscfile.py 2010-07-20 14:43:51 +0000
4@@ -13,7 +13,7 @@
5 'SignableTagFile',
6 'DSCFile',
7 'DSCUploadedFile',
8- 'findAndMoveChangelog',
9+ 'findChangelog',
10 'findCopyright',
11 ]
12
13@@ -204,6 +204,8 @@
14 else:
15 self.processSignature()
16
17+ self.unpacked_dir = None
18+
19 #
20 # Useful properties.
21 #
22@@ -483,18 +485,19 @@
23 "Verifying uploaded source package by unpacking it.")
24
25 # Get a temporary dir together.
26- tmpdir = tempfile.mkdtemp()
27+ self.unpacked_dir = tempfile.mkdtemp()
28
29 # chdir into it
30 cwd = os.getcwd()
31- os.chdir(tmpdir)
32- dsc_in_tmpdir = os.path.join(tmpdir, self.filename)
33+ os.chdir(self.unpacked_dir)
34+ dsc_in_tmpdir = os.path.join(self.unpacked_dir, self.filename)
35
36 package_files = self.files + [self]
37 try:
38 for source_file in package_files:
39- os.symlink(source_file.filepath,
40- os.path.join(tmpdir, source_file.filename))
41+ os.symlink(
42+ source_file.filepath,
43+ os.path.join(self.unpacked_dir, source_file.filename))
44 args = ["dpkg-source", "-sn", "-x", dsc_in_tmpdir]
45 dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
46 stderr=subprocess.PIPE)
47@@ -517,8 +520,9 @@
48 # SourcePackageRelease records.
49
50 # Check if 'dpkg-source' created only one directory.
51- temp_directories = [dirname for dirname in os.listdir(tmpdir)
52- if os.path.isdir(dirname)]
53+ temp_directories = [
54+ dirname for dirname in os.listdir(self.unpacked_dir)
55+ if os.path.isdir(dirname)]
56 if len(temp_directories) > 1:
57 yield UploadError(
58 'Unpacked source contains more than one directory: %r'
59@@ -528,31 +532,33 @@
60 # name (<sourcename>-<no_epoch(no_revision(version))>).
61
62 # Locate both the copyright and changelog files for later processing.
63- for error in findCopyright(self, tmpdir, self.logger):
64+ for error in findCopyright(self, self.unpacked_dir, self.logger):
65 yield error
66
67- for error in findAndMoveChangelog(self, cwd, tmpdir, self.logger):
68+ for error in findChangelog(self, self.unpacked_dir, self.logger):
69 yield error
70
71 self.logger.debug("Cleaning up source tree.")
72+ self.logger.debug("Done")
73+
74+ def cleanUp(self):
75+ if self.unpacked_dir is None:
76+ return
77 try:
78- shutil.rmtree(tmpdir)
79+ shutil.rmtree(self.unpacked_dir)
80 except OSError, error:
81 # XXX: dsilvers 2006-03-15: We currently lack a test for this.
82 if errno.errorcode[error.errno] != 'EACCES':
83- yield UploadError(
84+ raise UploadError(
85 "%s: couldn't remove tmp dir %s: code %s" % (
86- self.filename, tmpdir, error.errno))
87+ self.filename, self.unpacked_dir, error.errno))
88 else:
89- yield UploadWarning(
90- "%s: Couldn't remove tree, fixing up permissions." %
91- self.filename)
92- result = os.system("chmod -R u+rwx " + tmpdir)
93+ result = os.system("chmod -R u+rwx " + self.unpacked_dir)
94 if result != 0:
95- yield UploadError("chmod failed with %s" % result)
96- shutil.rmtree(tmpdir)
97+ raise UploadError("chmod failed with %s" % result)
98+ shutil.rmtree(self.unpacked_dir)
99+ self.unpacked_dir = None
100
101- self.logger.debug("Done")
102
103 def findBuild(self):
104 """Find and return the SourcePackageRecipeBuild, if one is specified.
105@@ -733,15 +739,14 @@
106 logger.debug("Copying copyright contents.")
107 dsc_file.copyright = open(copyright_file).read().strip()
108
109-def findAndMoveChangelog(dsc_file, target_dir, source_dir, logger):
110+def findChangelog(dsc_file, source_dir, logger):
111 """Find and move any debian/changelog.
112
113- This function finds the changelog file within the source package and
114- moves it to target_dir. The changelog file is later uploaded to the
115- librarian by DSCFile.storeInDatabase().
116+ This function finds the changelog file within the source package. The
117+ changelog file is later uploaded to the librarian by
118+ DSCFile.storeInDatabase().
119
120 :param dsc_file: A DSCFile object where the copyright will be stored.
121- :param target_dir: The directory where the changelog will end up.
122 :param source_dir: The directory where the source was extracted.
123 :param logger: A logger object for debug output.
124 """
125@@ -756,9 +761,8 @@
126 return
127
128 # Move the changelog file out of the package direcotry
129- logger.debug("Found changelog contents; moving to root directory")
130- dsc_file.changelog_path = os.path.join(target_dir, "changelog")
131- shutil.move(changelog_file, dsc_file.changelog_path)
132+ logger.debug("Found changelog")
133+ dsc_file.changelog_path = changelog_file
134
135 def check_format_1_0_files(filename, file_type_counts, component_counts,
136 bzip2_count):
137
138=== modified file 'lib/lp/archiveuploader/nascentupload.py'
139--- lib/lp/archiveuploader/nascentupload.py 2010-05-27 22:18:16 +0000
140+++ lib/lp/archiveuploader/nascentupload.py 2010-07-20 14:43:51 +0000
141@@ -839,6 +839,12 @@
142 'Exception while accepting:\n %s' % e, exc_info=True)
143 self.do_reject(notify)
144 return False
145+ else:
146+ self.cleanUp()
147+
148+ def cleanUp(self):
149+ if self.changes.dsc is not None:
150+ self.changes.dsc.cleanUp()
151
152 def do_reject(self, notify=True):
153 """Reject the current upload given the reason provided."""
154@@ -869,6 +875,7 @@
155 self.queue_root.notify(summary_text=self.rejection_message,
156 changes_file_object=changes_file_object, logger=self.logger)
157 changes_file_object.close()
158+ self.cleanUp()
159
160 def _createQueueEntry(self):
161 """Return a PackageUpload object."""
162
163=== modified file 'lib/lp/archiveuploader/tests/test_dscfile.py'
164--- lib/lp/archiveuploader/tests/test_dscfile.py 2010-07-18 00:26:33 +0000
165+++ lib/lp/archiveuploader/tests/test_dscfile.py 2010-07-20 14:43:51 +0000
166@@ -7,11 +7,14 @@
167
168 import os
169
170+from canonical.config import config
171+from canonical.launchpad.scripts.logger import QuietFakeLogger
172 from canonical.testing.layers import LaunchpadZopelessLayer
173 from lp.archiveuploader.dscfile import (
174- findAndMoveChangelog, findCopyright)
175+ DSCFile, findChangelog, findCopyright)
176 from lp.archiveuploader.nascentuploadfile import UploadError
177-from lp.archiveuploader.tests import mock_logger_quiet
178+from lp.archiveuploader.tests import datadir, mock_logger_quiet
179+from lp.archiveuploader.uploadpolicy import BuildDaemonUploadPolicy
180 from lp.testing import TestCase, TestCaseWithFactory
181
182
183@@ -27,7 +30,6 @@
184 os.makedirs(self.dir_path)
185 self.copyright_path = os.path.join(self.dir_path, "copyright")
186 self.changelog_path = os.path.join(self.dir_path, "changelog")
187- self.changelog_dest = os.path.join(self.tmpdir, "changelog")
188 self.dsc_file = self.MockDSCFile()
189
190 def testBadDebianCopyright(self):
191@@ -66,8 +68,8 @@
192 dangling symlink in an attempt to try and access files on the system
193 processing the source packages."""
194 os.symlink("/etc/passwd", self.changelog_path)
195- errors = list(findAndMoveChangelog(
196- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
197+ errors = list(findChangelog(
198+ self.dsc_file, self.tmpdir, mock_logger_quiet))
199
200 self.assertEqual(len(errors), 1)
201 self.assertIsInstance(errors[0], UploadError)
202@@ -82,12 +84,12 @@
203 file.write(changelog)
204 file.close()
205
206- errors = list(findAndMoveChangelog(
207- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
208+ errors = list(findChangelog(
209+ self.dsc_file, self.tmpdir, mock_logger_quiet))
210
211 self.assertEqual(len(errors), 0)
212 self.assertEqual(self.dsc_file.changelog_path,
213- self.changelog_dest)
214+ self.changelog_path)
215
216 def testOversizedFile(self):
217 """Test that a file larger than 10MiB will fail.
218@@ -106,8 +108,8 @@
219 file.write(empty_file)
220 file.close()
221
222- errors = list(findAndMoveChangelog(
223- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
224+ errors = list(findChangelog(
225+ self.dsc_file, self.tmpdir, mock_logger_quiet))
226
227 self.assertIsInstance(errors[0], UploadError)
228 self.assertEqual(
229@@ -120,12 +122,27 @@
230
231 layer = LaunchpadZopelessLayer
232
233+ def getDscFile(self, name):
234+ dsc_path = datadir(os.path.join('suite', name, name + '.dsc'))
235+ class Changes:
236+ architectures = ['source']
237+ logger = QuietFakeLogger()
238+ policy = BuildDaemonUploadPolicy()
239+ policy.distroseries = self.factory.makeDistroSeries()
240+ policy.archive = self.factory.makeArchive()
241+ policy.distro = policy.distroseries.distribution
242+ return DSCFile(dsc_path, 'digest', 0, 'main/editors',
243+ 'priority', 'package', 'version', Changes, policy, logger)
244+
245 def test_ReadOnlyCWD(self):
246 """Processing a file should work when cwd is read-only."""
247 tempdir = self.useTempDir()
248- dsc_file = self.factory.makeDscFile(tempdir)
249 os.chmod(tempdir, 0555)
250 try:
251- list(dsc_file.unpackAndCheckSource())
252+ dsc_file = self.getDscFile('bar_1.0-1')
253+ try:
254+ list(dsc_file.verify())
255+ finally:
256+ dsc_file.cleanUp()
257 finally:
258 os.chmod(tempdir, 0755)
259
260=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
261--- lib/lp/archiveuploader/uploadprocessor.py 2010-06-29 14:01:01 +0000
262+++ lib/lp/archiveuploader/uploadprocessor.py 2010-07-20 14:43:51 +0000
263@@ -293,7 +293,7 @@
264 (distribution, suite_name,
265 archive) = parse_upload_path(relative_path)
266 except UploadPathError, e:
267- # pick some defaults to create the NascentUploap() object.
268+ # pick some defaults to create the NascentUpload() object.
269 # We will be rejecting the upload so it doesn matter much.
270 distribution = getUtility(IDistributionSet)['ubuntu']
271 suite_name = None