diff --git a/bioconda_utils/bot/commands.py b/bioconda_utils/bot/commands.py index e6ac38b5e2..ce039dbc37 100644 --- a/bioconda_utils/bot/commands.py +++ b/bioconda_utils/bot/commands.py @@ -117,8 +117,18 @@ async def command_bump(ghapi, issue_number, _user, *_args): @command_routes.register("lint") -async def command_lint(ghapi, issue_number, _user, *_args): - """Lint the current recipe""" +async def command_lint(ghapi, issue_number, _user, *args): + """Lint the current recipe + + If the additional argument ``fix`` is provided, an attempt to fix errors + will be scheduled instead of a new check run. This is identical to clicking + the "Fix Issues" button on the checks page. + """ + if args: + if args[0] == 'fix': + pr = await ghapi.get_prs(number=issue_number) + tasks.lint_fix.s(None, pr['head']['sha'], ghapi).apply_async() + return f"Scheduled fixing lints in #{issue_number}" ( tasks.get_latest_pr_commit.s(issue_number, ghapi) | tasks.create_check_run.s(ghapi) diff --git a/bioconda_utils/bot/events.py b/bioconda_utils/bot/events.py index 799d846058..210061d31b 100644 --- a/bioconda_utils/bot/events.py +++ b/bioconda_utils/bot/events.py @@ -109,7 +109,7 @@ async def handle_check_run(event, ghapi): else: logger.error("Unknown requested action in check run: %s", requested_action) else: - logger.error("Unknown action %s", action) + logger.info("Action '%s' unhandled", action) @event_routes.register("pull_request") async def handle_pull_request(event, ghapi): diff --git a/bioconda_utils/bot/tasks.py b/bioconda_utils/bot/tasks.py index 3d506247d4..7c308831cc 100644 --- a/bioconda_utils/bot/tasks.py +++ b/bioconda_utils/bot/tasks.py @@ -92,7 +92,7 @@ async def __aenter__(self): elif self.ref: fork_user = None fork_repo = None - branch_name = "unknown" + branch_name = None ref = self.ref elif self.branch_name: fork_user = None @@ -442,23 +442,23 @@ async def merge_pr(self, pr_number: int, comment_id: int, ghapi) -> Tuple[bool, head_ref = pr['head']['ref'] head_sha = pr['head']['sha'] head_user = pr['head']['repo']['owner']['login'] - # get path for Circle + + # get artifacts from circleci if head_user == ghapi.user: - branch = head_ref + circle_path = head_ref else: - branch = "pull/{}".format(pr_number) + circle_path = "pull/{}".format(pr_number) + capi = AsyncCircleAPI(ghapi.session, token=CIRCLE_TOKEN) + artifacts = await capi.get_artifacts(circle_path, head_sha) lines = [] - - capi = AsyncCircleAPI(ghapi.session, token=CIRCLE_TOKEN) - artifacts = await capi.get_artifacts(branch, head_sha) files = [] images = [] packages = [] - for path, url, buildno in artifacts: + for path, url, _buildno in artifacts: match = PACKAGE_RE.match(url) if match: - repo_url, arch, fname = match.groups() + _repo_url, arch, fname = match.groups() fpath = os.path.join(arch, fname) files.append((url, fpath)) packages.append(fpath) @@ -483,7 +483,7 @@ async def merge_pr(self, pr_number: int, comment_id: int, ghapi) -> Tuple[bool, try: fds = [] urls = [] - for url,path in files: + for url, path in files: fpath = os.path.join(tmpdir, path) fdir = os.path.dirname(fpath) if not os.path.exists(fdir): @@ -569,8 +569,16 @@ async def merge_pr(self, pr_number: int, comment_id: int, ghapi) -> Tuple[bool, if not res: return res, msg - if not branch.startswith('pull/'): - await ghapi.delete_branch(branch) + if head_user == ghapi.user: + # PR head branch is on same account as our install - it's a PR not + # from a fork. Go ahead and try to delete the branch. + if head_ref in ('master', 'bulk'): + # Safeguard - never delete master or bulk! + logger.error("Cowardly refusing to delete master or bulk") + else: + logger.info("Trying to delete branch %s", head_ref) + await ghapi.delete_branch(head_ref) + return res, msg diff --git a/bioconda_utils/githandler.py b/bioconda_utils/githandler.py index 6c3f780a97..39a77e05b5 100644 --- a/bioconda_utils/githandler.py +++ b/bioconda_utils/githandler.py @@ -8,6 +8,7 @@ import tempfile import subprocess from typing import List, Union +import shutil import git import yaml @@ -218,6 +219,14 @@ def get_remote_branch(self, branch_name: str, try_fetch=False): return None for remote_ref in remote_refs: if remote_ref.remote_ref_path == branch_name: + remote_heads = [head for head in self.fork_remote.refs + if head.commit.hexsha == branch_name] + if remote_heads: + if len(remote_heads) > 1: + logger.warning("Found multiple heads for %s - using %s", + branch_name, remote_heads[0]) + logger.warning(" other options: %s", remote_heads[1:]) + return remote_heads[0] return remote_ref.ref def get_latest_master(self): @@ -249,6 +258,9 @@ def create_local_branch(self, branch_name: str, remote_branch: str = None): remote_branch = self.get_remote_branch(remote_branch, try_fetch=False) if remote_branch is None: return None + if branch_name is None and hasattr(remote_branch, 'remote_head'): + branch_name = remote_branch.remote_head + logger.info("Resolved %s to %s", remote_branch, branch_name) self.repo.create_head(branch_name, remote_branch) return self.get_local_branch(branch_name) @@ -532,8 +544,15 @@ class TempGitHandler(GitHandlerBase): might be working in multiple threads and want to avoid blocking waits for a single repo. It also improves robustness: If something goes wrong with this repo, it will not break the entire process. - """ + This class uses a bare clone as a cache to speed up subsequent + instantiations of temporary git clones. This bare "caching" clone + will be created in a temporary directory. To save on download + times when debugging, you can use the environment variable + ``BIOCONDA_REPO_CACHEDIR`` to specify a directory lasting accross + instantiations of the Python interpreter. + + """ _local_mirror_tmpdir: Union[str, tempfile.TemporaryDirectory] = None @classmethod @@ -570,6 +589,7 @@ def _get_local_mirror(cls, url: str) -> git.Repo: # Make location of repo in tmpdir from url _, _, fname = url.rpartition('@') + fname = fname.lstrip('https://') tmpname = getattr(cls._local_mirror_tmpdir, 'name', cls._local_mirror_tmpdir) mirror_name = os.path.join(tmpname, fname) @@ -612,6 +632,8 @@ def __init__(self, fork_user=None, fork_repo=None, dry_run=False) -> None: + self._clean = True + userpass = "" if password is not None and username is None: username = "x-access-token" @@ -645,12 +667,19 @@ def censor(string): logger.info("Finished setting up repo in %s", self.tempdir) super().__init__(repo, dry_run, home_url, fork_url) + def keep_tempdir(self): + """Disable temp dir removal on `close`""" + self._clean = False def close(self) -> None: """Remove temporary clone and cleanup resources""" super().close() - logger.info("Removing repo from %s", self.tempdir.name) - self.tempdir.cleanup() + if self._clean: + logger.info("Removing repo from %s", self.tempdir.name) + self.tempdir.cleanup() + else: + logger.warning("Keeping repo in %s%s", self.tempdir.name, "_saved") + shutil.copytree(self.tempdir.name, self.tempdir.name + "_saved") class BiocondaRepo(GitHandler, BiocondaRepoMixin): @@ -658,3 +687,7 @@ class BiocondaRepo(GitHandler, BiocondaRepoMixin): class TempBiocondaRepo(TempGitHandler, BiocondaRepoMixin): pass + + +# Allow setting a pre-checkout from env +TempGitHandler.set_mirror_dir(os.environ.get('BIOCONDA_REPO_CACHEDIR')) diff --git a/bioconda_utils/githubhandler.py b/bioconda_utils/githubhandler.py index 4caebc3166..b85cce19a6 100644 --- a/bioconda_utils/githubhandler.py +++ b/bioconda_utils/githubhandler.py @@ -359,7 +359,6 @@ async def get_prs_from_sha(self, head_sha: str, only_open=False) -> List[int]: closed=False if only_open else None) for pull in result.get('items', []): pr_number = int(pull['number']) - logger.error("checking %s", pr_number) full_pr = await self.get_prs(number=pr_number) if full_pr['head']['sha'].startswith(head_sha): pr_numbers.append(pr_number) @@ -894,7 +893,7 @@ async def get_contents(self, path: str, ref: str = None) -> str: content = content_bytes.decode('utf-8') return content - async def delete_branch(self, ref: str) -> None: + async def delete_branch(self, ref: str) -> bool: """Delete a branch (ref) Arguments: @@ -902,7 +901,12 @@ async def delete_branch(self, ref: str) -> None: """ var_data = copy(self.var_default) var_data['ref'] = ref - await self.api.delete(self.GIT_REFERENCE, var_data) + try: + await self.api.delete(self.GIT_REFERENCE, var_data) + return True + except gidgethub.InvalidField as exc: + logger.info("Failed to delete branch %s: %s", ref, exc) + return False def _deparse_card_pr_number(self, card: Dict[str, Any]) -> Dict[str, Any]: """Extracts the card's issue's number from the content_url diff --git a/bioconda_utils/lint/check_noarch.py b/bioconda_utils/lint/check_noarch.py index 3dca6d2598..cb3ce6737c 100644 --- a/bioconda_utils/lint/check_noarch.py +++ b/bioconda_utils/lint/check_noarch.py @@ -10,6 +10,10 @@ from . import LintCheck, ERROR, WARNING, INFO +from logging import getLogger + +logger = getLogger(__name__) + # Noarch or not checks: # @@ -22,6 +26,7 @@ # b) Not use ``- python [<>]3``, # but use ``skip: True # [py[23]k]`` + class should_be_noarch_python(LintCheck): """The recipe should be build as ``noarch`` @@ -36,17 +41,14 @@ class should_be_noarch_python(LintCheck): """ def check_deps(self, deps): - if 'python' not in deps: - return # not a python package - if all('build' not in loc for loc in deps['python']): - return # only uses python in run/host - if any(dep.startswith('compiler_') for dep in deps): - return # not compiled - if self.recipe.get('build/noarch', None) == 'python': - return # already marked noarch: python - self.message(section='build', data=True) + if not self.recipe.is_noarch(python=True) and \ + self.recipe.has_dep('python', section='host') and \ + not self.recipe.has_compiler() and \ + not self.recipe.has_selector(): + self.message(section='build', data=True) def fix(self, _message, _data): + logger.warning("Lint fix: setting build/noarch=python") self.recipe.set('build/noarch', 'python') return True @@ -66,13 +68,16 @@ class should_be_noarch_generic(LintCheck): """ requires = ['should_be_noarch_python'] def check_deps(self, deps): - if any(dep.startswith('compiler_') for dep in deps): - return # not compiled - if self.recipe.get('build/noarch', None): - return # already marked noarch - self.message(section='build', data=True) + if not self.recipe.is_noarch(python=False) and \ + not self.recipe.has_dep('python', section='host') and \ + not self.recipe.has_compiler() and \ + not self.recipe.has_selector(): + logger.error("here") + logger.error(self.recipe.has_selector()) + self.message(section='build', data=True) def fix(self, _message, _data): + logger.warning("Lint fix: setting build/noarch=generic") self.recipe.set('build/noarch', 'generic') return True @@ -86,11 +91,8 @@ class should_not_be_noarch_compiler(LintCheck): """ def check_deps(self, deps): - if not any(dep.startswith('compiler_') for dep in deps): - return # not compiled - if self.recipe.get('build/noarch', False) is False: - return # no noarch, or noarch=False - self.message(section='build/noarch') + if self.recipe.is_noarch() and self.recipe.has_compiler(): + self.message(section='build/noarch') class should_not_be_noarch_skip(LintCheck): @@ -100,11 +102,23 @@ class should_not_be_noarch_skip(LintCheck): """ def check_recipe(self, recipe): - if self.recipe.get('build/noarch', False) is False: - return # no noarch, or noarch=False - if self.recipe.get('build/skip', False) is False: - return # no skip or skip=False - self.message(section='build/noarch') + if self.recipe.is_noarch() and \ + self.recipe.get('build/skip', False) is not False: + self.message(section='build/noarch') + + +class should_not_be_noarch_selector(LintCheck): + """The recipe uses ``# [cond]`` but is marked noarch + + Recipes using conditional lines cannot be noarch. + + """ + requires = ['should_use_compilers', + 'should_not_be_noarch_skip', + 'should_not_be_noarch_source'] + def check_recipe(self, recipe): + if self.recipe.is_noarch() and self.recipe.has_selector(): + self.message(section='build/noarch') class should_not_use_skip_python(LintCheck): @@ -127,16 +141,12 @@ class should_not_use_skip_python(LintCheck): bad_skip_terms = ('py2k', 'py3k', 'python') def check_deps(self, deps): - if 'python' not in deps: - return # not a python package - if any(dep.startswith('compiler_') for dep in deps): - return # not compiled - if self.recipe.get('build/skip', None) is None: - return # no build: skip: section - skip_line = self.recipe.get_raw('build/skip') - if not any(term in skip_line for term in self.bad_skip_terms): - return # no offending skip terms - self.message(section='build/skip') + if self.recipe.has_dep('python') and \ + not self.recipe.has_compiler() and \ + self.recipe.get('build/skip', None) is not None and \ + any(term in self.bad_skip_terms + for term in self.recipe.get_raw('build/skip')): + self.message(section='build/skip') class should_not_be_noarch_source(LintCheck): @@ -146,9 +156,6 @@ class should_not_be_noarch_source(LintCheck): platform. Remove the noarch section or use just one source for all platforms. """ - - _pat = re.compile(r'# +\[.*\]') def check_source(self, source, section): - # just search the entire source entry for a comment - if self._pat.search(self.recipe.get_raw(f"{section}")): - self.message(section) + if self.recipe.is_noarch() and self.recipe.has_selector(section): + self.message(section) diff --git a/bioconda_utils/recipe.py b/bioconda_utils/recipe.py index da1596548a..bed1a85e10 100644 --- a/bioconda_utils/recipe.py +++ b/bioconda_utils/recipe.py @@ -146,6 +146,7 @@ class Recipe(): "cdt": lambda x: x } + _selector_re = re.compile(r'# +\[.*\]') def __init__(self, recipe_dir, recipe_folder): if not recipe_dir.startswith(recipe_folder): @@ -415,7 +416,7 @@ def get_raw_range(self, path): a tuple of first_row, first_column, last_row, last_column """ if not path: - return 0, 0, len(self.meta_yaml), len(self.meta_yaml[-1]) + return 0, 0, len(self.meta_yaml)-1, len(self.meta_yaml[-1]) nodes, keys = self._walk(path) nodes.pop() # pop parsed value @@ -524,10 +525,10 @@ def set(self, path, value): _, col, row, _ = self.get_raw_range(found_path) backup = deepcopy(self.meta_yaml) for key in path.split('/')[len(keys):]: - self.meta_yaml.insert(row, ' ' * col + key + ':') + self.meta_yaml.insert(row + 1, ' ' * col + key + ':') row += 1 col += 2 - self.meta_yaml[row-1] += " marker" + self.meta_yaml[row] += " None" self.render() # get old content @@ -751,6 +752,48 @@ def conda_release(self): self._conda_tempdir.cleanup() self._conda_tempdir = None + def has_selector(self, section: str = '') -> bool: + """Checks if the recipe uses ``# [cond]`` line selectors + + Args: + section: Limit check to section + + """ + if self._selector_re.search(self.get_raw(section)): + return True + return False + + def has_compiler(self) -> bool: + """Checks if the recipe uses a compiler""" + if any(dep.startswith('compiler_') for dep in self.get_deps()): + return True + return False + + def is_noarch(self, python: bool = None) -> bool: + """Checks if the recipe is marked noarch + + Args: + python: If true, checks for noarch python. + If false, checks for noarch not python. + """ + noarch = self.get('build/noarch', False) + if python: + return noarch == 'python' + if python is False: + return noarch in (True, 'generic') + return noarch in (True, 'generic', 'python') + + def has_dep(self, dep: str = None, section=None) -> bool: + """Checks if the recipe requires **dep** + + Args: + dep: The dependency to check for or None + section: The section(s) the dep must occur in + """ + if dep: + return dep in self.get_deps_dict(section) + return bool(self.get_deps_dict(section)) + def load_parallel_iter(recipe_folder, packages): recipes = list(utils.get_recipes(recipe_folder, packages)) diff --git a/test/conftest.py b/test/conftest.py index 8e51b80e13..17844f55f5 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -97,14 +97,21 @@ def recipes_folder(tmpdir: py.path.local): def dict_merge(base, add): + """Merge **add** into **base** + + Takes care to keep ruamel.yaml CommentedMap comments intact. + """ for key, value in add.items(): if isinstance(value, dict): - base[key] = dict_merge(base.get(key, {}), value) + base[key] = dict_merge(base.get(key, type(base)()), value) elif isinstance(base, list): for num in range(len(base)): - base[num][key] = dict_merge(base[num].get(key, {}), add) + base[num][key] = dict_merge(base[num].get(key, type(base[num])()), add) else: base[key] = value + if hasattr(add, 'copy_attributes') and \ + hasattr(base, 'copy_attributes'): + add.copy_attributes(base) return base diff --git a/test/lint_cases.yaml b/test/lint_cases.yaml index 6c33a933cf..a6d21c71d0 100644 --- a/test/lint_cases.yaml +++ b/test/lint_cases.yaml @@ -191,10 +191,10 @@ tests: expect: has_windows_bat_file # FIXME: should_be_noarch without python - name: should_be_noarch_python - add: { requirements: { build: [python], run: [python] } } + add: { requirements: { host: [python], run: [python] } } expect: should_be_noarch_python - name: should_be_noarch_python_good - add: { build: { noarch: python }, requirements: { build: [python], run: [python] } } + add: { build: { noarch: python }, requirements: { host: [python], run: [python] } } - name: should_be_noarch_python_good_skip add: build: { noarch: python, skip: false } @@ -204,12 +204,18 @@ tests: build: { noarch: python } requirements: { build: [compiler_gcc]} expect: should_not_be_noarch_compiler -- name: should_not_be_noarch_skip +- name: should_not_be_noarch_python_skip add: build: noarch: python skip: True # [osx] expect: should_not_be_noarch_skip +- name: should_not_be_noarch_generic_skip + add: + build: + noarch: generic + skip: True # [osx] + expect: should_not_be_noarch_skip - name: should_not_be_noarch_source expect: should_not_be_noarch_source remove: source @@ -219,6 +225,16 @@ tests: sha256: 123 - url: https://elsewhere # [osx] sha256: 123 +- name: should_not_be_noarch_source_good + remove: source + add: + build: + noarch: False + source: + - url: https://somewhere # [linux] + sha256: 123 + - url: https://elsewhere # [osx] + sha256: 123 - name: noarch_java_with_python_wrapper add: build: { noarch: generic }