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
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
10 changes: 3 additions & 7 deletions conda_build/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -1564,15 +1564,11 @@ def create_info_files(m, replacements, files, prefix):

write_no_link(m, files)

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

with open(join(m.config.info_dir, "git"), "w", encoding="utf-8") as fo:
for src in sources:
if src.get("git_url"):
for source_dict in m.get_section("source"):
if source_dict.get("git_url"):
source.git_info(
os.path.join(m.config.work_dir, src.get("folder", "")),
os.path.join(m.config.work_dir, source_dict.get("folder", "")),
m.config.build_prefix,
git=None,
verbose=m.config.verbose,
Expand Down
86 changes: 58 additions & 28 deletions conda_build/metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from collections import OrderedDict
from functools import lru_cache
from os.path import isfile, join
from typing import Literal
from typing import Literal, overload

from bs4 import UnicodeDammit

Expand Down Expand Up @@ -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.

}

# Fields that may either be a dictionary or a list of dictionaries.
Expand Down Expand Up @@ -1316,9 +1317,11 @@ def parse_until_resolved(
@classmethod
def fromstring(cls, metadata, config=None, variant=None):
m = super().__new__(cls)
if not config:
config = Config()
m.meta = parse(metadata, config=config, path="", variant=variant)
m.path = ""
m._meta_path = ""
m.requirements_path = ""
Comment on lines +1320 to +1322
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!

config = config or Config(variant=variant)
m.meta = parse(metadata, config=config, path="")
m.config = config
m.parse_again(permit_undefined_jinja=True)
return m
Expand All @@ -1333,18 +1336,45 @@ def fromdict(cls, metadata, config=None, variant=None):
m._meta_path = ""
m.requirements_path = ""
m.meta = sanitize(metadata)

if not config:
config = Config(variant=variant)

m.config = config
m.config = config or Config(variant=variant)
m.undefined_jinja_vars = []
m.final = False

return m

def get_section(self, section):
return self.meta.get(section, {})
@overload
def get_section(self, section: Literal["source", "outputs"]) -> list[dict]:
...

@overload
def get_section(
self,
section: Literal[
"package",
"build",
"requirements",
"app",
"test",
"about",
"extra",
],
) -> dict:
...

def get_section(self, name):
section = self.meta.get(name)
if name in OPTIONALLY_ITERABLE_FIELDS:
if not section:
return []
elif isinstance(section, dict):
return [section]
elif not isinstance(section, list):
raise ValueError(f"Expected {name} to be a list")
else:
if not section:
return {}
elif not isinstance(section, dict):
raise ValueError(f"Expected {name} to be a dict")
return section

def get_value(self, name, default=None, autotype=True):
"""
Expand All @@ -1358,24 +1388,26 @@ def get_value(self, name, default=None, autotype=True):
:return: The named value from meta.yaml
"""
names = name.split("/")
assert len(names) in (2, 3), "Bad field name: " + name
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.

raise ValueError(f"Section is not indexable: {section}")
index = int(index)
else:
raise ValueError(f"Bad field name: {name}")

# get correct default
if autotype and default is None and FIELDS.get(section, {}).get(key):
default = FIELDS[section][key]()

section_data = self.get_section(section)
if isinstance(section_data, dict):
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

raise ValueError(
f"Got non-zero index ({index}), but section {section} is not a list."
)
kenodegard marked this conversation as resolved.
Show resolved Hide resolved
elif isinstance(section_data, list):
# The 'source' section can be written a list, in which case the name
# is passed in with an index, e.g. get_value('source/0/git_url')
Expand All @@ -1386,13 +1418,12 @@ def get_value(self, name, default=None, autotype=True):
)
index = 0

if len(section_data) == 0:
if not section_data:
section_data = {}
else:
section_data = section_data[index]
assert isinstance(
section_data, dict
), f"Expected {section}/{index} to be a dict"
if not isinstance(section_data, dict):
raise ValueError(f"Expected {name} to be a dict")

value = section_data.get(key, default)

Expand Down Expand Up @@ -1475,7 +1506,7 @@ def get_depends_top_and_out(self, typ):
if not self.is_output:
matching_output = [
out
for out in self.meta.get("outputs", [])
for out in self.get_section("outputs")
if out.get("name") == self.name()
]
if matching_output:
Expand Down Expand Up @@ -2014,7 +2045,7 @@ def uses_jinja(self):
return len(matches) > 0

