Skip to content

Remove slashes and commas from variant names#2370

Merged
h-vetinari merged 2 commits into
conda-forge:mainfrom
h-vetinari:slash
Aug 22, 2025
Merged

Remove slashes and commas from variant names#2370
h-vetinari merged 2 commits into
conda-forge:mainfrom
h-vetinari:slash

Conversation

@h-vetinari

Copy link
Copy Markdown
Member

Fix issues discovered by py3.14 migration. Currently, the fact that we have values like

channel_sources:
- conda-forge,conda-forge/label/python_rc

in a zip-key, which ends up in the variant file names as

.ci_support/linux_aarch64_channel_sourcesconda-forge,conda-forge/label/python_rcpython3.14.____cp314.yaml
                                                                ^     ^
                                                                !!!!!!!

This causes obvious issues because these slashes cause folders where there shouldn't be any (c.f. #2366). Just removing slashes from the filenames fixes this.

Fixes #2366

@h-vetinari h-vetinari requested a review from a team as a code owner August 22, 2025 12:06
@beckermr

Copy link
Copy Markdown
Member

Does this get squash merged?

@h-vetinari

h-vetinari commented Aug 22, 2025

Copy link
Copy Markdown
Member Author

Does this get squash merged?

I'd say no, but as you wish; b8a55f3 is really independent and just necessary to unbreak CI. I could squash the other two with an interactive rebase, if you prefer

@h-vetinari h-vetinari merged commit 3278c03 into conda-forge:main Aug 22, 2025
2 checks passed
@h-vetinari h-vetinari deleted the slash branch August 22, 2025 12:32
@beckermr

Copy link
Copy Markdown
Member

I don't care either way, but folks have preferences so I'm asking more than I used to. :)

@beckermr

Copy link
Copy Markdown
Member

It turns out that conda-recipe-manager simply does not raise errors any more on for loops as opposed to actually parsing them. That seems like a bug in crm?

cc @schuylermartin45 for viz

@schuylermartin45

Copy link
Copy Markdown

I think I'm missing some context, but here are my initial thoughts:

  1. We recently changed how exceptions are handled. We catch all exceptions from the parsing stage, re-raise them as a ParsingException and then log the entire trace. That way folks down the line can write better exception handlers: https://github.com/conda/conda-recipe-manager/pull/412/files#diff-fb0998d8d273c4070015eb731daf03346cd252dcfde16dd46d14c0fa9270ab04R603

  2. There have been a lot of improvements to our parsing success rate lately. I wonder if the confusion is around something that was once not supported and now is supported(?)

Do you have the offending file recipe file that we can look at?

Comment thread tests/test_lint_recipe.py

def test_lint_recipe_parses_forblock():
with tempfile.TemporaryDirectory() as tmpdir:
# CRM cannot parse this one

@h-vetinari h-vetinari Aug 27, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@schuylermartin45, this test (just unfold below) started not triggering the hint "The recipe is not parsable by parser conda-recipe-manager" anymore. Not sure what conda-smithy does internally there, perhaps just something changed w.r.t. what smithy expects CRM to do

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh I see it/understand now. Let me take a look. We did not intentionally add support for the JINJA for loop syntax, so this is surprising.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @h-vetinari. This is indeed why I started looking at this more carefully.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One of the reasons I wanted to keep the commits separate is that now you can just revert a801ab1 on top of main, and go from there. :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I went through the release tags and this difference is a few months old. CRM v0.5.1 is the last version to throw-on-parse, CRM v0.6.0 (released in June) is the first to not throw.

@schuylermartin45 schuylermartin45 Aug 27, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tracking: conda/conda-recipe-manager#430

This is a little nuanced. I'm guessing this change is a consequence of our efforts to improve parsing compatibility. So to re-introduce this exception on purpose feels weird to me.

At the same time, I understand the reasoning why we should. We don't "support" editing or manipulating or rendering such loops.

One could argue the CRM CLI command crm bump-recipe is improved by ignoring this support gap (because a recipe can still be bumped to a new version) and crm convert should at least warn the user about the unsupported syntax.

There is a function I've left unimplemented for a long time, just in case we ever needed it: RecipeReader::has_unsupported_statements(). Maybe it is time to circle back on that idea.

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.

When using rattler cpython 3.14rc migration has / in files under .ci_support

4 participants