add v2 recipe editing capabilities for bumping version & build number #2920
Conversation
|
@beckermr - I ported all recipe editing here. It's a bit involved because the tool does the following:
lmk what you think! :) |
|
Yep. If you look at the existing code it detects a bunch of errors and outputs an error message etc. we need to match that api. |
|
Also the hashing bits should use the util in the repo. We've had issues with hashing stalling for some URLs and killing the bot. I'll leave more comments later. Sorry for the terse comments. |
|
@beckermr - I just tackled the API of |
| data = _load_yaml(file) | ||
| build_number_modified = _update_build_number_in_context(data, new_build_number) | ||
| if not build_number_modified: | ||
| _update_build_number_in_recipe(data, new_build_number) | ||
|
|
||
| return _dump_yaml_to_str(data) |
There was a problem hiding this comment.
People can and have mixed setting build numbers in both the jinja2 vars / context and explicitly in the recipe. I know it is awful. We should update the code to do both and check if the build number in the non-context part is templated or not before updating.
| ) -> dict[str, str]: | ||
| """ | ||
| Load all string values from the context dictionary as Jinja2 templates. | ||
| Use linux-64 as default target_platform, build_platform, and mpi. |
There was a problem hiding this comment.
I'm concerned that assuming linux as the default will result in invalid updates for some recipes.
There was a problem hiding this comment.
I understand where you are coming from but we do ignore the selectors in the version updating code (e.g. we go down both branches, then and else.
However, we do use the value for templating, so people could theoretically do something like
source:
url: https://foo.${{ "linux" if linux else "bla" }}/...
sha256: ${{ "foo" ...But that doesn't seem very likely and also buggy so I don't think we'll see that a lot in the wild.
There was a problem hiding this comment.
Are folks allowed to use if..then in the context section to change the values of variables?
There was a problem hiding this comment.
I believe that doesn't work in the context section. However you can use templating and inline Jinja expressions, such as:
context:
version: "1.2.3"
version_string: "v${{ version if linux else "foo" }}"There was a problem hiding this comment.
right so we need to make this correct.
There was a problem hiding this comment.
That won't change other items that might be platform dependent in the context section.
There was a problem hiding this comment.
But the SHA hash will make it impossible, thankfully :)
There was a problem hiding this comment.
Nope. Folks could define parts of urls in the context section that are platform dependent that are not hashes. So when the bot comes through to render those urls and parts, it will compute the wrong hash since it might have a valid, but incorrect url for that platform.
There was a problem hiding this comment.
@beckermr - i sorta get where you come from but is it really necessary to care about all of these edge cases on day one?
It is also not a very clever way to write the recipe. You would have to "double-if" the source.
E.g.
context:
foo: ${{ "bla" if linux else "foo" }}
source:
- if: linux
then:
url: ${{ foo }}
sha256: 1abcd
else:
url: ${{ foo }}
sha256: 2bcdeSo I don't think many people would wanna write their sources like this (doesn't make much logical sense).
There was a problem hiding this comment.
Yes we should care. My suggested procedure for this work was to abstract the current version update algorithm to be independent of the recipe format backend and then use that. It should handle most, if not all, of the cases I am bringing up.
| env = SandboxedEnvironment( | ||
| variable_start_string="${{", | ||
| variable_end_string="}}", | ||
| trim_blocks=True, | ||
| lstrip_blocks=True, | ||
| autoescape=jinja2.select_autoescape(default_for_string=False), | ||
| undefined=_MissingUndefined, | ||
| ) |
There was a problem hiding this comment.
Can you add a comment on these options, why they are set, and how that relates to the supported features for the v2 recipe?
Also, will this parser fail on if conditions, for loops, or other non-supported jinja2 filters / syntax?
There was a problem hiding this comment.
I don't think it will fail. rattler-build will later fail if things aren't valid YAML though.
There was a problem hiding this comment.
Is there a way we can make it fail and output an error?
| ) -> dict[str, str]: | ||
| """ | ||
| Load all string values from the context dictionary as Jinja2 templates. | ||
| Use linux-64 as default target_platform, build_platform, and mpi. |
There was a problem hiding this comment.
Again I am concerned about the defaults here for platform etc.
| return re.search(pattern, url) is not None | ||
|
|
||
|
|
||
| def update_hash(source: Source, url: str, hash_: Hash | None) -> None: |
There was a problem hiding this comment.
Some people will set the hash in the context section. yay edge cases!
There was a problem hiding this comment.
Yeah it's pretty discouraged by the schema right now and I don't see the benefit for the users to set the hash in the context. We just inject it in the source and the old hash will persist in the context for now.
If someone gets annoyed we can modify this.
There was a problem hiding this comment.
This could break some recipes and people will be even more annoyed by that. We need to fix it.
There was a problem hiding this comment.
I made a lint to prevent this in conda-smithy: conda-forge/conda-smithy#2028
Still needs tests etc. but I think that's the better way forward.
There was a problem hiding this comment.
@beckermr how would it break a recipe though. You would have:
context:
sha256: foo
source:
url: bla
sha256: "1234..."
At some point someone would come and clean up the left-over hash from the context.
It would only break if the SHA hash was part of the URL. But in that case, the autotick updater has basically no chance anyways :)
| class HashError(Exception): | ||
| def __init__(self, url: str) -> None: | ||
| self.url = url | ||
| self.message = f"Could not hash {url}" |
There was a problem hiding this comment.
It is useful to add the templated string that was rendered and what version in that string was tried to this error so that folks can diagnose issues.
| template = env.from_string(url) | ||
| rendered_url = template.render(context_variables) |
There was a problem hiding this comment.
The bot tries a series of url template modifications in order to get version updates to work. People do things like add / remove v, there are multiple pypi urls etc. We need to use those here.
There was a problem hiding this comment.
Yeah, that would all be done in the context or in the rendering process. There is no functional difference here.
There was a problem hiding this comment.
Wait, you mean "people" as in upstream maintainers doing that?
There was a problem hiding this comment.
I mean upstream changes the urls and the bot tries to account for it. See here: https://github.com/regro/cf-scripts/blob/master/conda_forge_tick/url_transforms.py
There was a problem hiding this comment.
I guess I am slightly confused because I would have expected the version ticker thingy to already find these if necessary - and this to be present in the new_version?
There was a problem hiding this comment.
Which also lets me wonder if we need to adjust the code somewhere else where the different versions are being tried out :)
There was a problem hiding this comment.
The version ticker thing does this too, but it does not record which url variant worked. Hence it is done here again. It is possible that the recipe can change between when the version is found and when we update the recipe as well.
There was a problem hiding this comment.
I left a bunch of comments. One more is that the tests are not complete in that they don't touch a lot of the edge cases.
This PR is getting pretty big, so my suggestion is that we split to three or four PRs: 1) add the parser + tests for it, 2) the build number bump, 3) the version change + basic tests, and 4) version tests for edge cases.
|
Thanks for the comments @beckermr - I'll try to get to it asap. |
|
No rush! |
|
I wanted to quickly type out what a v2 recipe looks like: context:
version: "0.9.6"
package:
name: bottom
version: ${{ version }}
source:
url: https://github.com/ClementTsang/bottom/archive/refs/tags/${{ version }}.tar.gz
sha256: 202130e0d7c362d0d0cf211f6a13e31be3a02f13f998f88571e59a7735d60667
build:
script:
- if: unix
then:
- cargo install --locked --root $PREFIX --path . --no-track
else:
- cargo install --locked --root %PREFIX% --path . --no-track
requirements:
build:
- ${{ compiler("rust") }}
tests:
- script:
- btm --help Now if you had multiple sources, or conditional sources, you are still forced to spell them out. E.g. source:
- if: linux
then:
url: https://github.com/ClementTsang/bottom/archive/refs/tags/${{ version }}-linux.tar.gz
sha256: 202130e0d7c362d0d0cf211f6a13e31be3a02f13f998f88571e59a7735d60667
- if: win
then:
url: https://github.com/ClementTsang/bottom/archive/refs/tags/${{ version }}-win.tar.gz
sha256: 8a86c4eecf12446ff273afc03e1b3a09a911d0b7981db1af58cb45c439161295
- if: osx
then:
url: https://github.com/ClementTsang/bottom/archive/refs/tags/${{ version }}-osx.tar.gz
sha256: 688787d8ff144c502c7f5cffaafe2cc588d86079f9de88304c26b0cb99ce91c6I looked at the code for the selector evaluation and it makes a lot of sense for meta.yamls'. But for the new recipe format it doesn't really make sense. The use case doesn't really exist because people wouldn't write it that way. You wouldn't even really use any platform specific jinja variables in the URLs etc. since you can just insert the proper URL (just include the version). So IMO we can ignore this for the new recipes. I also do not (really not) see a good use case for templating the SHA hash. Can someone come up with a good use case for templating that hash (for the new recipe format)? Maybe I am blind! :) |
|
Regarding the version variations - that makes sense. We only support a subset of what is being tried in the current munger but that should be fine. I'll try to get to it. There is also an item about the R-version needing some slight adjustment that we should handle. |
|
Yeah it's not really about what you think is a good or bad use of the format. If it is allowed, it will happen and the bot will have to deal with it. :/ |
Hi @beckermr - this is the initial PR to port over the functionality for the recipe editing for v2 recipes.
I'll add some tests now.