Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

johanneskoester
Copy link
Contributor

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.

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.
@johanneskoester
Copy link
Contributor Author

@bioconda/core, please have a look at this, opinions welcome. In particular regarding part one.

@johanneskoester
Copy link
Contributor Author

An example recipe that implements the changed strategy from part one can be found here: bioconda/bioconda-recipes#11042

@daler
Copy link
Member

daler commented Sep 25, 2018

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.

@johanneskoester
Copy link
Contributor Author

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).

@bgruening
Copy link
Member

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.

@johanneskoester
Copy link
Contributor Author

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 conda skeleton pypi --profile plain-yaml somepackage.

@johanneskoester
Copy link
Contributor Author

I would take care of that PR if you agree that this would be a feasible approach to proceed fast.

@bgruening
Copy link
Member

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!

@johanneskoester
Copy link
Contributor Author

johanneskoester commented Sep 25, 2018

Do you want to approach conda-forge about this, or shall we ask our connection officer @mbargull? :-)

@bgruening
Copy link
Member

I can do this if you like. But maybe waiting for the PRs?

@mbargull
Copy link
Member

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., version and sha256, I even like writing

{% set version = "1.0.0" %}
{% set sha256 = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" %}

which means adding unneeded boilerplate for sha256. But that way you get small nice diffs on updates 😉.

But if we can use PKG_VERSION (which is a thing I haven't ever thought of before! Nice!) to avoid {% set version = "..." %}, I'd be willing to trade those concise diffs for general better parsability (by human or machine). conda-build's meta.yaml files are surprisingly complicated to process, so reducing the complexity there would be nice.


Re conda-forge:
I agree it's beneficial to try to speak the same "language" on bioconda-recipes, conda-forge, and AnacondaRecipes and not create very divergent "dialects". It surely makes life easier for people that contribute to multiple of those instances!
Nonetheless, if we can speak a dialect that is not that different, but is also clearer/easier (e.g., for new contributors), and as expressive (!) (and still a good fit for automatic processing) as the other ones, then I think that's fine for us to do. Best case is of course that we all (Bioconda, conda-forge, Anaconda) can agree to switch to the clearer variety anyway 😄.


Now, setting that "personal opinion babbling" above aside, and for the important part:
Having changes to the skeletons would be nice, yes.
But much more crucial is to make sure that using {{ PKG_VERSION }} is a well supported concept in conda-build! It's not something I have seen in the Conda build ecosystem before and is not covered by any tests in conda-build's test suite.
So, IMO, a hard requirement for us to adopt this scheme, is to contribute test cases/recipes to conda-build. Otherwise it may break in the future and might be considered as not "officially supported" (see, e.g., conda/conda-build#2684 (comment) or conda/conda-build#1379 (comment)). Hence, I think the first person outside of Bioconda we have to discuss this with is @msarahan!

@mingwandroid
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants