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

Fix MetaData.get_section handling for "source" & "outputs" #5112

Merged
merged 9 commits into from
Dec 15, 2023

Conversation

kenodegard
Copy link
Contributor

@kenodegard kenodegard commented Dec 12, 2023

Description

The recipe format supports 9 fields, all of which are expected to be dict except "source" and "outputs" which can be either a single dict or a list of dict.

Previously this distinction had to be uniquely handled throughout out the code, e.g.:

sources = m.get_section("source")
if hasattr(sources, "keys"):
sources = [sources]

It instead makes more sense for the MetaData.get_section to auto handle this sanitization for us and to always return a list of dict for "source" and "outputs".

Resolves #5111

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@kenodegard kenodegard self-assigned this Dec 12, 2023
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Dec 12, 2023
@kenodegard kenodegard requested a review from a team December 14, 2023 17:00
jezdez
jezdez previously approved these changes Dec 14, 2023
conda_build/metadata.py Outdated Show resolved Hide resolved
assert (
not index
), f"Got non-zero index ({index}), but section {section} is not a list."
if isinstance(section_data, dict) and index:
Copy link
Member

@marcelotrevisani marcelotrevisani Dec 14, 2023

Choose a reason for hiding this comment

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

Suggested change
if isinstance(section_data, dict) and index:
if isinstance(section_data, Mapping) and index:

you might need to import it as well

from collections.abc import Mapping

if len(names) == 2:
section, key = names
index = None
elif len(names) == 3:
section, index, key = names
assert section == "source", "Section is not a list: " + section
if section not in OPTIONALLY_ITERABLE_FIELDS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunate name, given how dicts are also iterable, technically speaking. Not that we should change it, but I wonder if MAYBE_LIST_OF_DICTS_FIELDS (or similar) would have been better.

@jaimergp jaimergp linked an issue Dec 14, 2023 that may be closed by this pull request
2 tasks
beeankha
beeankha previously approved these changes Dec 14, 2023
@kenodegard kenodegard mentioned this pull request Dec 14, 2023
67 tasks
Comment on lines +1320 to +1322
m.path = ""
m._meta_path = ""
m.requirements_path = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

initialization missing that was discovered by the newly added unit test

Copy link
Member

Choose a reason for hiding this comment

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

Huh!

Comment on lines +1320 to +1322
m.path = ""
m._meta_path = ""
m.requirements_path = ""
Copy link
Member

Choose a reason for hiding this comment

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

Huh!

@kenodegard kenodegard merged commit f2087e6 into conda:3.28.x Dec 15, 2023
24 checks passed
@kenodegard kenodegard deleted the fix-5111 branch December 15, 2023 16:52
@@ -622,6 +622,7 @@ def parse(data, config, path=None):
"prelink_message": None,
"readme": None,
},
"extra": {},
Copy link
Member

Choose a reason for hiding this comment

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

extra could be anything else (scalar or sequence, not just mapping) acc. to https://docs.conda.io/projects/conda-build/en/stable/resources/define-metadata.html#extra-section .
This broke an assumption in conda-smithy at least: conda-forge/conda-smithy#1816 .

So, technically a breaking change; but I'd be okay with requiring a mapping here.
This needs an update to the docs, though.

Copy link
Member

Choose a reason for hiding this comment

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

Opened gh-5125 to track this.

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Dec 20, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

source lists do not allow path in conda-build 3.28.x
7 participants