diff --git a/ognibuild/buildlog.py b/ognibuild/buildlog.py index 8246d87..b7b5f3e 100644 --- a/ognibuild/buildlog.py +++ b/ognibuild/buildlog.py @@ -193,7 +193,7 @@ class InstallFixer(BuildFixer): req = problem_to_upstream_requirement(error) return req is not None - def fix(self, error, context): + def fix(self, error, phase): reqs = problem_to_upstream_requirement(error) if reqs is None: return False @@ -228,7 +228,7 @@ class ExplainInstallFixer(BuildFixer): req = problem_to_upstream_requirement(error) return req is not None - def fix(self, error, context): + def fix(self, error, phase): reqs = problem_to_upstream_requirement(error) if reqs is None: return False diff --git a/ognibuild/debian/fix_build.py b/ognibuild/debian/fix_build.py index 6dc7eb0..ad71645 100644 --- a/ognibuild/debian/fix_build.py +++ b/ognibuild/debian/fix_build.py @@ -111,7 +111,7 @@ from buildlog_consultant.sbuild import ( ) from ..buildlog import problem_to_upstream_requirement -from ..fix_build import BuildFixer, resolve_error, DependencyContext +from ..fix_build import BuildFixer, resolve_error from ..resolver.apt import ( AptRequirement, get_package_for_python_module, @@ -129,10 +129,39 @@ class CircularDependency(Exception): self.package = package +class DebianPackagingContext(object): + + def __init__(self, tree, subpath, committer, update_changelog): + self.tree = tree + self.subpath = subpath + self.committer = committer + self.update_changelog = update_changelog + + def commit(self, summary: str, update_changelog: Optional[bool] = None) -> bool: + if update_changelog is None: + update_changelog = self.update_changelog + with self.tree.lock_write(): + try: + if update_changelog: + cl_path = self.tree.abspath(os.path.join(self.subpath, "debian/changelog")) + with ChangelogEditor(cl_path) as editor: + editor.add_entry([summary]) + debcommit(self.tree, committer=self.committer, subpath=self.subpath) + else: + self.tree.commit( + message=summary, committer=self.committer, specific_files=[self.subpath] + ) + except PointlessCommit: + return False + else: + return True + + class PackageDependencyFixer(BuildFixer): - def __init__(self, apt_resolver): + def __init__(self, context, apt_resolver): self.apt_resolver = apt_resolver + self.context = context def __repr__(self): return "%s(%r)" % (type(self).__name__, self.apt_resolver) @@ -144,7 +173,7 @@ class PackageDependencyFixer(BuildFixer): req = problem_to_upstream_requirement(error) return req is not None - def fix(self, error, context): + def fix(self, error, phase): reqs = problem_to_upstream_requirement(error) if reqs is None: return False @@ -154,82 +183,33 @@ class PackageDependencyFixer(BuildFixer): changed = False for req in reqs: - package = self.apt_resolver.resolve(req) - if package is None: - return False - if context.phase[0] == "autopkgtest": - return add_test_dependency( - context.tree, - context.phase[1], - package, - committer=context.committer, - subpath=context.subpath, - update_changelog=context.update_changelog, - ) - elif context.phase[0] == "build": - return add_build_dependency( - context.tree, - package, - committer=context.committer, - subpath=context.subpath, - update_changelog=context.update_changelog, - ) - else: - logging.warning('Unknown phase %r', context.phase) + apt_req = self.apt_resolver.resolve(req) + if apt_req is None: return False + if add_dependency(self.context, phase, apt_req): + changed = True return changed -class BuildDependencyContext(DependencyContext): - def __init__( - self, phase, tree, apt, subpath="", committer=None, update_changelog=True - ): - self.phase = phase - super(BuildDependencyContext, self).__init__( - tree, apt, subpath, committer, update_changelog - ) - - def add_dependency(self, requirement: AptRequirement): - return add_build_dependency( - self.tree, - requirement, - committer=self.committer, - subpath=self.subpath, - update_changelog=self.update_changelog, - ) - - -class AutopkgtestDependencyContext(DependencyContext): - def __init__( - self, phase, tree, apt, subpath="", committer=None, update_changelog=True - ): - self.phase = phase - super(AutopkgtestDependencyContext, self).__init__( - tree, apt, subpath, committer, update_changelog - ) - - def add_dependency(self, requirement): +def add_dependency(context, phase, requirement: AptRequirement): + if phase[0] == "autopkgtest": return add_test_dependency( - self.tree, - self.phase[1], + context, + phase[1], requirement, - committer=self.committer, - subpath=self.subpath, - update_changelog=self.update_changelog, ) + elif phase[0] == "build": + return add_build_dependency(context, requirement) + else: + logging.warning('Unknown phase %r', phase) + return False -def add_build_dependency( - tree: Tree, - requirement: AptRequirement, - committer: Optional[str] = None, - subpath: str = "", - update_changelog: bool = True, -): +def add_build_dependency(context, requirement: AptRequirement): if not isinstance(requirement, AptRequirement): raise TypeError(requirement) - control_path = os.path.join(tree.abspath(subpath), "debian/control") + control_path = os.path.join(context.tree.abspath(context.subpath), "debian/control") try: with ControlEditor(path=control_path) as updater: for binary in updater.binaries: @@ -250,27 +230,14 @@ def add_build_dependency( return False logging.info("Adding build dependency: %s", desc) - return commit_debian_changes( - tree, - subpath, - "Add missing build dependency on %s." % desc, - committer=committer, - update_changelog=update_changelog, - ) + return context.commit("Add missing build dependency on %s." % desc) -def add_test_dependency( - tree, - testname, - requirement, - committer=None, - subpath="", - update_changelog=True, -): +def add_test_dependency(context, testname, requirement): if not isinstance(requirement, AptRequirement): raise TypeError(requirement) - tests_control_path = os.path.join(tree.abspath(subpath), "debian/tests/control") + tests_control_path = os.path.join(context.tree.abspath(context.subpath), "debian/tests/control") try: with Deb822Editor(path=tests_control_path) as updater: @@ -296,38 +263,11 @@ def add_test_dependency( desc = requirement.pkg_relation_str() logging.info("Adding dependency to test %s: %s", testname, desc) - return commit_debian_changes( - tree, - subpath, + return context.commit( "Add missing dependency for test %s on %s." % (testname, desc), - update_changelog=update_changelog, ) -def commit_debian_changes( - tree: MutableTree, - subpath: str, - summary: str, - committer: Optional[str] = None, - update_changelog: bool = True, -) -> bool: - with tree.lock_write(): - try: - if update_changelog: - cl_path = tree.abspath(os.path.join(subpath, "debian/changelog")) - with ChangelogEditor(cl_path) as editor: - editor.add_entry([summary]) - debcommit(tree, committer=committer, subpath=subpath) - else: - tree.commit( - message=summary, committer=committer, specific_files=[subpath] - ) - except PointlessCommit: - return False - else: - return True - - def targeted_python_versions(tree: Tree, subpath: str) -> Set[str]: with tree.get_file(os.path.join(subpath, "debian/control")) as f: control = Deb822(f) @@ -345,34 +285,34 @@ def targeted_python_versions(tree: Tree, subpath: str) -> Set[str]: return targeted -def fix_missing_python_distribution(error, context): # noqa: C901 +def fix_missing_python_distribution(error, phase, apt, context): # noqa: C901 targeted = targeted_python_versions(context.tree, context.subpath) default = not targeted - pypy_pkg = context.apt.get_package_for_paths( + pypy_pkg = apt.get_package_for_paths( ["/usr/lib/pypy/dist-packages/%s-.*.egg-info" % error.distribution], regex=True ) if pypy_pkg is None: pypy_pkg = "pypy-%s" % error.distribution - if not context.apt.package_exists(pypy_pkg): + if not apt.package_exists(pypy_pkg): pypy_pkg = None - py2_pkg = context.apt.get_package_for_paths( + py2_pkg = apt.get_package_for_paths( ["/usr/lib/python2\\.[0-9]/dist-packages/%s-.*.egg-info" % error.distribution], regex=True, ) if py2_pkg is None: py2_pkg = "python-%s" % error.distribution - if not context.apt.package_exists(py2_pkg): + if not apt.package_exists(py2_pkg): py2_pkg = None - py3_pkg = context.apt.get_package_for_paths( + py3_pkg = apt.get_package_for_paths( ["/usr/lib/python3/dist-packages/%s-.*.egg-info" % error.distribution], regex=True, ) if py3_pkg is None: py3_pkg = "python3-%s" % error.distribution - if not context.apt.package_exists(py3_pkg): + if not apt.package_exists(py3_pkg): py3_pkg = None extra_build_deps = [] @@ -405,16 +345,13 @@ def fix_missing_python_distribution(error, context): # noqa: C901 for dep_pkg in extra_build_deps: assert dep_pkg is not None - if not context.add_dependency(dep_pkg): + if not add_dependency(context, phase, dep_pkg): return False return True -def fix_missing_python_module(error, context): - if getattr(context, "tree", None) is not None: - targeted = targeted_python_versions(context.tree, context.subpath) - else: - targeted = set() +def fix_missing_python_module(error, phase, apt, context): + targeted = targeted_python_versions(context.tree, context.subpath) default = not targeted if error.minimum_version: @@ -422,9 +359,9 @@ def fix_missing_python_module(error, context): else: specs = [] - pypy_pkg = get_package_for_python_module(context.apt, error.module, "pypy", specs) - py2_pkg = get_package_for_python_module(context.apt, error.module, "cpython2", specs) - py3_pkg = get_package_for_python_module(context.apt, error.module, "cpython3", specs) + pypy_pkg = get_package_for_python_module(apt, error.module, "pypy", specs) + py2_pkg = get_package_for_python_module(apt, error.module, "cpython2", specs) + py3_pkg = get_package_for_python_module(apt, error.module, "cpython3", specs) extra_build_deps = [] if error.python_version == 2: @@ -456,16 +393,16 @@ def fix_missing_python_module(error, context): for dep_pkg in extra_build_deps: assert dep_pkg is not None - if not context.add_dependency(dep_pkg): + if not add_dependency(context, phase, dep_pkg): return False return True -def retry_apt_failure(error, context): +def retry_apt_failure(error, phase, apt, context): return True -def enable_dh_autoreconf(context): +def enable_dh_autoreconf(context, phase): # Debhelper >= 10 depends on dh-autoreconf and enables autoreconf by # default. debhelper_compat_version = get_debhelper_compat_level(context.tree.abspath(".")) @@ -479,28 +416,28 @@ def enable_dh_autoreconf(context): return dh_invoke_add_with(line, b"autoreconf") if update_rules(command_line_cb=add_with_autoreconf): - return context.add_dependency(AptRequirement.simple("dh-autoreconf")) + return add_dependency(context, phase, AptRequirement.simple("dh-autoreconf")) return False -def fix_missing_configure(error, context): +def fix_missing_configure(error, phase, context): if not context.tree.has_filename("configure.ac") and not context.tree.has_filename( "configure.in" ): return False - return enable_dh_autoreconf(context) + return enable_dh_autoreconf(context, phase) -def fix_missing_automake_input(error, context): +def fix_missing_automake_input(error, phase, context): # TODO(jelmer): If it's ./NEWS, ./AUTHORS or ./README that's missing, then # try to set 'export AUTOMAKE = automake --foreign' in debian/rules. # https://salsa.debian.org/jelmer/debian-janitor/issues/88 - return enable_dh_autoreconf(context) + return enable_dh_autoreconf(context, phase) -def fix_missing_config_status_input(error, context): +def fix_missing_config_status_input(error, phase, context): autogen_path = "autogen.sh" rules_path = "debian/rules" if context.subpath not in (".", ""): @@ -519,21 +456,13 @@ def fix_missing_config_status_input(error, context): if not update_rules(makefile_cb=add_autogen, path=rules_path): return False - if context.update_changelog: - commit_debian_changes( - context.tree, - context.subpath, - "Run autogen.sh during build.", - committer=context.committer, - update_changelog=context.update_changelog, - ) - - return True + return context.commit("Run autogen.sh during build.") class PgBuildExtOutOfDateControlFixer(BuildFixer): - def __init__(self, session): + def __init__(self, packaging_context, session): self.session = session + self.context = packaging_context def can_fix(self, problem): return isinstance(problem, NeedPgBuildExtUpdateControl) @@ -547,16 +476,11 @@ class PgBuildExtOutOfDateControlFixer(BuildFixer): shutil.copy( self.session.external_path('debian/control'), context.tree.abspath(os.path.join(context.subpath, 'debian/control'))) - return commit_debian_changes( - context.tree, - context.subpath, - "Run 'pgbuildext updatecontrol'.", - committer=context.committer, - update_changelog=False, - ) + return self.context.commit( + "Run 'pgbuildext updatecontrol'.", update_changelog=False) -def fix_missing_makefile_pl(error, context): +def fix_missing_makefile_pl(error, phase, context): if ( error.filename == "Makefile.PL" and not context.tree.has_filename("Makefile.PL") @@ -568,7 +492,8 @@ def fix_missing_makefile_pl(error, context): class SimpleBuildFixer(BuildFixer): - def __init__(self, problem_cls: Type[Problem], fn): + def __init__(self, packaging_context, problem_cls: Type[Problem], fn): + self.context = packaging_context self._problem_cls = problem_cls self._fn = fn @@ -579,28 +504,46 @@ class SimpleBuildFixer(BuildFixer): def can_fix(self, problem: Problem): return isinstance(problem, self._problem_cls) - def _fix(self, problem: Problem, context): - return self._fn(problem, context) + def _fix(self, problem: Problem, phase): + return self._fn(problem, phase, self.context) -def versioned_package_fixers(session): +class DependencyBuildFixer(BuildFixer): + def __init__(self, packaging_context, apt_resolver, problem_cls: Type[Problem], fn): + self.context = packaging_context + self.apt_resolver = apt_resolver + self._problem_cls = problem_cls + self._fn = fn + + def __repr__(self): + return "%s(%s, %r, %s)" % ( + type(self).__name__, self._problem_cls.__name__, self._fn.__name__) + + def can_fix(self, problem: Problem): + return isinstance(problem, self._problem_cls) + + def _fix(self, problem: Problem, phase): + return self._fn(problem, phase, self.apt_resolver, self.context) + + +def versioned_package_fixers(session, packaging_context): return [ - PgBuildExtOutOfDateControlFixer(session), - SimpleBuildFixer(MissingConfigure, fix_missing_configure), - SimpleBuildFixer(MissingAutomakeInput, fix_missing_automake_input), - SimpleBuildFixer(MissingConfigStatusInput, fix_missing_config_status_input), - SimpleBuildFixer(MissingPerlFile, fix_missing_makefile_pl), + PgBuildExtOutOfDateControlFixer(packaging_context, session), + SimpleBuildFixer(packaging_context, MissingConfigure, fix_missing_configure), + SimpleBuildFixer(packaging_context, MissingAutomakeInput, fix_missing_automake_input), + SimpleBuildFixer(packaging_context, MissingConfigStatusInput, fix_missing_config_status_input), + SimpleBuildFixer(packaging_context, MissingPerlFile, fix_missing_makefile_pl), ] -def apt_fixers(apt) -> List[BuildFixer]: +def apt_fixers(apt, packaging_context) -> List[BuildFixer]: from ..resolver.apt import AptResolver resolver = AptResolver(apt) return [ - SimpleBuildFixer(MissingPythonModule, fix_missing_python_module), - SimpleBuildFixer(MissingPythonDistribution, fix_missing_python_distribution), - SimpleBuildFixer(AptFetchFailure, retry_apt_failure), - PackageDependencyFixer(resolver), + DependencyBuildFixer(packaging_context, apt, MissingPythonModule, fix_missing_python_module), + DependencyBuildFixer(packaging_context, apt, MissingPythonDistribution, fix_missing_python_distribution), + DependencyBuildFixer(packaging_context, apt, AptFetchFailure, retry_apt_failure), + PackageDependencyFixer(packaging_context, resolver), ] @@ -619,7 +562,10 @@ def build_incrementally( update_changelog=True, ): fixed_errors = [] - fixers = versioned_package_fixers(apt.session) + apt_fixers(apt) + packaging_context = DebianPackagingContext( + local_tree, subpath, committer, update_changelog) + fixers = (versioned_package_fixers(apt.session, packaging_context) + + apt_fixers(apt, packaging_context)) logging.info("Using fixers: %r", fixers) while True: try: @@ -647,29 +593,8 @@ def build_incrementally( logging.warning("Last fix did not address the issue. Giving up.") raise reset_tree(local_tree, subpath=subpath) - if e.phase[0] == "build": - context = BuildDependencyContext( - e.phase, - local_tree, - apt, - subpath=subpath, - committer=committer, - update_changelog=update_changelog, - ) - elif e.phase[0] == "autopkgtest": - context = AutopkgtestDependencyContext( - e.phase, - local_tree, - apt, - subpath=subpath, - committer=committer, - update_changelog=update_changelog, - ) - else: - logging.warning("unable to install for context %r", e.phase) - raise try: - if not resolve_error(e.error, context, fixers): + if not resolve_error(e.error, e.phase, fixers): logging.warning("Failed to resolve error %r. Giving up.", e.error) raise except GeneratedFile: diff --git a/ognibuild/fix_build.py b/ognibuild/fix_build.py index 80460f9..6bfc593 100644 --- a/ognibuild/fix_build.py +++ b/ognibuild/fix_build.py @@ -16,8 +16,9 @@ # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA import logging -from typing import List, Optional, Dict +from typing import List, Optional, Dict, Tuple +from buildlog_consultant import Problem from buildlog_consultant.common import ( find_build_failure_description, MissingCommand, @@ -32,35 +33,16 @@ from .session import Session, run_with_tee class BuildFixer(object): """Build fixer.""" - def can_fix(self, problem): + def can_fix(self, problem: Problem): raise NotImplementedError(self.can_fix) - def _fix(self, problem, context): + def _fix(self, problem: Problem, phase: Tuple[str, ...]): raise NotImplementedError(self._fix) - def fix(self, problem, context): + def fix(self, problem: Problem, phase: Tuple[str, ...]): if not self.can_fix(problem): return None - return self._fix(problem, context) - - -class DependencyContext(object): - def __init__( - self, - tree: MutableTree, - apt: AptManager, - subpath: str = "", - committer: Optional[str] = None, - update_changelog: bool = True, - ): - self.tree = tree - self.apt = apt - self.subpath = subpath - self.committer = committer - self.update_changelog = update_changelog - - def add_dependency(self, package) -> bool: - raise NotImplementedError(self.add_dependency) + return self._fix(problem, phase) def run_detecting_problems(session: Session, args: List[str], **kwargs): @@ -104,7 +86,7 @@ def run_with_build_fixers(session: Session, args: List[str], fixers: List[BuildF return -def resolve_error(error, context, fixers): +def resolve_error(error, phase, fixers): relevant_fixers = [] for fixer in fixers: if fixer.can_fix(error): @@ -114,7 +96,7 @@ def resolve_error(error, context, fixers): return False for fixer in relevant_fixers: logging.info("Attempting to use fixer %s to address %r", fixer, error) - made_changes = fixer.fix(error, context) + made_changes = fixer.fix(error, phase) if made_changes: return True return False diff --git a/ognibuild/tests/test_debian_fix_build.py b/ognibuild/tests/test_debian_fix_build.py index a06884a..a947003 100644 --- a/ognibuild/tests/test_debian_fix_build.py +++ b/ognibuild/tests/test_debian_fix_build.py @@ -35,7 +35,7 @@ from ..debian.fix_build import ( resolve_error, versioned_package_fixers, apt_fixers, - BuildDependencyContext, + DebianPackagingContext, ) from breezy.tests import TestCaseWithTransport @@ -97,16 +97,11 @@ blah (0.1) UNRELEASED; urgency=medium session = PlainSession() apt = AptManager(session) apt._searchers = [DummyAptSearcher(self._apt_files)] - context = BuildDependencyContext( - ("build", ), - self.tree, - apt, - subpath="", - committer="ognibuild ", - update_changelog=True, - ) - fixers = versioned_package_fixers(session) + apt_fixers(apt) - return resolve_error(error, context, fixers) + context = DebianPackagingContext( + self.tree, subpath="", committer="ognibuild ", + update_changelog=True) + fixers = versioned_package_fixers(session, context) + apt_fixers(apt, context) + return resolve_error(error, ("build", ), fixers) def get_build_deps(self): with open(self.tree.abspath("debian/control"), "r") as f: