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.

Sign up for GitHub

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

Jump to bottom

add source linting for recipe v2 #2028

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

add source linting for recipe v2 #2028

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a88c2ed
add source linting for recipe v2
wolfv Aug 8, 2024
446e580
add end-to-end-tests
wolfv Aug 9, 2024
8472902
remove some print statements
wolfv Aug 9, 2024
b7b0f10
add test for v2 name, version and context jinja
wolfv Aug 9, 2024
601e1a1
remove debugging prints
wolfv Aug 9, 2024
f1b2ef2
remove lint & test for now
wolfv Aug 9, 2024
f7d0975
fix invocation
wolfv Aug 9, 2024
202e7e5
add lint for sha256 / md5 templating for the new recipe format
wolfv Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion conda_smithy/lint_recipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,10 @@ def lintify_meta_yaml(
lint_build_section_should_be_before_run(requirements_section, lints)

# 9: Files downloaded should have a hash.
lint_sources_should_have_hash(sources_section, lints)
if recipe_version == 1:
lint_sources_should_have_hash(sources_section, lints)
elif recipe_version == 2:
conda_recipe_v2_linter.lint_sources(sources_section, lints)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to have two functions. Please use one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to lint v1 recipes to make sure that the SHA hash is not templated as well @isuruf?
I think that would be a good change, but some churn involved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pass recipe_version to lint_sources_should_have_hash and make the distinction there. Btw, how do you write a recipe like https://github.com/conda-forge/cuda-nvcc-impl-feedstock/blob/main/recipe/meta.yaml#L24-L28?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write it like this (valid yaml!):

context:
  version: "12.6.20"

source:
  - if: target_platform == "linux-64:
    then:
      url: https://developer.download.nvidia.com/compute/cuda/redist/cuda_nvcc/linux-x86_64/cuda_nvcc-linux-x86_64-{{ version }}-archive.tar.xz
      sha256: 4336a230269db14336ca1f727a0502631390a52d37c17d3bde05ccda5d534406
      patches:
        - nvcc.profile.patch
  - if: target_platform == "linux-aarch64"
    then:
      url: https://developer.download.nvidia.com/compute/cuda/redist/cuda_nvcc/linux-sbsa/cuda_nvcc-linux-sbsa-{{ version }}-archive.tar.xz
      sha256: 06112bf1cc0cfb6b881a2e0420cb4611b6d701decbb195fbfb4f0e9966a42006
      patches:
        - nvcc.profile.patch
  - if: target_platform == "win-64"
    then:
      url: https://developer.download.nvidia.com/compute/cuda/redist/cuda_nvcc/x64/cuda_nvcc-x64-{{ version }}-archive.zip
      sha256: 03282a36fb98c2f227877a62efd04c36b1ab01fb1b676d9edbd0b10179e0a40a
      patches:
        - nvcc.profile.patch.win

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

biggest downside is the patch duplication for the two sources although I think that's ... OK :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you agree that is an abomination, we should figure out a non-abomination. That's the whole point of v2 IMO. Make recipes not be abominations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for context, the lint suggestion arose here

regro/cf-scripts#2920 (comment)

where I commented that you'd need to update the hash in the context section too. The suggestion was that by eliminating the use of variables to specify the hash, we could simplify the code in the bot. That entire block of code is 24 lines in the bot (https://github.com/regro/cf-scripts/blob/master/conda_forge_tick/update_recipe/version.py#L173-L197).

Then we had another discussion here

https://github.com/regro/cf-scripts/pull/2920/files/b7f61366a355ab41e0728e7cc325ed36596c3d67#discussion_r1709740255

where I pointed out that we have to evaluate the context section of the recipe under the assumptions of not just linux, but also other platforms in order to update things correctly. You suggested that this lint would help and we both agreed there would be other edge cases this would miss.

So we can have a separate discussion on this change on its own, but the context for it came from simplifying bot code by eliminating flexibility in the v2 recipe spec.

This is the motivation for my comments. This PR is an incomplete solution to solve complexity in the v2 recipe spec. We either need to reconsider the spec to eliminate complexity or we need to deal with it. Excluding the a small part of the complexity only in conda-forge is an incomplete solution. Given that many conda-forge devs helped make the spec, it's rather odd that we wouldn't use all of it.

As for the bot and the implementation, I don't see the point in avoiding the work to support the new spec in the same way. Otherwise, we do something that covers only part of the cases, people find bugs + submit bug reports, and then someone (you, me, someone else?) in the future is left either saying this won't ever happen or fixing it.

I suggested a better path forward for this work in multiple places related to eliminating technical debt around recipe parsing+editing (which is one of the original motivations for the new spec anyways) and then using that to move things ahead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think my motivation was never to make it less verbose to write complex recipes :)

My motivation was to make recipes structured, easy to parse and fast to evaluate (ie. not do the recursive craziness of conda-build).

I do think we could find solutions to this problem, but for me it's low priority as we can continue using meta.yaml, so if a user doesn't like the new format / additional verbosity there is no need to switch. From my POV it's also very few recipes that are structured this way (mostly binary repacks). And I also don't mind the more extended form, personally.

But if you have good ideas for improvements to the spec, I would love to hear them!

Thanks for adding the context, Matt. I am happy to fix bugs/issues as they come up related to the new format, of course!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're talking past one another a bit here, but this statement is a red flag:

I do think we could find solutions to this problem, but for me it's low priority as we can continue using meta.yaml, so if a user doesn't like the new format / additional verbosity there is no need to switch. From my POV it's also very few recipes that are structured this way (mostly binary repacks). And I also don't mind the more extended form, personally.

The goal here, I thought, is to move to a new spec and eliminate a previous one, all for tangible benefits (performance, interpretability, maintainability, etc.). If we are not in this process to actually deprecate and then remove the v1 spec, why on earth would we do all of this work just to support both in the end?

Thanks for adding the context, Matt. I am happy to fix bugs/issues as they come up related to the new format, of course!

Great and thank you! I have raised several issues around edge cases in the PRs we have going for v2 and my opinion is that now is a good time to address them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the outcome is that v2 replaces v1 I will be personally very happy. But I also don't want to force people on v2, especially not during the initial period. The whole transition will probably take at least a year, and the idea is that it's opt-in (if you have a recipe.yaml then new format is used, otherwise fall back to meta.yaml). I hope we're on the same page there :)


# 10: License should not include the word 'license'.
lint_license_should_not_have_license(about_section, lints)
Expand Down
19 changes: 19 additions & 0 deletions conda_smithy/linter/conda_recipe_v2_linter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,22 @@ def lint_usage_of_selectors_for_noarch(
"the selectors are necessary, please remove "
f"`noarch: {noarch_value}`."
)


def lint_sources(sources_section: list[dict[str, Any]], lints: List[str]):
for source in sources_section:
if "url" in source:
# make sure that we have a hash
if not (source.keys() & {"sha256", "md5"}):
lints.append("Source must have a sha256 or md5 checksum.")
# make sure that the hash is not a template (${{ ... }} will not match)
if source.get("sha256"):
if not re.match(r"[0-9a-f]{64}", source["sha256"]):
lints.append(
"sha256 checksum must be 64 characters long. Templates are not allowed for the sha256 checksum."
)
if source.get("md5"):
if not re.match(r"[0-9a-f]{32}", source["md5"]):
lints.append(
"md5 checksum must be 32 characters long. Templates are not allowed for the md5 checksum."
)
Loading