launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00099
[Merge] lp:~abentley/launchpad/no-write-cwd into lp:launchpad/devel
Aaron Bentley has proposed merging lp:~abentley/launchpad/no-write-cwd into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#595957 archive uploader tries to move the changelog to the current working directory
https://bugs.launchpad.net/bugs/595957
= 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
--
https://code.launchpad.net/~abentley/launchpad/no-write-cwd/+merge/29965
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/no-write-cwd into lp:launchpad/devel.
=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py 2010-06-25 21:35:15 +0000
+++ lib/lp/archiveuploader/dscfile.py 2010-07-15 09:50:57 +0000
@@ -13,7 +13,7 @@
'SignableTagFile',
'DSCFile',
'DSCUploadedFile',
- 'findAndMoveChangelog',
+ 'findChangelog',
'findCopyright',
]
@@ -204,6 +204,8 @@
else:
self.processSignature()
+ self.unpacked_dir = None
+
#
# Useful properties.
#
@@ -483,18 +485,19 @@
"Verifying uploaded source package by unpacking it.")
# Get a temporary dir together.
- tmpdir = tempfile.mkdtemp()
+ self.unpacked_dir = tempfile.mkdtemp()
# chdir into it
cwd = os.getcwd()
- os.chdir(tmpdir)
- dsc_in_tmpdir = os.path.join(tmpdir, self.filename)
+ os.chdir(self.unpacked_dir)
+ dsc_in_tmpdir = os.path.join(self.unpacked_dir, self.filename)
package_files = self.files + [self]
try:
for source_file in package_files:
os.symlink(source_file.filepath,
- os.path.join(tmpdir, source_file.filename))
+ os.path.join(self.unpacked_dir,
+ source_file.filename))
args = ["dpkg-source", "-sn", "-x", dsc_in_tmpdir]
dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
stderr=subprocess.PIPE)
@@ -517,8 +520,9 @@
# SourcePackageRelease records.
# Check if 'dpkg-source' created only one directory.
- temp_directories = [dirname for dirname in os.listdir(tmpdir)
- if os.path.isdir(dirname)]
+ temp_directories = [
+ dirname for dirname in os.listdir(self.unpacked_dir)
+ if os.path.isdir(dirname)]
if len(temp_directories) > 1:
yield UploadError(
'Unpacked source contains more than one directory: %r'
@@ -528,31 +532,33 @@
# name (<sourcename>-<no_epoch(no_revision(version))>).
# Locate both the copyright and changelog files for later processing.
- for error in findCopyright(self, tmpdir, self.logger):
+ for error in findCopyright(self, self.unpacked_dir, self.logger):
yield error
- for error in findAndMoveChangelog(self, cwd, tmpdir, self.logger):
+ for error in findChangelog(self, self.unpacked_dir, self.logger):
yield error
self.logger.debug("Cleaning up source tree.")
+ self.logger.debug("Done")
+
+ def cleanUp(self):
+ if self.unpacked_dir is None:
+ return
try:
- shutil.rmtree(tmpdir)
+ shutil.rmtree(self.unpacked_dir)
except OSError, error:
# XXX: dsilvers 2006-03-15: We currently lack a test for this.
if errno.errorcode[error.errno] != 'EACCES':
- yield UploadError(
+ raise UploadError(
"%s: couldn't remove tmp dir %s: code %s" % (
- self.filename, tmpdir, error.errno))
+ self.filename, self.unpacked_dir, error.errno))
else:
- yield UploadWarning(
- "%s: Couldn't remove tree, fixing up permissions." %
- self.filename)
- result = os.system("chmod -R u+rwx " + tmpdir)
+ result = os.system("chmod -R u+rwx " + self.unpacked_dir)
if result != 0:
- yield UploadError("chmod failed with %s" % result)
- shutil.rmtree(tmpdir)
+ raise UploadError("chmod failed with %s" % result)
+ shutil.rmtree(self.unpacked_dir)
+ self.unpacked_dir = None
- self.logger.debug("Done")
def findBuild(self):
"""Find and return the SourcePackageRecipeBuild, if one is specified.
@@ -733,15 +739,14 @@
logger.debug("Copying copyright contents.")
dsc_file.copyright = open(copyright_file).read().strip()
-def findAndMoveChangelog(dsc_file, target_dir, source_dir, logger):
+def findChangelog(dsc_file, source_dir, logger):
"""Find and move any debian/changelog.
- This function finds the changelog file within the source package and
- moves it to target_dir. The changelog file is later uploaded to the
- librarian by DSCFile.storeInDatabase().
+ This function finds the changelog file within the source package. The
+ changelog file is later uploaded to the librarian by
+ DSCFile.storeInDatabase().
:param dsc_file: A DSCFile object where the copyright will be stored.
- :param target_dir: The directory where the changelog will end up.
:param source_dir: The directory where the source was extracted.
:param logger: A logger object for debug output.
"""
@@ -756,9 +761,8 @@
return
# Move the changelog file out of the package direcotry
- logger.debug("Found changelog contents; moving to root directory")
- dsc_file.changelog_path = os.path.join(target_dir, "changelog")
- shutil.move(changelog_file, dsc_file.changelog_path)
+ logger.debug("Found changelog")
+ dsc_file.changelog_path = changelog_file
def check_format_1_0_files(filename, file_type_counts, component_counts,
bzip2_count):
=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py 2010-05-27 22:18:16 +0000
+++ lib/lp/archiveuploader/nascentupload.py 2010-07-15 09:50:57 +0000
@@ -839,6 +839,12 @@
'Exception while accepting:\n %s' % e, exc_info=True)
self.do_reject(notify)
return False
+ else:
+ self.cleanUp()
+
+ def cleanUp(self):
+ if self.changes.dsc is not None:
+ self.changes.dsc.cleanUp()
def do_reject(self, notify=True):
"""Reject the current upload given the reason provided."""
@@ -869,6 +875,7 @@
self.queue_root.notify(summary_text=self.rejection_message,
changes_file_object=changes_file_object, logger=self.logger)
changes_file_object.close()
+ self.cleanUp()
def _createQueueEntry(self):
"""Return a PackageUpload object."""
=== modified file 'lib/lp/archiveuploader/tests/test_dscfile.py'
--- lib/lp/archiveuploader/tests/test_dscfile.py 2010-06-25 21:35:15 +0000
+++ lib/lp/archiveuploader/tests/test_dscfile.py 2010-07-15 09:50:57 +0000
@@ -8,11 +8,14 @@
import os
import unittest
+from canonical.config import config
+from canonical.launchpad.scripts.logger import QuietFakeLogger
from canonical.testing.layers import LaunchpadZopelessLayer
from lp.archiveuploader.dscfile import (
- findAndMoveChangelog, findCopyright)
+ DSCFile, findChangelog, findCopyright)
from lp.archiveuploader.nascentuploadfile import UploadError
from lp.archiveuploader.tests import mock_logger_quiet
+from lp.archiveuploader.uploadpolicy import BuildDaemonUploadPolicy
from lp.testing import TestCase, TestCaseWithFactory
@@ -28,7 +31,6 @@
os.makedirs(self.dir_path)
self.copyright_path = os.path.join(self.dir_path, "copyright")
self.changelog_path = os.path.join(self.dir_path, "changelog")
- self.changelog_dest = os.path.join(self.tmpdir, "changelog")
self.dsc_file = self.MockDSCFile()
def testBadDebianCopyright(self):
@@ -67,8 +69,8 @@
dangling symlink in an attempt to try and access files on the system
processing the source packages."""
os.symlink("/etc/passwd", self.changelog_path)
- errors = list(findAndMoveChangelog(
- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+ errors = list(findChangelog(
+ self.dsc_file, self.tmpdir, mock_logger_quiet))
self.assertEqual(len(errors), 1)
self.assertIsInstance(errors[0], UploadError)
@@ -83,12 +85,12 @@
file.write(changelog)
file.close()
- errors = list(findAndMoveChangelog(
- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+ errors = list(findChangelog(
+ self.dsc_file, self.tmpdir, mock_logger_quiet))
self.assertEqual(len(errors), 0)
self.assertEqual(self.dsc_file.changelog_path,
- self.changelog_dest)
+ self.changelog_path)
def testOversizedFile(self):
@@ -108,8 +110,8 @@
file.write(empty_file)
file.close()
- errors = list(findAndMoveChangelog(
- self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+ errors = list(findChangelog(
+ self.dsc_file, self.tmpdir, mock_logger_quiet))
self.assertIsInstance(errors[0], UploadError)
self.assertEqual(
@@ -122,13 +124,29 @@
layer = LaunchpadZopelessLayer
+ def getDscFile(self, name):
+ dsc_path = os.path.join(config.root,
+ "lib/lp/archiveuploader/tests/data/suite", name, name + '.dsc')
+ class Changes:
+ architectures = ['source']
+ logger = QuietFakeLogger()
+ policy = BuildDaemonUploadPolicy()
+ policy.distroseries = self.factory.makeDistroSeries()
+ policy.archive = self.factory.makeArchive()
+ policy.distro = policy.distroseries.distribution
+ return DSCFile(dsc_path, 'digest', 0, 'main/editors',
+ 'priority', 'package', 'version', Changes, policy, logger)
+
def test_ReadOnlyCWD(self):
"""Processing a file should work when cwd is read-only."""
tempdir = self.useTempDir()
- dsc_file = self.factory.makeDscFile(tempdir)
os.chmod(tempdir, 0555)
try:
- list(dsc_file.unpackAndCheckSource())
+ dsc_file = self.getDscFile('bar_1.0-1')
+ try:
+ list(dsc_file.verify())
+ finally:
+ dsc_file.cleanUp()
finally:
os.chmod(tempdir, 0755)
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py 2010-06-29 14:01:01 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py 2010-07-15 09:50:57 +0000
@@ -293,7 +293,7 @@
(distribution, suite_name,
archive) = parse_upload_path(relative_path)
except UploadPathError, e:
- # pick some defaults to create the NascentUploap() object.
+ # pick some defaults to create the NascentUpload() object.
# We will be rejecting the upload so it doesn matter much.
distribution = getUtility(IDistributionSet)['ubuntu']
suite_name = None
