Hi Jelmer, I have just a few comments/questions below, but this looks good. And thanks a lot for getting rid of the untested/duplicated code. review approve code On Thu, 2010-02-04 at 16:24 +0000, Jelmer Vernooij wrote: > === modified file 'lib/canonical/launchpad/scripts/logger.py' > --- lib/canonical/launchpad/scripts/logger.py 2009-10-16 18:07:01 +0000 > +++ lib/canonical/launchpad/scripts/logger.py 2010-02-04 16:24:23 +0000 [...] > @@ -847,19 +854,14 @@ > Return the fetched filename if it was present in Librarian or None > if it wasn't. > """ > - # XXX cprov 2007-01-10 bug=78683: Looking for files within ubuntu > - # only. It doesn't affect the usual sync-source procedure. However > - # it needs to be revisited for derivation, we probably need > - # to pass the target distribution in order to make proper lookups. > - ubuntu = getUtility(IDistributionSet)['ubuntu'] Does this mean the bug above will be fixed too? > try: > - libraryfilealias = ubuntu.getFileByName( > + libraryfilealias = self.todistro.getFileByName( > filename, source=True, binary=False) > except NotFoundError: > return None > > - self.debug( > - "\t%s: already in distro - downloading from librarian" % > + self.logger.info( > + "%s: already in distro - downloading from librarian" % > filename) > > output_file = open(filename, 'w') > @@ -871,23 +873,23 @@ > """Try to fetch files from Librarian. > > It raises SyncSourceError if anything else then an > - 'orig.tar.gz' was found in Librarian. > - Return a boolean indicating whether or not the 'orig.tar.gz' is > - required in the upload. > + orig tarball was found in Librarian. > + Return the names of the files retrieved from the librarian. > """ > - orig_filename = None > + retrieved = [] > for filename in self.files.keys(): > if not self.fetchFileFromLibrarian(filename): > continue > + file_type = determine_source_file_type(filename) > # set the return code if an orig was, in fact, > # fetched from Librarian > - if filename.endswith("orig.tar.gz"): > - orig_filename = filename > - else: > + if not file_type in (SourcePackageFileType.ORIG_TARBALL, > + SourcePackageFileType.COMPONENT_ORIG_TARBALL): The indentation above is a bit odd; the two elements of the tuple should be lined up. > raise SyncSourceError( > - 'Oops, only orig.tar.gz can be retrieved from librarian') > + 'Oops, only orig tarball can be retrieved from librarian.') > + retrieved.append(filename) > > - return orig_filename > + return retrieved > > def fetchSyncFiles(self): > """Fetch files from the original sync source. > > === modified file 'scripts/ftpmaster-tools/sync-source.py' > --- scripts/ftpmaster-tools/sync-source.py 2010-02-02 22:34:39 +0000 > +++ scripts/ftpmaster-tools/sync-source.py 2010-02-04 16:24:23 +0000 > @@ -130,7 +131,7 @@ > > def generate_changes(dsc, dsc_files, suite, changelog, urgency, closes, > lp_closes, section, priority, description, > - have_orig_tar_gz, requested_by, origin): > + files_from_librarian, requested_by, origin): > """Generate a .changes as a string""" > > # XXX cprov 2007-07-03: > @@ -164,7 +165,7 @@ > changes += changelog > changes += "Files: \n" > for filename in dsc_files: > - if filename.endswith(".orig.tar.gz") and have_orig_tar_gz: > + if filename in files_from_librarian: > continue > changes += " %s %s %s %s %s\n" % (dsc_files[filename]["md5sum"], > dsc_files[filename]["size"], > @@ -363,11 +364,23 @@ > > > def import_dsc(dsc_filename, suite, previous_version, signing_rules, > - have_orig_tar_gz, requested_by, origin, current_sources, > + files_from_librarian, requested_by, origin, current_sources, > current_binaries): > - dsc = dak_utils.parse_changes(dsc_filename, signing_rules) > - dsc_files = dak_utils.build_file_list(dsc, is_a_dsc=1)\ > - > + dsc_file = open(dsc_filename, 'r') > + dsc = Dsc(dsc_file) > + > + if signing_rules.startswith("must be signed"): > + dsc_file.seek(0) > + (gpg_pre, payload, gpg_post) = dsc.split_gpg_and_payload(dsc_file) > + if gpg_pre == [] and gpg_post == []: > + dak_utils.fubar("signature required for %s but not present" Nice function name. > + % dsc_filename) > + if signing_rules == "must be signed and valid": > + if (gpg_pre[0] != "-----BEGIN PGP SIGNED MESSAGE-----" or > + gpg_post[0] != "-----BEGIN PGP SIGNATURE-----"): > + dak_utils.fubar("signature for %s invalid %r %r" % (dsc_filename, gpg_pre, gpg_post)) > + > + dsc_files = dict((entry['name'], entry) for entry in dsc['files']) > check_dsc(dsc, current_sources, current_binaries) > > # Add the .dsc itself to dsc_files so it's listed in the Files: field > @@ -400,7 +413,7 @@ > > changes = generate_changes( > dsc, dsc_files, suite, changelog, urgency, closes, lp_closes, > - section, priority, description, have_orig_tar_gz, requested_by, > + section, priority, description, files_from_librarian, requested_by, > origin) > > # XXX cprov 2007-07-03: Soyuz wants an unsigned changes > @@ -565,86 +578,21 @@ > if not Sources.has_key(pkg): > dak_utils.fubar("%s doesn't exist in the Sources file." % (pkg)) > > - have_orig_tar_gz = False > - > - # Fetch the source > - files = Sources[pkg]["files"] > - for filename in files: > - # First see if we can find the source in the librarian > - archive_ids = [a.id for a in Options.todistro.all_distro_archives] > - query = """ > - SELECT DISTINCT ON (LibraryFileContent.sha1, LibraryFileContent.filesize) > - LibraryFileAlias.id > - FROM SourcePackageFilePublishing, LibraryFileAlias, LibraryFileContent > - WHERE > - LibraryFileAlias.id = SourcePackageFilePublishing.libraryfilealias AND > - LibraryFileContent.id = LibraryFileAlias.content AND > - SourcePackageFilePublishing.libraryfilealiasfilename = %s AND > - SourcePackageFilePublishing.archive IN %s > - """ % sqlvalues(filename, archive_ids) > - cur = cursor() > - cur.execute(query) > - results = cur.fetchall() > - if results: > - if not filename.endswith("orig.tar.gz"): > - dak_utils.fubar( > - "%s (from %s) is in the DB but isn't an orig.tar.gz. " > - "(Probably published in an older release)" > - % (filename, pkg)) > - if len(results) > 1: > - dak_utils.fubar( > - "%s (from %s) returns multiple IDs (%s) for " > - "orig.tar.gz. Help?" % (filename, pkg, results)) > - have_orig_tar_gz = filename > - print (" - <%s: already in distro - downloading from librarian>" > - % (filename)) > - output_file = open(filename, 'w') > - librarian_input = Library.getFileByAlias(results[0][0]) > - output_file.write(librarian_input.read()) > - output_file.close() > - continue > - > - # Download the file > - download_f = ( > - "%s%s" % (origin["url"], files[filename]["remote filename"])) > - if not os.path.exists(filename): > - print " - <%s: downloading from %s>" % (filename, origin["url"]) > - sys.stdout.flush() > - urllib.urlretrieve(download_f, filename) > - else: > - print " - <%s: cached>" % (filename) > - > - # Check md5sum and size match Source > - actual_md5sum = md5sum_file(filename) > - expected_md5sum = files[filename]["md5sum"] > - if actual_md5sum != expected_md5sum: > - dak_utils.fubar( > - "%s: md5sum check failed (%s [actual] vs. %s [expected])." > - % (filename, actual_md5sum, expected_md5sum)) > - actual_size = os.stat(filename)[stat.ST_SIZE] > - expected_size = int(files[filename]["size"]) > - if actual_size != expected_size: > - dak_utils.fubar( > - "%s: size mismatch (%s [actual] vs. %s [expected])." > - % (filename, actual_size, expected_size)) > - > - # Remember the name of the .dsc file > - if filename.endswith(".dsc"): > - dsc_filename = os.path.abspath(filename) > - > - if origin["dsc"] == "must be signed and valid": > - signing_rules = 1 > - elif origin["dsc"] == "must be signed": > - signing_rules = 0 > - else: > - signing_rules = -1 > - > - import_dsc(dsc_filename, suite, previous_version, signing_rules, > - have_orig_tar_gz, requested_by, origin, current_sources, > - current_binaries) Great to see all this going away, but, have you tested your branch on dogfood or something? I ask because since (as you said) most of this was untested, there's a big chance some bits got lost during this refactoring. > - > - if have_orig_tar_gz: > - os.unlink(have_orig_tar_gz) > + syncsource = SyncSource(Sources[pkg]["files"], origin, Log, > + urllib.urlretrieve, Options.todistro) > + try: > + files_from_librarian = syncsource.fetchLibrarianFiles() > + dsc_filename = syncsource.fetchSyncFiles() > + syncsource.checkDownloadedFiles() > + except SyncSourceError, e: > + dak_utils.fubar("Fetching files failed: %s" % (str(e),)) > + > + if dsc_filename is None: > + dak_utils.fubar("No dsc filename in %r" % Sources[pkg]["files"].keys()) > + > + import_dsc(os.path.abspath(dsc_filename), suite, previous_version, > + origin["dsc"], files_from_librarian, requested_by, origin, > + current_sources, current_binaries) > > > def do_diff(Sources, Suite, origin, arguments, current_binaries): -- Guilherme Salgado