-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add lints that prevent jinja variable misuse. Add subcommand for bumping build number. #339
base: master
Are you sure you want to change the base?
Conversation
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/core, please have a look at this, opinions welcome. In particular regarding part one. |
An example recipe that implements the changed strategy from part one can be found here: bioconda/bioconda-recipes#11042 |
Thanks @johanneskoester, this is a good idea. Will take some effort to retrofit existing recipes, but using jinja vars at the top for version/name/hash never sat well with me, and you describe exactly why in the lint docs. |
Indeed, I am very happy as well that we finally can get rid of them. I suggest merging ASAP, and then fix old recipes when we anyway have to rebuild them (e.g., because of the compiler update on conda-forge). |
I have the feeling that this diverges us more from the rest of the conda universe. I do think its a good change but we should talk to conda-forge and push it their in parallel and even more important I think we should push for skeleton changes. Currently all the skeleton generators do generate the opposite, isn't it? It would be a bad UX if people using skeletons and need to change a lot of stuff to make our linter happy. Thanks @johanneskoester for working on this, I do like it - I just think we should discuss this at a different level and not go for a bioconda only solution. |
Yes I agree @bgruening. Might be that conda-forge does not want to change this quickly, because I think they have a lot of tooling that relies on those jinja variables. Nevertheless it is worth a try. Apart from that, I think we can solve the issue with the skeletons by providing a PR that adds an option to generate a jinja-free recipe from every skeleton generator. This way, we only have to tell people, e.g., use |
I would take care of that PR if you agree that this would be a feasible approach to proceed fast. |
That would be fantastic!!! Thanks Johannes! |
Do you want to approach conda-forge about this, or shall we ask our connection officer @mbargull? :-) |
I can do this if you like. But maybe waiting for the PRs? |
I'm generally in favor of reducing the boilerplate that is "dynamically composing static elements" in the recipes. I never understood why one would want to transform a concise {% set version = "1.0.0" %}
{% set sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" %}
package:
name: package
version: {{ version }}
source:
url: https://pypi.io/packages/source/p/package/package-{{ version }}.tar.gz
sha256: {{ sha256 }} into {% set name = "package" %}
{% set version = "1.0.0" %}
{% set file_ext = "tar.gz" %}
{% set hash_type = "sha256" %}
{% set hash_val = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" %}
package:
name: {{ name|lower }}
version: {{ version }}
source:
fn: {{ name }}-{{ version }}.{{ file_ext }}
url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.{{ file_ext }}
{{ hash_type }}: {{ hash_val }} in their recipe. I mean, isn't url: https://pypi.io/packages/source/{{ name[0] }}/{{ name }}/{{ name }}-{{ version }}.{{ file_ext }} too inconsistent? Far too much static tokens in there! Why not write it as: {% set url_prefix = "https://pypi.io/packages/source" %}
{% set fn = "{}-{}.{}".format(name, version, file_ext) %}
{% set url = [url_prefix, name[0], name, fn]|join("/") %}
...
url: {{ url }} ? 😈 But seriously, that overuse of Jinja templates for static content always makes me shudder. That being said, for the usual dynamic parts, i.e., {% set version = "1.0.0" %}
{% set sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" %} which means adding unneeded boilerplate for But if we can use Re Now, setting that "personal opinion babbling" above aside, and for the important part: |
I strongly prefer the jinja2 style with these values at the top. Primrarily because it makes diff'ing and merging so much easier if these variables are in one block at the top and the rest of the recipe doesn't change. |
Part one
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.
Part two
This PR adds a subcommand
bioconda-utils bump-build-number
, that takes a list of recipes and increases their build number by one. This helps us to rebuild stuff when the version pinnings change. Ideally, I would like to automate this, e.g. by finding out which pinning changed, and scanning through all packages with a corresponding dependency. However, this is not done yet.