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

which_package to yield unique collection of package records #5108

Merged
merged 10 commits into from
Dec 14, 2023
Merged

which_package to yield unique collection of package records #5108

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
20d0a12
Add which_package unittest
kenodegard Dec 12, 2023
7998d33
Only yield package record once
kenodegard Dec 12, 2023
3f19216
Add news
kenodegard Dec 12, 2023
f955083
Skip using tmp_env in older conda
kenodegard Dec 12, 2023
2ce0e20
Refactor test_which_package into a local test
kenodegard Dec 14, 2023
904078a
Prefer str over repr in _lookup_in_prefix_packages
kenodegard Dec 14, 2023
424e1f2
Update news/5108-fix-which_package
kenodegard Dec 14, 2023
89c7bc8
Set comparison
kenodegard Dec 14, 2023
496092c
Fix softlink test per feedback
kenodegard Dec 14, 2023
05785c7
Remove tmp_env fixture
kenodegard Dec 14, 2023
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: 2 additions & 3 deletions conda_build/inspect_pkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,8 @@ def which_package(
path = prefix / path

for prec in PrefixData(str(prefix)).iter_records():
for file in prec["files"]:
if samefile(prefix / file, path):
yield prec
if any(samefile(prefix / file, path) for file in prec["files"]):
Copy link
Contributor

@dholth dholth Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(n * m) ? I see this version skips work if true.

Too bad to iterate over all installed files to do the search

Copy link
Contributor Author

Choose a reason for hiding this comment

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

worst case yes, same as before

any returns on the first True: https://docs.python.org/3/library/functions.html#any

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what avoids the duplicated records to begin with, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In a subsequent PR, we could see if implementing a cached path -> artifact map would speed this up. It can be a veeery slow part of the post processing stage in conda-build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what avoids the duplicated records to begin with, right?

correct, this is essentially what was happening pre #5041:

conda-build/conda_build/inspect_pkg.py

Lines 57 to 62 in c71c4ab

for dist in fn(prefix):
# dfiles = set(dist.get('files', []))
dfiles = dist_files(prefix, dist)
# TODO :: This is completely wrong when the env is on a case-sensitive FS!
if any(norm_ipp == normcase(w) for w in dfiles):
yield dist

cached path -> artifact map

definitely something to consider

yield prec


def print_object_info(info, key):
Expand Down
19 changes: 19 additions & 0 deletions news/5108-fix-which_package
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
### Enhancements

* <news item>

### Bug fixes

* Fix `conda_build.inspect_pkg.which_package` so it does not return duplicate package records. (#5108)
kenodegard marked this conversation as resolved.
Show resolved Hide resolved

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
2 changes: 2 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
from conda_build.utils import check_call_env, copy_into, prepend_bin_path
from conda_build.variants import get_default_variant

pytest_plugins = ["conda.testing"]

kenodegard marked this conversation as resolved.
Show resolved Hide resolved

@pytest.fixture(scope="function")
def testing_workdir(monkeypatch: MonkeyPatch, tmp_path: Path) -> Iterator[str]:
Expand Down
26 changes: 26 additions & 0 deletions tests/test_inspect_pkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Copyright (C) 2014 Anaconda, Inc
# SPDX-License-Identifier: BSD-3-Clause
from __future__ import annotations

import pytest
from conda import __version__ as conda_version
from conda.core.prefix_data import PrefixData
from packaging.version import Version, parse

from conda_build.inspect_pkg import which_package

if TYPE_CHECKING := False:
from conda.testing import TmpEnvFixture


@pytest.mark.skipif(
parse(conda_version) < Version("23.5.0"),
reason="tmp_env fixture first available in conda 23.5.0",
)
def test_which_package(tmp_env: TmpEnvFixture):
with tmp_env("ca-certificates") as prefix:
pd = PrefixData(prefix)
prec = pd.get("ca-certificates")
precs = list(which_package(prefix / prec.files[0], prefix))
kenodegard marked this conversation as resolved.
Show resolved Hide resolved
assert len(precs) == 1
assert precs[0] == prec
Loading