launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lpbuildd/binarypackage.py'
> --- lpbuildd/binarypackage.py	2015-05-31 23:36:45 +0000
> +++ lpbuildd/binarypackage.py	2015-07-03 19:01:42 +0000
> @@ -103,6 +126,119 @@
>          args.append(self._dscfile)
>          self.runSubProcess(self._sbuildpath, args)
>  
> +    def getAvailablePackages(self):
> +        """Return the available binary packages in the chroot.
> +
> +        :return: A dictionary mapping package names to a set of the
> +            available versions of each package.
> +        """
> +        available = defaultdict(set)
> +        apt_lists = os.path.join(
> +            self.chroot_path, "var", "lib", "apt", "lists")
> +        for name in sorted(os.listdir(apt_lists)):
> +            if name.endswith("_Packages"):
> +                path = os.path.join(apt_lists, name)
> +                with open(path, "rb") as packages_file:
> +                    for section in apt_pkg.TagFile(packages_file):
> +                        available[section["package"]].add(section["version"])
> +                        if "provides" in section:
> +                            provides = PkgRelation.parse_relations(
> +                                section["provides"])
> +                            for provide in provides:
> +                                # Disjunctions are currently undefined here.
> +                                if len(provide) > 1:
> +                                    continue
> +                                # Virtual packages may only provide an exact
> +                                # version or none.
> +                                provide_version = provide[0].get("version")
> +                                if (provide_version is not None and
> +                                        provide_version[0] != "="):
> +                                    continue
> +                                available[provide[0]["name"]].add(
> +                                    provide_version[1]
> +                                    if provide_version else None)
> +        return available
> +

Did you consider using python-apt for this?

> +    def getBuildDepends(self, dscpath, arch_indep):
> +        """Get the build-dependencies of a source package.
> +
> +        :param dscpath: The path of a .dsc file.
> +        :param arch_indep: True iff we were asked to build the
> +            architecture-independent parts of this source package.
> +        :return: The build-dependencies, in the form returned by
> +            `debian.deb822.PkgRelation.parse_relations`.
> +        """
> +        deps = []
> +        with open(dscpath, "rb") as dscfile:
> +            dsc = Dsc(dscfile)
> +            fields = ["Build-Depends"]
> +            if arch_indep:
> +                fields.append("Build-Depends-Indep")
> +            for field in fields:
> +                if field in dsc:
> +                    deps.extend(PkgRelation.parse_relations(dsc[field]))
> +        return deps
> +
> +    def relationMatches(self, dep, available):
> +        """Return True iff a dependency matches para.
> +

para?

> +        :param dep: A dictionary with at least a "name" key, perhaps also a
> +            "version" key, and optionally other keys, of the kind returned
> +            in a list of lists by
> +            `debian.deb822.PkgRelation.parse_relations`.
> +        :param available: A dictionary mapping package names to a list of
> +            the available versions of each package.
> +        """
> +        if dep["name"] not in available:
> +            return False
> +        if dep.get("version") is None:
> +            return True
> +        dep_version = dep.get("version")
> +        operator_map = {
> +            "<<": (lambda a, b: a < b),
> +            "<=": (lambda a, b: a <= b),
> +            "=": (lambda a, b: a == b),
> +            ">=": (lambda a, b: a >= b),
> +            ">>": (lambda a, b: a > b),
> +            }
> +        operator = operator_map[dep_version[0]]
> +        want_version = dep_version[1]
> +        for version in available[dep["name"]]:
> +            if (version is not None and
> +                    operator(Version(version), want_version)):
> +                return True
> +        return False
> +
> +    def analyseDepWait(self, deps, avail):
> +        """Work out the correct dep-wait for a failed build, if any.
> +
> +        We only consider direct build-dependencies, because indirect ones
> +        can't easily be turned into an accurate dep-wait: they might be
> +        resolved either by an intermediate package changing or by the
> +        missing dependency becoming available.  We err on the side of
> +        failing a build rather than setting a dep-wait if it's possible that
> +        the dep-wait might be incorrect.  Any exception raised during the
> +        analysis causes the build to be failed.
> +
> +        :param deps: The source package's build-dependencies, in the form
> +            returned by `debian.deb822.PkgRelation.parse_relations`.
> +        :param avail: A dictionary mapping package names to a set of the
> +            available versions of each package.
> +        :return: A dependency relation string representing the packages that
> +            need to become available before this build can proceed, or None
> +            if the build should be failed instead.
> +        """
> +        try:
> +            unsat_deps = []
> +            for or_dep in deps:
> +                if not any(self.relationMatches(dep, avail) for dep in or_dep):
> +                    unsat_deps.append(or_dep)
> +            if unsat_deps:
> +                return PkgRelation.str(unsat_deps)
> +        except Exception as e:
> +            print("Failed to analyse dep-wait: %s" % e)
> +        return None

This'll go to launchpad-buildd's log, not the build log, which is unfortunate given that we're about to stop keeping launchpad-buildd logs. Consider including it in the build log, possibly with a traceback.

> +
>      def iterate_SBUILD(self, success):
>          """Finished the sbuild run."""
>          if success == SBuildExitCodes.OK:


-- 
https://code.launchpad.net/~cjwatson/launchpad-buildd/improve-depwait/+merge/263808
Your team Launchpad code reviewers is subscribed to branch lp:launchpad-buildd.