@property
def uses_vcs_in_meta(self) -> Literal["git" | "svn" | "mercurial"] | None:
def uses_vcs_in_meta(self) -> Literal["git", "svn", "mercurial"] | None:
"""returns name of vcs used if recipe contains metadata associated with version control systems.
If this metadata is present, a download/copy will be forced in parse_or_try_download.
"""
Expand All @@ -2034,7 +2065,7 @@ def uses_vcs_in_meta(self) -> Literal["git" | "svn" | "mercurial"] | None:
return vcs

@property
def uses_vcs_in_build(self) -> Literal["git" | "svn" | "mercurial"] | None:
def uses_vcs_in_build(self) -> Literal["git", "svn", "mercurial"] | None:
# TODO :: Re-work this. Is it even useful? We can declare any vcs in our build deps.
build_script = "bld.bat" if on_win else "build.sh"
build_script = os.path.join(self.path, build_script)
Expand Down Expand Up @@ -2271,9 +2302,8 @@ def pin_depends(self):

@property
def source_provided(self):
return not bool(self.meta.get("source")) or (
os.path.isdir(self.config.work_dir)
and len(os.listdir(self.config.work_dir)) > 0
return not self.get_section("source") or (
os.path.isdir(self.config.work_dir) and os.listdir(self.config.work_dir)
)

def reconcile_metadata_with_output_dict(self, output_metadata, output_dict):
Expand Down
10 changes: 5 additions & 5 deletions conda_build/render.py
Original file line number Diff line number Diff line change
Expand Up @@ -721,17 +721,17 @@ def finalize_metadata(
# if source/path is relative, then the output package makes no sense at all. The next
# best thing is to hard-code the absolute path. This probably won't exist on any
# system other than the original build machine, but at least it will work there.
if source_path := m.get_value("source/path"):
if not isabs(source_path):
m.meta["source"]["path"] = normpath(join(m.path, source_path))
for source_dict in m.get_section("source"):
if (source_path := source_dict.get("path")) and not isabs(source_path):
source_dict["path"] = normpath(join(m.path, source_path))
elif (
(git_url := m.get_value("source/git_url"))
(git_url := source_dict.get("git_url"))
# absolute paths are not relative paths
and not isabs(git_url)
# real urls are not relative paths
and ":" not in git_url
):
m.meta["source"]["git_url"] = normpath(join(m.path, git_url))
source_dict["git_url"] = normpath(join(m.path, git_url))

m.meta.setdefault("build", {})

Expand Down
11 changes: 2 additions & 9 deletions conda_build/source.py
Original file line number Diff line number Diff line change
Expand Up @@ -1027,18 +1027,11 @@ def provide(metadata):
- unpack
- apply patches (if any)
"""
meta = metadata.get_section("source")
if not os.path.isdir(metadata.config.build_folder):
os.makedirs(metadata.config.build_folder)
os.makedirs(metadata.config.build_folder, exist_ok=True)
git = None

if hasattr(meta, "keys"):
dicts = [meta]
else:
dicts = meta

try:
for source_dict in dicts:
for source_dict in metadata.get_section("source"):
folder = source_dict.get("folder")
src_dir = os.path.join(metadata.config.work_dir, folder if folder else "")
if any(k in source_dict for k in ("fn", "url")):
Expand Down
19 changes: 19 additions & 0 deletions news/5112-fix-multiple-sources
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
### Enhancements

* Update `conda_build.metadata.MetaData.get_section` to always return lists for "source" and "outputs". (#5112)

### Bug fixes

* Fix finalizing recipes with multiple sources. (#5112)
kenodegard marked this conversation as resolved.
Show resolved Hide resolved

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
23 changes: 22 additions & 1 deletion tests/test_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
from conda_build import api
from conda_build.config import Config
from conda_build.metadata import (
FIELDS,
OPTIONALLY_ITERABLE_FIELDS,
MetaData,
_hash_dependencies,
get_selectors,
Expand All @@ -23,7 +25,7 @@
)
from conda_build.utils import DEFAULT_SUBDIRS

from .utils import metadata_dir, thisdir
from .utils import metadata_dir, metadata_path, thisdir


def test_uses_vcs_in_metadata(testing_workdir, testing_metadata):
Expand Down Expand Up @@ -459,3 +461,22 @@ def test_get_selectors(
# override with True values
**{key: True for key in expected},
}


def test_fromstring():
MetaData.fromstring((metadata_path / "multiple_sources" / "meta.yaml").read_text())


def test_fromdict():
MetaData.fromdict(
yamlize((metadata_path / "multiple_sources" / "meta.yaml").read_text())
)


def test_get_section(testing_metadata: MetaData):
for name in FIELDS:
section = testing_metadata.get_section(name)
if name in OPTIONALLY_ITERABLE_FIELDS:
assert isinstance(section, list)
else:
assert isinstance(section, dict)
Loading