launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10082
[Merge] lp:~cjwatson/launchpad/no-reupload-files into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/no-reupload-files into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/no-reupload-files/+merge/115702
== Summary ==
The package copier goes to lots of effort to re-upload files to the public librarian, when in fact that's unnecessary since the public and private librarian are actually the same thing and all we need to do is toggle the restricted flag. See #launchpad-dev discussion here:
http://irclogs.ubuntu.com/2012/07/18/%23launchpad-dev.html#t22:58
== Proposed fix ==
Toggle LFA.restricted rather than creating a new LFA and plugging it into all the old objects.
While renaming assertNewFiles to assertChangedFiles, I also took the opportunity to make its argument ordering match the conventional ordering for assertEqual arguments.
== Tests ==
bin/test -vvct test_copypackage -t test_packagecopyjob
== Lint ==
Just a bit of pre-existing lint that's a bit awkward to clean:
./lib/lp/soyuz/scripts/tests/test_copypackage.py
1350: E501 line too long (82 characters)
--
https://code.launchpad.net/~cjwatson/launchpad/no-reupload-files/+merge/115702
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/no-reupload-files into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2012-07-19 04:40:03 +0000
+++ database/schema/security.cfg 2012-07-19 11:07:23 +0000
@@ -893,13 +893,14 @@
public.karma = SELECT, INSERT
public.karmaaction = SELECT
public.language = SELECT
+public.libraryfilealias = SELECT, INSERT, UPDATE
public.message = SELECT, INSERT
public.messagechunk = SELECT, INSERT
public.milestone = SELECT
public.milestonetag = SELECT
public.packagecopyjob = SELECT, INSERT, DELETE
public.packagecopyrequest = SELECT, INSERT, UPDATE
-public.packagediff = SELECT, INSERT, UPDATE
+public.packagediff = SELECT, INSERT
public.packageset = SELECT, INSERT
public.packagesetgroup = SELECT
public.packagesetinclusion = SELECT, INSERT, UPDATE, DELETE
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2012-07-11 13:43:32 +0000
+++ lib/lp/soyuz/model/queue.py 2012-07-19 11:07:23 +0000
@@ -877,12 +877,11 @@
changes_file = release.package_upload.changesfile
for new_file in update_files_privacy(pub_record):
- debug(logger,
- "Re-uploaded %s to librarian" % new_file.filename)
+ debug(logger, "Made %s public" % new_file.filename)
for custom_file in self.customfiles:
update_files_privacy(custom_file)
debug(logger,
- "Re-uploaded custom file %s to librarian" %
+ "Made custom file %s public" %
custom_file.libraryfilealias.filename)
if ISourcePackagePublishingHistory.providedBy(pub_record):
pas_verify = BuildDaemonPackagesArchSpecific(
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2012-07-05 09:43:58 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2012-07-19 11:07:23 +0000
@@ -12,14 +12,11 @@
'do_copy',
'_do_delayed_copy',
'_do_direct_copy',
- 're_upload_file',
'update_files_privacy',
]
from itertools import repeat
from operator import attrgetter
-import os
-import tempfile
import apt_pkg
from lazr.delegates import delegates
@@ -29,8 +26,6 @@
from lp.app.errors import NotFoundError
from lp.buildmaster.enums import BuildStatus
from lp.services.database.bulk import load_related
-from lp.services.librarian.interfaces import ILibraryFileAliasSet
-from lp.services.librarian.utils import copy_and_close
from lp.soyuz.adapters.notification import notify
from lp.soyuz.adapters.packagelocation import build_package_location
from lp.soyuz.enums import (
@@ -58,41 +53,6 @@
)
-def re_upload_file(libraryfile, restricted=False):
- """Re-upload a librarian file to the public server.
-
- :param libraryfile: a `LibraryFileAlias`.
- :param restricted: whether or not the new file should be restricted.
-
- :return: A new `LibraryFileAlias`.
- """
- # XXX cprov 2009-06-12: This function could be incorporated in ILFA.
- # I just don't see a clear benefit in doing that right now.
-
- # Open the libraryfile for reading.
- libraryfile.open()
-
- # Make a temporary file to hold the download. It's annoying
- # having to download to a temp file but there are no guarantees
- # how large the files are, so using StringIO would be dangerous.
- fd, filepath = tempfile.mkstemp()
- temp_file = os.fdopen(fd, 'wb')
-
- # Read the old library file into the temp file.
- copy_and_close(libraryfile, temp_file)
-
- # Upload the file to the unrestricted librarian and make
- # sure the publishing record points to it.
- new_lfa = getUtility(ILibraryFileAliasSet).create(
- libraryfile.filename, libraryfile.content.filesize,
- open(filepath, "rb"), libraryfile.mimetype, restricted=restricted)
-
- # Junk the temporary file.
- os.remove(filepath)
-
- return new_lfa
-
-
# XXX cprov 2009-06-12: this function should be incorporated in
# IPublishing.
def update_files_privacy(pub_record):
@@ -101,40 +61,40 @@
:param pub_record: One of a SourcePackagePublishingHistory or
BinaryPackagePublishingHistory record.
- :return: a list of re-uploaded `LibraryFileAlias` objects.
+ :return: a list of changed `LibraryFileAlias` objects.
"""
package_files = []
archive = None
if ISourcePackagePublishingHistory.providedBy(pub_record):
archive = pub_record.archive
- # Re-upload the package files files if necessary.
+ # Unrestrict the package files files if necessary.
sourcepackagerelease = pub_record.sourcepackagerelease
package_files.extend(
[(source_file, 'libraryfile')
for source_file in sourcepackagerelease.files])
- # Re-upload the package diff files if necessary.
+ # Unrestrict the package diff files if necessary.
package_files.extend(
[(diff, 'diff_content')
for diff in sourcepackagerelease.package_diffs])
- # Re-upload the source upload changesfile if necessary.
+ # Unrestrict the source upload changesfile if necessary.
package_upload = sourcepackagerelease.package_upload
package_files.append((package_upload, 'changesfile'))
package_files.append((sourcepackagerelease, 'changelog'))
elif IBinaryPackagePublishingHistory.providedBy(pub_record):
archive = pub_record.archive
- # Re-upload the binary files if necessary.
+ # Unrestrict the binary files if necessary.
binarypackagerelease = pub_record.binarypackagerelease
package_files.extend(
[(binary_file, 'libraryfile')
for binary_file in binarypackagerelease.files])
- # Re-upload the upload changesfile file as necessary.
+ # Unrestrict the upload changesfile file as necessary.
build = binarypackagerelease.build
package_upload = build.package_upload
package_files.append((package_upload, 'changesfile'))
- # Re-upload the buildlog file as necessary.
+ # Unrestrict the buildlog file as necessary.
package_files.append((build, 'log'))
elif IPackageUploadCustom.providedBy(pub_record):
- # Re-upload the custom files included
+ # Unrestrict the custom files included
package_files.append((pub_record, 'libraryfilealias'))
# And set archive to the right attribute for PUCs
archive = pub_record.packageupload.archive
@@ -143,26 +103,22 @@
"pub_record is not one of SourcePackagePublishingHistory, "
"BinaryPackagePublishingHistory or PackageUploadCustom.")
- re_uploaded_files = []
+ changed_files = []
for obj, attr_name in package_files:
- old_lfa = getattr(obj, attr_name, None)
- # Only reupload restricted files published in public archives,
+ lfa = getattr(obj, attr_name, None)
+ # Only unrestrict restricted files published in public archives,
# not the opposite. We don't have a use-case for privatizing
# files yet.
- if (old_lfa is None or
- old_lfa.restricted == archive.private or
- old_lfa.restricted == False):
+ if (lfa is None or
+ lfa.restricted == archive.private or
+ lfa.restricted == False):
continue
- new_lfa = re_upload_file(old_lfa, restricted=archive.private)
- # Most of the attributes set here are not normally editable.
- # However, since we've just created all the publication records
- # here, and since we know that the calling user must have access to
- # the private source archive, we can get away with removing the
- # security proxy.
- setattr(removeSecurityProxy(obj), attr_name, new_lfa)
- re_uploaded_files.append(new_lfa)
+ # LibraryFileAlias.restricted is normally read-only, but we have a
+ # good excuse here.
+ removeSecurityProxy(lfa).restricted = archive.private
+ changed_files.append(lfa)
- return re_uploaded_files
+ return changed_files
# XXX cprov 2009-07-01: should be part of `ISourcePackagePublishingHistory`.
@@ -621,8 +577,8 @@
:param packageupload: The `IPackageUpload` that caused this publication
to be created.
:param unembargo: If True, allow copying restricted files from a private
- archive to a public archive, and re-upload them to the public
- librarian when doing so.
+ archive to a public archive, and unrestrict their library files when
+ doing so.
:param logger: An optional logger.
:raise CannotCopy when one or more copies were not allowed. The error
@@ -714,18 +670,17 @@
previous_version=old_version)
if not archive.private and has_restricted_files(source):
# Fix copies by overriding them according to the current
- # ancestry and re-upload files with privacy mismatch. We
+ # ancestry and unrestrict files with privacy mismatch. We
# must do this *after* calling notify (which only actually
# sends mail on commit), because otherwise the new changelog
# LFA won't be visible without a commit, which may not be
# safe here.
for pub_record in sub_copies:
pub_record.overrideFromAncestry()
- for new_file in update_files_privacy(pub_record):
+ for changed_file in update_files_privacy(pub_record):
if logger is not None:
logger.info(
- "Re-uploaded %s to librarian" %
- new_file.filename)
+ "Made %s public" % changed_file.filename)
overrides_index += 1
copies.extend(sub_copies)
=== modified file 'lib/lp/soyuz/scripts/tests/test_copypackage.py'
--- lib/lp/soyuz/scripts/tests/test_copypackage.py 2012-06-27 17:20:45 +0000
+++ lib/lp/soyuz/scripts/tests/test_copypackage.py 2012-07-19 11:07:23 +0000
@@ -73,7 +73,6 @@
CopyChecker,
do_copy,
PackageCopier,
- re_upload_file,
update_files_privacy,
)
from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
@@ -85,112 +84,12 @@
from lp.testing.dbuser import switch_dbuser
from lp.testing.layers import (
DatabaseLayer,
- LaunchpadFunctionalLayer,
LaunchpadZopelessLayer,
)
from lp.testing.mail_helpers import pop_notifications
from lp.testing.matchers import HasQueryCount
-class ReUploadFileTestCase(TestCaseWithFactory):
- """Test `ILibraryFileAlias` reupload helper.
-
- A `ILibraryFileAlias` object can be reupload to a different or
- the same privacy context.
-
- In both cases it will result in a new `ILibraryFileAlias` with
- the same contents than the original, but with usage attributes,
- like 'last_accessed' and 'hits', and expiration date reset.
- """
-
- layer = LaunchpadFunctionalLayer
-
- def assertSameContent(self, old, new):
- """Assert both given `ILibraryFileAlias` object are the same.
-
- Their filename, mimetype and file contents should be the same.
- """
- self.assertEqual(old.filename, new.filename, 'Filename mismatch.')
- self.assertEqual(old.mimetype, new.mimetype, 'MIME type mismatch.')
- self.assertEqual(old.read(), new.read(), 'Content mismatch.')
-
- def assertFileIsReset(self, reuploaded_file):
- """Assert the given `ILibraryFileAlias` attributes were reset.
-
- The expiration date and the hits counter are reset and the
- last access records was on file creation.
- """
- self.assertIsNone(reuploaded_file.expires)
- self.assertEqual(
- reuploaded_file.last_accessed, reuploaded_file.date_created)
- self.assertEqual(reuploaded_file.hits, 0)
-
- def testReUploadFileToTheSameContext(self):
- # Re-uploading a librarian file to the same privacy/server
- # context results in a new `LibraryFileAlias` object with
- # the same content and empty expiration date and usage counter.
- old = self.factory.makeLibraryFileAlias()
- transaction.commit()
-
- new = re_upload_file(old)
- transaction.commit()
-
- self.assertIsNot(old, new)
- self.assertEqual(
- old.restricted, new.restricted, 'New file still private.')
- self.assertSameContent(old, new)
- self.assertFileIsReset(new)
-
- def testReUploadFileToPublic(self):
- # Re-uploading a private librarian file to the public context
- # results in a new restricted `LibraryFileAlias` object with
- # the same content and empty expiration date and usage counter.
- private_file = self.factory.makeLibraryFileAlias(restricted=True)
- transaction.commit()
-
- public_file = re_upload_file(private_file)
- transaction.commit()
-
- self.assertIsNot(private_file, public_file)
- self.assertFalse(
- public_file.restricted, 'New file still private.')
- self.assertSameContent(private_file, public_file)
- self.assertFileIsReset(public_file)
-
- def testReUploadFileToPrivate(self):
- # Re-uploading a public librarian file to the private context
- # results in a new restricted `LibraryFileAlias` object with
- # the same content and empty expiration date and usage counter.
- public_file = self.factory.makeLibraryFileAlias()
- transaction.commit()
-
- private_file = re_upload_file(public_file, restricted=True)
- transaction.commit()
-
- self.assertIsNot(public_file, private_file)
- self.assertTrue(
- private_file.restricted, 'New file still public')
- self.assertSameContent(public_file, private_file)
- self.assertFileIsReset(private_file)
-
- def test_re_upload_file_does_not_leak_file_descriptors(self):
- # Reuploading a library file doesn't leak file descriptors.
- private_file = self.factory.makeLibraryFileAlias(restricted=True)
- transaction.commit()
-
- def number_of_open_files():
- return len(os.listdir('/proc/%d/fd/' % os.getpid()))
- previously_open_files = number_of_open_files()
-
- public_file = re_upload_file(private_file)
- # The above call would've raised an error if the upload failed, but
- # better safe than sorry.
- self.assertIsNot(None, public_file)
-
- open_files = number_of_open_files() - previously_open_files
- self.assertEqual(0, open_files)
-
-
class UpdateFilesPrivacyTestCase(TestCaseWithFactory):
"""Test publication `updateFilesPrivacy` helper.
@@ -218,10 +117,11 @@
'BinaryPackagePublishingHistory or PackageUploadCustom.',
update_files_privacy, None)
- def assertNewFiles(self, new_files, result):
- """Check new files created during update_files_privacy."""
+ def assertChangedFiles(self, expected, changed_files):
+ """Check files changed during update_files_privacy."""
self.assertEqual(
- sorted([new_file.filename for new_file in new_files]), result)
+ expected,
+ sorted(changed_file.filename for changed_file in changed_files))
def _checkSourceFilesPrivacy(self, pub_record, restricted,
expected_n_files):
@@ -295,9 +195,9 @@
# In this scenario update_files_privacy does nothing. The 3 testing
# source files are still private.
- new_files = update_files_privacy(private_source)
+ changed_files = update_files_privacy(private_source)
self.layer.commit()
- self.assertNewFiles(new_files, [])
+ self.assertChangedFiles([], changed_files)
self.assertSourceFilesArePrivate(private_source, 3)
# Copy The original source to a public PPA, at this point all
@@ -310,15 +210,15 @@
public_archive)
self.assertSourceFilesArePrivate(public_source, 3)
- # update_files_privacy on the copied source moves all files from
- # the restricted librarian to the public one.
- new_files = update_files_privacy(public_source)
+ # update_files_privacy on the copied source makes all files
+ # unrestricted.
+ changed_files = update_files_privacy(public_source)
self.layer.commit()
- self.assertNewFiles(new_files, [
+ self.assertChangedFiles([
'foo.diff.gz',
'foo_666.dsc',
'foo_666_source.changes',
- ])
+ ], changed_files)
self.assertSourceFilesArePublic(public_source, 3)
# Note that the files from the original source are now also public,
@@ -370,9 +270,9 @@
# In this scenario update_files_privacy does nothing. The 3 testing
# binary files are still private.
- new_files = update_files_privacy(private_binary)
+ changed_files = update_files_privacy(private_binary)
self.layer.commit()
- self.assertNewFiles(new_files, [])
+ self.assertChangedFiles([], changed_files)
self.assertBinaryFilesArePrivate(private_binary, 3)
# Copy The original binary to a public PPA, at this point all
@@ -385,17 +285,16 @@
public_archive)[0]
self.assertBinaryFilesArePrivate(public_binary, 3)
- # update_files_privacy on the copied binary moves all files from
- # the restricted librarian to the public one.
- new_files = update_files_privacy(public_binary)
+ # update_files_privacy on the copied binary makes all files
+ # unrestricted.
+ changed_files = update_files_privacy(public_binary)
self.layer.commit()
- self.assertNewFiles(
- new_files, [
- 'buildlog_ubuntutest-breezy-autotest-i386.'
- 'foo_666_FULLYBUILT.txt.gz',
- 'foo-bin_666_all.deb',
- 'foo-bin_666_i386.changes',
- ])
+ self.assertChangedFiles([
+ 'buildlog_ubuntutest-breezy-autotest-i386.'
+ 'foo_666_FULLYBUILT.txt.gz',
+ 'foo-bin_666_all.deb',
+ 'foo-bin_666_i386.changes',
+ ], changed_files)
self.assertBinaryFilesArePublic(public_binary, 3)
# Note that the files from the original binary are now also public,
@@ -432,12 +331,12 @@
self.assertSourceFilesArePublic(copied_source, 3)
self.assertBinaryFilesArePublic(copied_binary, 3)
- new_source_files = update_files_privacy(copied_source)
- new_binary_files = update_files_privacy(copied_binary)
+ changed_source_files = update_files_privacy(copied_source)
+ changed_binary_files = update_files_privacy(copied_binary)
self.layer.commit()
- self.assertNewFiles(new_source_files, [])
+ self.assertChangedFiles([], changed_source_files)
self.assertSourceFilesArePublic(copied_source, 3)
- self.assertNewFiles(new_binary_files, [])
+ self.assertChangedFiles([], changed_binary_files)
self.assertBinaryFilesArePublic(copied_binary, 3)
@@ -2938,15 +2837,14 @@
'INFO \tfoo 1.1 in warty',
'INFO \tfoo-bin 1.1 in warty hppa',
'INFO \tfoo-bin 1.1 in warty i386',
- 'INFO Re-uploaded foo_1.1.dsc to librarian',
- 'INFO Re-uploaded diff_file to librarian',
- 'INFO Re-uploaded foo_1.1_source.changes to librarian',
- 'INFO Re-uploaded changelog to librarian',
- 'INFO Re-uploaded foo-bin_1.1_all.deb to librarian',
- 'INFO Re-uploaded foo-bin_1.1_i386.changes to librarian',
- 'INFO Re-uploaded '
- 'buildlog_ubuntu-warty-i386.foo_1.1_FULLYBUILT.txt.gz to '
- 'librarian',
+ 'INFO Made foo_1.1.dsc public',
+ 'INFO Made diff_file public',
+ 'INFO Made foo_1.1_source.changes public',
+ 'INFO Made changelog public',
+ 'INFO Made foo-bin_1.1_all.deb public',
+ 'INFO Made foo-bin_1.1_i386.changes public',
+ 'INFO Made '
+ 'buildlog_ubuntu-warty-i386.foo_1.1_FULLYBUILT.txt.gz public',
'INFO Copied:',
'INFO \tfoo 1.1 in warty',
'INFO \tfoo-bin 1.1 in warty hppa',
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2012-07-16 11:12:23 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2012-07-19 11:07:23 +0000
@@ -1053,8 +1053,7 @@
self.assertEqual(BugTaskStatus.NEW, bugtask280.status)
def test_copying_unembargoes_files(self):
- # The unembargo flag causes the job to re-upload restricted files to
- # the public librarian.
+ # The unembargo flag causes the job to unrestrict files.
self.distroseries.status = SeriesStatus.CURRENT
target_archive = self.factory.makeArchive(
self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY)
Follow ups
