From c35267fc9ad7822dd54b7882a3b0bbcf03ec14e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Mon, 24 Sep 2018 20:54:59 +0200 Subject: [PATCH 1/6] Add lints that prevent jinja variable misuse. Current conda-build supports accessing package version in meta.yaml via {{ PKG_VERSION }}. Therefore, it is no longer reasonable to define your own jinja variables. For other parts like name, build number and checksum, it was never reasonable because these either don't change during updates (name) nor do they occur more than once. By enforcing non-jinja style here, the recipe becomes much more readable. Moreover, it becomes easier to update programatically. --- bioconda_utils/cli.py | 10 +++ bioconda_utils/lint_functions.py | 64 +++++++++++++++ bioconda_utils/utils.py | 36 +++++++++ test/test_linting.py | 129 +++++++++++++++++++++++++++++++ test/test_utils.py | 61 +++++++++++++++ 5 files changed, 300 insertions(+) diff --git a/bioconda_utils/cli.py b/bioconda_utils/cli.py index 48e4128345..440905a0e6 100644 --- a/bioconda_utils/cli.py +++ b/bioconda_utils/cli.py @@ -632,6 +632,16 @@ def pypi_check(recipe_folder, config, loglevel='info', packages='*', only_out_of print('\t'.join(map(str, result))) +@arg('recipe', nargs="+", help='Path to recipes') +@arg('--loglevel', default='debug', help='Log level') +def bump_build_number(recipes, loglevel="info"): + """ + Bumps the build number in given recipes. + """ + utils.setup_logger('bioconda_utils', loglevel) + utils.bump_build_number(recipes) + + def main(): argh.dispatch_commands([ build, dag, dependent, lint, duplicates, diff --git a/bioconda_utils/lint_functions.py b/bioconda_utils/lint_functions.py index fe973d64c1..f537e3cabd 100644 --- a/bioconda_utils/lint_functions.py +++ b/bioconda_utils/lint_functions.py @@ -3,6 +3,7 @@ import os import re +import yaml import pandas import numpy as np @@ -93,6 +94,65 @@ def lint_metas(recipe, metas, df, *args, **kwargs): return lint_metas +def _superfluous_jinja_var(recipe, entry, required=False, + jinja_var=re.compile(r"{{.*?}}"), + jinja_var_def=re.compile(r"{%.+?%}")): + with open(os.path.join(recipe, 'meta.yaml')) as content: + def quote_var(match, quotes='"\''): + if (content[match.start() - 1] not in quotes and + content[match.end()] not in quotes): + return '"{}"'.format(match.group()) + else: + return match.group() + + # remove var defs to make yaml happy + content = jinja_var_def.sub("", content.read()) + # quote var usages if necessary to make yaml happy + content = jinja_var.sub(quote_var, content) + # load as plain yaml + meta = yaml.load(content, Loader=yaml.SafeLoader) + m = meta + xpath = entry.split('/') + for p in xpath: + try: + m = m[p] + except KeyError: + if required: + return { + 'missing_entry': True, + 'fix': 'add entry {} to meta.yaml'.format(entry) + } + else: + return + + match = jinja_var.search(m) + if match: + return { + 'unwanted_jinja_var': True, + 'fix': 'remove jinja var {} from entry {} in ' + 'meta.yaml'.format(match.group(), entry) + } + + +def jinja_var_name(recipe, meta, df,): + return _superfluous_jinja_var(recipe, 'package/name', required=True) + + +def jinja_var_buildnum(recipe, meta, df,): + return _superfluous_jinja_var(recipe, 'build/number', required=True) + + +def jinja_var_version(recipe, meta, df): + return _superfluous_jinja_var(recipe, 'package/version', required=True) + + +def jinja_var_checksum(recipe, meta, df): + for checksum in ["sha256", "sha1", "md5"]: + ret = _superfluous_jinja_var(recipe, 'source/{}'.format(checksum)) + if ret: + return ret + + @lint_multiple_metas def in_other_channels(recipe, meta, df): """ @@ -405,5 +465,9 @@ def compilers_must_be_in_build(recipe, meta, df): should_not_use_fn, should_use_compilers, compilers_must_be_in_build, + jinja_var_version, + jinja_var_buildnum, + jinja_var_name, + jinja_var_checksum, #bioconductor_37, ) diff --git a/bioconda_utils/utils.py b/bioconda_utils/utils.py index c29df26f88..38a4eb998c 100644 --- a/bioconda_utils/utils.py +++ b/bioconda_utils/utils.py @@ -1082,6 +1082,42 @@ def modified_recipes(git_range, recipe_folder, config_file): return existing +def bump_build_numbers(recipes): + def bump(recipe): + if not recipe.endswith("meta.yaml"): + recipe = os.path.join(recipe, "meta.yaml") + state = 'general' + res = [] + num = None + bumped = None + with open(recipe) as content: + for line in content: + if line.startswith('build'): + state = 'build' + elif state == 'build' and line.lstrip().startswith('number:'): + toks = line.split(':') + if toks == 1: + logger.warning("Failed to parse build number of recipe " + "%s: expected format 'number: value' " + "violated.", recipe) + return + try: + num = int(toks[1].strip()) + except ValueError as e: + logger.warning("Failed to parse build number of " + "recipe %s: %s", recipe, e) + return + bumped = num + 1 + line = '{}: {}\n'.format(toks[0], bumped) + res.append(line) + with open(recipe, "w") as out: + out.writelines(res) + logger.info("Bumped build number of recipe {} from {} to {}".format(recipe, num, bumped)) + + for recipe in recipes: + bump(recipe) + + class Progress: def __init__(self): self.thread = Thread(target=self.progress) diff --git a/test/test_linting.py b/test/test_linting.py index 3e95a9fa65..2206f9850b 100644 --- a/test/test_linting.py +++ b/test/test_linting.py @@ -1019,6 +1019,135 @@ def test_should_not_use_fn(): ) +def test_jinja_var_name(): + run_lint( + func=lint_functions.jinja_var_name, + should_pass=[ + ''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + build: + number: 0 + ''', + ], + should_fail=[ + r''' + a: + meta.yaml: | + {% set name = "a" %} + package: + name: "{{ name|lower }}" + version: 0.1 + build: + number: 0 + ''', + r''' + a: + meta.yaml: | + {% set name = "a" %} + package: + name: {{ name }} + version: 0.1 + build: + number: 0 + ''', + ] + ) + + +def test_jinja_var_version(): + run_lint( + func=lint_functions.jinja_var_version, + should_pass=[ + ''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + build: + number: 0 + ''', + ], + should_fail=[ + r''' + a: + meta.yaml: | + {% set version = "1.0" %} + package: + name: a + version: {{ version }} + build: + number: 0 + ''', + ] + ) + + +def test_jinja_var_buildnum(): + run_lint( + func=lint_functions.jinja_var_buildnum, + should_pass=[ + ''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + build: + number: 0 + ''', + ], + should_fail=[ + r''' + a: + meta.yaml: | + {% set buildnum = 1 %} + package: + name: a + version: 0.1 + build: + number: {{ buildnum }} + ''', + ] + ) + + +def test_jinja_var_checksum(): + run_lint( + func=lint_functions.jinja_var_checksum, + should_pass=[ + ''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + build: + number: 0 + source: + sha256: abc + ''', + ], + should_fail=[ + r''' + a: + meta.yaml: | + {% set sha256 = "abc" %} + package: + name: a + version: 0.1 + build: + number: 0 + source: + sha256: {{ sha256 }} + ''', + ] + ) + #def test_bioconductor_37(): # run_lint( # func=lint_functions.bioconductor_37, diff --git a/test/test_utils.py b/test/test_utils.py index 8680efc1f8..34a63898fd 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -10,6 +10,7 @@ import tarfile import logging import shutil +import filecmp from textwrap import dedent from bioconda_utils import utils @@ -474,6 +475,66 @@ def test_get_channel_packages(): utils.get_channel_packages('bioconda') +def test_bump_build_numbers(): + r = Recipes( + """ + valid: + meta.yaml: | + package: + name: one + version: "0.1" + build: + number: 1 + + invalid_jinja: + meta.yaml: | + package: + name: two + version: "0.1" + build: + number: {{ CONDA_NCURSES }} + + no_build_num: + meta.yaml: | + package: + name: two + version: "0.1" + """, from_string=True) + r.write_recipes() + + expected = Recipes( + """ + valid: + meta.yaml: | + package: + name: one + version: "0.1" + build: + number: 2 + + invalid_jinja: + meta.yaml: | + package: + name: two + version: "0.1" + build: + number: {{ CONDA_NCURSES }} + + no_build_num: + meta.yaml: | + package: + name: two + version: "0.1" + """, from_string=True) + expected.write_recipes() + + utils.bump_build_numbers(r.recipe_dirs.values()) + get_meta = lambda recipe: os.path.join(recipe, "meta.yaml") + for recipe, path in r.recipe_dirs.items(): + assert filecmp.cmp(get_meta(path), + get_meta(expected.recipe_dirs[recipe])) + + def test_built_package_paths(): r = Recipes( """ From a0343d8dac410d4749fde1484820ae86e494765f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Mon, 24 Sep 2018 21:52:31 +0200 Subject: [PATCH 2/6] use baseloader --- bioconda_utils/lint_functions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bioconda_utils/lint_functions.py b/bioconda_utils/lint_functions.py index f537e3cabd..dc587e5a70 100644 --- a/bioconda_utils/lint_functions.py +++ b/bioconda_utils/lint_functions.py @@ -110,7 +110,7 @@ def quote_var(match, quotes='"\''): # quote var usages if necessary to make yaml happy content = jinja_var.sub(quote_var, content) # load as plain yaml - meta = yaml.load(content, Loader=yaml.SafeLoader) + meta = yaml.load(content, Loader=yaml.BaseLoader) m = meta xpath = entry.split('/') for p in xpath: From 6d10c60f560bb354e86c2613206357b957113067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Tue, 25 Sep 2018 09:31:16 +0200 Subject: [PATCH 3/6] documentation --- bioconda_utils/lint_functions.py | 4 ++-- docs/source/linting.rst | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/bioconda_utils/lint_functions.py b/bioconda_utils/lint_functions.py index dc587e5a70..c7979dcde0 100644 --- a/bioconda_utils/lint_functions.py +++ b/bioconda_utils/lint_functions.py @@ -129,8 +129,8 @@ def quote_var(match, quotes='"\''): if match: return { 'unwanted_jinja_var': True, - 'fix': 'remove jinja var {} from entry {} in ' - 'meta.yaml'.format(match.group(), entry) + 'fix': 'replace jinja var {} in entry {} in ' + 'meta.yaml with a concrete value'.format(match.group(), entry) } diff --git a/docs/source/linting.rst b/docs/source/linting.rst index 0bc0fdde05..6cc637526a 100644 --- a/docs/source/linting.rst +++ b/docs/source/linting.rst @@ -312,6 +312,27 @@ Rational: The compiler tools must not be in ``host:`` or ``run:`` sections. How to resolve: Move ``{{ compiler() }}`` variables to the ``build:`` section. +`unwanted_jinja_var` +~~~~~~~~~~~~~~~~~~~~ +Reason for the failing: The recipe uses a jinja variable for version, package name, build number or checksum. + +Rationale: Using a jinja variable for package version is no longer necessary with conda-build 3, because the version can be referenced from all over the ``meta.yaml`` via the jinja expression ``{{ PKG_VERSION }}``. +For the other cases (package name, build number, checksum), using a jinja variable has no benefit, because they are usually only specified at a single place in the ``meta.yaml``. +Therefore, it is better for both human readability and parseability to stick to plain YAML syntax in these cases. + +How to resolve: Directly specify name, version, build number and checksum at the corresponding entry in the ``meta.yaml``. +Use the jinja expression ``{{ PKG_VERSION }}`` to refer to the version in other places of the ``meta.yaml``, e.g., in the ``url``. + + +`missing_entry` +~~~~~~~~~~~~~~~ +Reason for the failing: The recipe is missing a required entry. + +Rationale: The missing entry is crucial for building or updating the package. Affected entries are ``package::name``, ``package::version``, and ``build::number``. + +How to resolve: add the missing entry. + + .. `bioconductor_37` ~~~~~~~~~~~~~~~~~ From d6a6235c049df152afd50ea4bddf6f9fcd196152 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Tue, 25 Sep 2018 12:16:04 +0200 Subject: [PATCH 4/6] lint for missing build number. --- bioconda_utils/lint_functions.py | 26 ++++++++++++--------- docs/source/linting.rst | 39 ++++++++++++++++++++++++++++---- test/test_linting.py | 26 +++++++++++++++++++++ 3 files changed, 75 insertions(+), 16 deletions(-) diff --git a/bioconda_utils/lint_functions.py b/bioconda_utils/lint_functions.py index c7979dcde0..996aa7befe 100644 --- a/bioconda_utils/lint_functions.py +++ b/bioconda_utils/lint_functions.py @@ -94,7 +94,7 @@ def lint_metas(recipe, metas, df, *args, **kwargs): return lint_metas -def _superfluous_jinja_var(recipe, entry, required=False, +def _superfluous_jinja_var(recipe, entry, jinja_var=re.compile(r"{{.*?}}"), jinja_var_def=re.compile(r"{%.+?%}")): with open(os.path.join(recipe, 'meta.yaml')) as content: @@ -117,13 +117,7 @@ def quote_var(match, quotes='"\''): try: m = m[p] except KeyError: - if required: - return { - 'missing_entry': True, - 'fix': 'add entry {} to meta.yaml'.format(entry) - } - else: - return + return match = jinja_var.search(m) if match: @@ -135,15 +129,15 @@ def quote_var(match, quotes='"\''): def jinja_var_name(recipe, meta, df,): - return _superfluous_jinja_var(recipe, 'package/name', required=True) + return _superfluous_jinja_var(recipe, 'package/name') def jinja_var_buildnum(recipe, meta, df,): - return _superfluous_jinja_var(recipe, 'build/number', required=True) + return _superfluous_jinja_var(recipe, 'build/number') def jinja_var_version(recipe, meta, df): - return _superfluous_jinja_var(recipe, 'package/version', required=True) + return _superfluous_jinja_var(recipe, 'package/version') def jinja_var_checksum(recipe, meta, df): @@ -153,6 +147,15 @@ def jinja_var_checksum(recipe, meta, df): return ret +@lint_multiple_metas +def missing_buildnum(recipe, meta, df): + if meta.get_value('build/number') is None: + return { + 'missing_buildnum': True, + 'fix': 'add build->number to meta.yaml' + } + + @lint_multiple_metas def in_other_channels(recipe, meta, df): """ @@ -469,5 +472,6 @@ def compilers_must_be_in_build(recipe, meta, df): jinja_var_buildnum, jinja_var_name, jinja_var_checksum, + missing_buildnum, #bioconductor_37, ) diff --git a/docs/source/linting.rst b/docs/source/linting.rst index 6cc637526a..0ea1562fc1 100644 --- a/docs/source/linting.rst +++ b/docs/source/linting.rst @@ -312,18 +312,47 @@ Rational: The compiler tools must not be in ``host:`` or ``run:`` sections. How to resolve: Move ``{{ compiler() }}`` variables to the ``build:`` section. -`unwanted_jinja_var` +`jinja_var_version` ~~~~~~~~~~~~~~~~~~~~ -Reason for the failing: The recipe uses a jinja variable for version, package name, build number or checksum. +Reason for the failing: The recipe uses a jinja variable for defining the package version Rationale: Using a jinja variable for package version is no longer necessary with conda-build 3, because the version can be referenced from all over the ``meta.yaml`` via the jinja expression ``{{ PKG_VERSION }}``. -For the other cases (package name, build number, checksum), using a jinja variable has no benefit, because they are usually only specified at a single place in the ``meta.yaml``. -Therefore, it is better for both human readability and parseability to stick to plain YAML syntax in these cases. -How to resolve: Directly specify name, version, build number and checksum at the corresponding entry in the ``meta.yaml``. +How to resolve: Directly specify version, in the corresponding entry in the ``meta.yaml``. Use the jinja expression ``{{ PKG_VERSION }}`` to refer to the version in other places of the ``meta.yaml``, e.g., in the ``url``. +`jinja_var_name` +~~~~~~~~~~~~~~~~ + +Reason for the failing: The recipe uses a jinja variable for package name. + +Rationale: For package name, using a jinja variable has no benefit, because it is not supposed to change with updates. +Therefore, it is better for both human readability and parseability to stick to plain YAML syntax. + +How to resolve: Directly specify name in the corresponding entry in the ``meta.yaml``. + +`jinja_var_buildnum` +~~~~~~~~~~~~~~~~~~~~ + +Reason for the failing: The recipe uses a jinja variable for the build number. + +Rationale: For build number, using a jinja variable has no benefit, because, although it changes during updates, it only appears at one place in the ``meta.yaml``. +Therefore, it is better for both human readability and parseability to stick to plain YAML syntax. + +How to resolve: Directly specify build number in the corresponding entry in the ``meta.yaml``. + +`jinja_var_checksum` +~~~~~~~~~~~~~~~~~~~~ + +Reason for the failing: The recipe uses a jinja variable for the source checksum. + +Rationale: For checksum (sha1, sha256, or md5), using a jinja variable has no benefit, because, although it changes during updates, it only appears at one place in the ``meta.yaml``. +Therefore, it is better for both human readability and parseability to stick to plain YAML syntax. + +How to resolve: Directly specify checksum in the corresponding entry in the ``meta.yaml``. + + `missing_entry` ~~~~~~~~~~~~~~~ Reason for the failing: The recipe is missing a required entry. diff --git a/test/test_linting.py b/test/test_linting.py index 2206f9850b..f7fc761a79 100644 --- a/test/test_linting.py +++ b/test/test_linting.py @@ -1148,6 +1148,32 @@ def test_jinja_var_checksum(): ] ) + +def test_missing_buildnum(): + run_lint( + func=lint_functions.missing_buildnum, + should_pass=[ + ''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + build: + number: 0 + ''', + ], + should_fail=[ + r''' + a: + meta.yaml: | + package: + name: a + version: 0.1 + ''', + ] + ) + #def test_bioconductor_37(): # run_lint( # func=lint_functions.bioconductor_37, From 2d05252f1fb928c3517f4d95b2442f3fef51451d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Tue, 25 Sep 2018 13:54:06 +0200 Subject: [PATCH 5/6] Fix missing_buildnum lint. --- bioconda_utils/lint_functions.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/bioconda_utils/lint_functions.py b/bioconda_utils/lint_functions.py index 996aa7befe..f0ea912802 100644 --- a/bioconda_utils/lint_functions.py +++ b/bioconda_utils/lint_functions.py @@ -94,9 +94,12 @@ def lint_metas(recipe, metas, df, *args, **kwargs): return lint_metas -def _superfluous_jinja_var(recipe, entry, - jinja_var=re.compile(r"{{.*?}}"), - jinja_var_def=re.compile(r"{%.+?%}")): +JINJA_VAR_DEF = re.compile(r"{%.+?%}") +JINJA_VAR = re.compile(r"{{.*?}}") + + +def load_plain_yaml(recipe): + """Load meta.yaml without applying any jinja templating.""" with open(os.path.join(recipe, 'meta.yaml')) as content: def quote_var(match, quotes='"\''): if (content[match.start() - 1] not in quotes and @@ -106,11 +109,15 @@ def quote_var(match, quotes='"\''): return match.group() # remove var defs to make yaml happy - content = jinja_var_def.sub("", content.read()) + content = JINJA_VAR_DEF.sub("", content.read()) # quote var usages if necessary to make yaml happy - content = jinja_var.sub(quote_var, content) + content = JINJA_VAR.sub(quote_var, content) # load as plain yaml - meta = yaml.load(content, Loader=yaml.BaseLoader) + return yaml.load(content, Loader=yaml.BaseLoader) + + +def _superfluous_jinja_var(recipe, entry): + meta = load_plain_yaml(recipe) m = meta xpath = entry.split('/') for p in xpath: @@ -119,7 +126,7 @@ def quote_var(match, quotes='"\''): except KeyError: return - match = jinja_var.search(m) + match = JINJA_VAR.search(m) if match: return { 'unwanted_jinja_var': True, @@ -147,9 +154,11 @@ def jinja_var_checksum(recipe, meta, df): return ret -@lint_multiple_metas def missing_buildnum(recipe, meta, df): - if meta.get_value('build/number') is None: + meta = load_plain_yaml(recipe) + try: + meta['build']['number'] + except KeyError: return { 'missing_buildnum': True, 'fix': 'add build->number to meta.yaml' From 91b3c8d7512d6aa6ef892a247bef120b8f539428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20K=C3=B6ster?= Date: Fri, 16 Nov 2018 14:10:13 +0100 Subject: [PATCH 6/6] add missing docs. --- docs/source/linting.rst | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/source/linting.rst b/docs/source/linting.rst index 0ea1562fc1..7a02591edf 100644 --- a/docs/source/linting.rst +++ b/docs/source/linting.rst @@ -353,13 +353,17 @@ Therefore, it is better for both human readability and parseability to stick to How to resolve: Directly specify checksum in the corresponding entry in the ``meta.yaml``. -`missing_entry` +`missing_buildnum` ~~~~~~~~~~~~~~~ -Reason for the failing: The recipe is missing a required entry. +Reason for the failing: The recipe is missing a build number definition. -Rationale: The missing entry is crucial for building or updating the package. Affected entries are ``package::name``, ``package::version``, and ``build::number``. +Rationale: Build number is crucial for building or updating the package. +Although conda would infer it automatically, it is for operational reasons beneficial +always have it explicitly defined in the recipe. -How to resolve: add the missing entry. + + +How to resolve: add ``. ..