launchpad-reviewers team mailing list archive

[Celso Providelo <celso.providelo@xxxxxxxxxxxxx>]

Review: Needs Fixing

See inline comments

Diff comments:

> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py	2014-07-28 01:58:49 +0000
> +++ lib/lp/archivepublisher/publishing.py	2014-07-29 19:20:26 +0000
> @@ -755,6 +755,29 @@
>          for path in paths:
>              os.utime(path, (latest_timestamp, latest_timestamp))
>  
> +    def _getI18nFiles(self, distroseries, pocket, component):
> +        # Get and return a list of all i18n files and filepaths.
> +        suite = distroseries.getSuite(pocket)
> +
> +        i18n_path = os.path.join(component, "i18n")
> +        i18n_dir = os.path.join(self._config.distsroot, suite, i18n_path)
> +        i18n_files = []
> +        i18n_filepaths = []
> +        try:
> +            for i18n_file in os.listdir(i18n_dir):
> +                if not i18n_file.startswith('Translation-'):
> +                    continue
> +                if not i18n_file.endswith('.bz2'):
> +                    # Save bandwidth: mirrors should only need the .bz2
> +                    # versions.
> +                    continue
> +                i18n_filepaths.append(os.path.join(i18n_path, i18n_file))
> +                i18n_files.append(i18n_file)
> +        except OSError as e:
> +            if e.errno != errno.ENOENT:
> +                raise
> +        return i18n_files, i18n_filepaths

Why returning file paths and their basename()s ?
You could return only full paths and let the callsite that needs basename() to do so ([os.path.basename(path) for path in i18_filepaths])

> +
>      def _writeSuite(self, distroseries, pocket):
>          """Write out the Release files for the provided suite."""
>          # XXX: kiko 2006-08-24: Untested method.
> @@ -774,6 +797,7 @@
>          all_architectures = [
>              a.architecturetag for a in distroseries.enabled_architectures]
>          all_files = set()
> +        files = set()

I don't understand why you need another set() to be included later on in the existing all_files set() ?

>          for component in all_components:
>              self._writeSuiteSource(
>                  distroseries, pocket, component, all_files)
> @@ -782,6 +806,9 @@
>                      distroseries, pocket, component, architecture, all_files)
>              self._writeSuiteI18n(
>                  distroseries, pocket, component, all_files)
> +            i18n_files, i18n_filepaths = self._getI18nFiles(
> +                distroseries, pocket, component)
> +            files.update(i18n_filepaths)
>  
>          drsummary = "%s %s " % (self.distro.displayname,
>                                  distroseries.displayname)
> @@ -808,7 +835,10 @@
>              release_file["NotAutomatic"] = "yes"
>              release_file["ButAutomaticUpgrades"] = "yes"
>  
> -        for filename in sorted(all_files, key=os.path.dirname):
> +        sorted(all_files, key=os.path.dirname)

sorted() is not in-place, it returns a new list. 'all_files' still unsorted after this line.

> +        all_files.update(files)

ditto.

> +
> +        for filename in sorted(all_files):

This is not preserving the previous behaviour (sorting using dirname()).

>              entry = self._readIndexFileContents(suite, filename)
>              if entry is None:
>                  continue
> @@ -902,19 +932,8 @@
>  
>          i18n_dir = os.path.join(self._config.distsroot, suite, component,
>                                  "i18n")
> -        i18n_files = []
> -        try:
> -            for i18n_file in os.listdir(i18n_dir):
> -                if not i18n_file.startswith('Translation-'):
> -                    continue
> -                if not i18n_file.endswith('.bz2'):
> -                    # Save bandwidth: mirrors should only need the .bz2
> -                    # versions.
> -                    continue
> -                i18n_files.append(i18n_file)
> -        except OSError as e:
> -            if e.errno != errno.ENOENT:
> -                raise
> +        i18n_files, i18n_filepaths = self._getI18nFiles(
> +            distroseries, pocket, component)

as mentioned above, get the full paths from the base method and build a list with their basename()s here.

>          if not i18n_files:
>              # If the i18n directory doesn't exist or is empty, we don't need
>              # to index it.
> 
> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py	2014-07-28 01:58:49 +0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py	2014-07-29 19:20:26 +0000
> @@ -1342,6 +1342,10 @@
>  
>          self.getPubSource(filecontent='Hello world')
>  
> +        # Make sure that apt-ftparchive generates i18n/Translation-en* files.
> +        ds = self.ubuntutest.getSeries('breezy-autotest')
> +        ds.include_long_descriptions = False
> +

I suppose these tests are broken, at least the Release file content/checksum checks.

>          publisher.A_publish(False)
>          publisher.C_doFTPArchive(False)
>  
> 


-- 
https://code.launchpad.net/~cjohnston/launchpad/1313576-release-hashes/+merge/228744
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.