Restore pre-`3.28` behavior for `which_package` #5141

mbargull · 2024-01-16T20:24:46Z

Description

Fix linking check regressions by restoring pre-3.28 behavior for which_package.

Fixes gh-5136.

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?~~

Signed-off-by: Marcel Bargull <[email protected]>

mbargull · 2024-01-16T23:18:30Z

conda_build/inspect_pkg.py

    try:
-        lstat = (prefix / path).lstat()
-    except FileNotFoundError:
-        # FileNotFoundError: path doesn't exist
-        return
-    else:
-        yield from _file_package_mapping(prefix).get(lstat, ())
-
+        path = Path(path).relative_to(prefix)
+    except ValueError:
+        # If path is already relative to prefix, we get a ValueError.
+        pass


I don't know why this was changed to take paths not relative to the prefix in 3.28 .
To keep the change set small, I just added handling of non-relatives paths here instead of at which_packages call sites.

yeah this change came about because I saw how before every call to which_package we would strip the prefix, so inlining the relative logic here makes sense

conda_build/inspect_pkg.py

mbargull · 2024-01-16T23:25:17Z

tests/conftest.py

+            "git",
+            "clone",
+            "https://github.com/conda/conda_build_test_recipe",
+            "--branch=1.21.11",


This is an unrelated change to make the tests pass.
It should probably not be picked verbatim for the main branch.

since we're about to start the January releases I'm thinking we will want to change the target branch and revert this change

Do you mean this commit in particular or the whole PR?
I'm totally fine to drop the commit -- it was just a means to get "all green".
For the PR itself, I'd like us to have another 3.28 patch release if possible.
The thing is that we don't have any working 3.28 package on conda-forge (some are already marked broken) and I'd like us to get one such release before we introduce further changes from the upcoming version.
Meaning, as a maintainer of infrastructure-related packages on conda-forge, I'd like to do one of the following:

Put out a build for a new 3.28 patch release once tagged upstream.

Add this PR as a patch to the recipe for the current version to get a working downstream build.

Mark all 3.28 builds as broken and thus continue to use 3.27 for now on conda-forge.

Option 1 being the most preferred.
Option 2 is not ideal since we'd have divergent behavior up- and downstream.
Option 3 would be the last resort to get things working again.
(For conda-forge we'd rather want to increase not decrease the use of linking checks and omitting the linking checks via config would only create maintenance burden and is thus not an option I'm willing to go with.)

working with the team to figure out a release plan

added to 24.1.x milestone only because this is something that will need to be resolved and merged before the release can proceed

mbargull · 2024-01-16T23:36:59Z

tests/test_inspect_pkg.py

+    # Files might be deleted from the prefix during the build, but they should
+    # still be recognized since they will be present in the run environment.
+    (tmp_path / "deleted").unlink(missing_ok=True)  # packageA
+    (tmp_path / "deleted_shared").unlink(missing_ok=True)  # packageA & packageB


The linking checks are actually run on the host prefix (an environment for actual run dependencies is not created unless tests are run).
The assumptions on the linking checks are actually about what would be provided via run dependencies -- i.e., the checks on the host prefix are not entirely accurate.

This conceptual issue is something to be discussed elsewhere.

But taking it as a status-quo leaves us with the issue that the previous 3.28 code seemed to more expect these checks to happen on an actual run-dependency-resolved environment.
Since it is not the case, we'd rather have to work on the host prefix' properties.
One such property is that a build script might remove files arbitrarily (e.g., one would link to $CC prog.c -o prog -l somelib ; rm libsomelib.so and conda-build=3.27 would still detect the package that provides libsomelib.so).

ah I see, a mistake on my part in understanding where overlinking detection happened

Yeah, I wouldn't blame you for that :D.
The whole thing is probably under-documented, not the "most correct way", and also the code can be confusing since it has a host_prefix -> run_prefix renaming with the calls to check_overlinking_impl...

tests/test_inspect_pkg.py

mbargull · 2024-01-16T23:40:18Z

tests/test_inspect_pkg.py

+    # removed files should still return a package
+    # this occurs when, e.g., a build script removes files installed by another package
+    # (The opposite case with files really missing from the run environment,
+    # e.g., due to a post-install script removing them, is less likely and not
+    # covered.)
+    #  covered here.)
    for file in removed:
-        assert not len(list(which_package(tmp_path / file, tmp_path)))
+        assert len(list(which_package(tmp_path / file, tmp_path))) == 1


This change is a direct consequence of https://github.com/conda/conda-build/pull/5141/files#r1454205167 above, i.e., files may be removed during build from the host prefix.

mbargull · 2024-01-16T23:49:37Z

@kenodegard, I reviewed the code for which_package for <=3.27and found some more behavioral changes that3.28.x` introduced.
See the code as well as review comments in this PR for details.

I think we should revert the function's behavior to (mostly) what was done in <3.27 and put out a new 3.28 patch release for it.
We should discuss further changes (behavioral and/or performance-wise) for future versions rather than the 3.28.x line.

Co-authored-by: Ken Odegard <[email protected]>

kenodegard

minor suggestions

tests/test_inspect_pkg.py

news/5141-fix-which_package

conda_build/inspect_pkg.py

Co-authored-by: Ken Odegard <[email protected]>

kenodegard

I added the regression test I wrote in #5140, otherwise LGTM

minrk · 2024-01-18T08:31:17Z

Thanks for the fix!

conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jan 16, 2024

mbargull force-pushed the fix-which_package branch from 672e8b9 to 7f39942 Compare January 16, 2024 20:35

Add test for deleted files in which_package

6ed8ba1

Signed-off-by: Marcel Bargull <[email protected]>

mbargull force-pushed the fix-which_package branch from 7f39942 to 6ed8ba1 Compare January 16, 2024 20:36

mbargull added 4 commits January 16, 2024 22:07

Add which_package case-insensitivity test on Win

f27d5cb

Signed-off-by: Marcel Bargull <[email protected]>

Restore pre-3.28 behavior for which_package

c35e194

Signed-off-by: Marcel Bargull <[email protected]>

Change removed file handling in which_package test

b27f6f5

Signed-off-by: Marcel Bargull <[email protected]>

Fix expected tag for test_git_in_output_version

0ef8203

Signed-off-by: Marcel Bargull <[email protected]>

mbargull force-pushed the fix-which_package branch from 2d2873d to 0ef8203 Compare January 16, 2024 23:13

mbargull commented Jan 16, 2024

View reviewed changes

conda_build/inspect_pkg.py Outdated Show resolved Hide resolved

mbargull commented Jan 16, 2024

View reviewed changes

tests/test_inspect_pkg.py Outdated Show resolved Hide resolved

mbargull commented Jan 16, 2024

View reviewed changes

mbargull marked this pull request as ready for review January 16, 2024 23:44

mbargull mentioned this pull request Jan 17, 2024

Invalidate cache after adding/removing packages #5140

Closed

3 tasks

kenodegard linked an issue Jan 17, 2024 that may be closed by this pull request

Link checker cannot find dependencies for multi-output recipe in 3.28.3 #5136

Closed

Remove superfluous path separator replacement in which_package

eb7c846

Co-authored-by: Ken Odegard <[email protected]>

kenodegard previously approved these changes Jan 17, 2024

View reviewed changes

kenodegard added this to the 24.1.x milestone Jan 17, 2024

mbargull dismissed kenodegard’s stale review via c3ca583 January 17, 2024 16:26

which_package: Apply style+test refinements from code review

c3ca583

Co-authored-by: Ken Odegard <[email protected]>

kenodegard requested a review from jezdez January 17, 2024 18:10

kenodegard mentioned this pull request Jan 17, 2024

Release 3.28.x #5071

Closed

67 tasks

Add regression test

97dfe65

kenodegard approved these changes Jan 17, 2024

View reviewed changes

kenodegard enabled auto-merge (squash) January 17, 2024 19:23

kenodegard removed this from the 24.1.x milestone Jan 17, 2024

kenodegard added this to the 3.28.0 milestone Jan 17, 2024

kenodegard merged commit d0daa2b into conda:3.28.x Jan 17, 2024
23 checks passed

This was referenced Jan 18, 2024

Link checker cannot find dependencies for multi-output recipe in 3.28.3 #5136

Closed

purge _file_package_mapping cache when re-using prefixes for multiple outputs #5138

Closed

kenodegard removed this from the 3.28.0 milestone Jan 18, 2024

github-actions bot added the locked [bot] locked due to inactivity label Jan 18, 2025

github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore pre-`3.28` behavior for `which_package` #5141

Restore pre-`3.28` behavior for `which_package` #5141

mbargull commented Jan 16, 2024 •

edited

Loading

mbargull Jan 16, 2024

kenodegard Jan 17, 2024

mbargull Jan 16, 2024

kenodegard Jan 17, 2024

mbargull Jan 17, 2024

kenodegard Jan 17, 2024

mbargull Jan 16, 2024

kenodegard Jan 17, 2024

mbargull Jan 17, 2024

mbargull Jan 16, 2024

mbargull commented Jan 16, 2024

kenodegard left a comment

kenodegard left a comment

minrk commented Jan 18, 2024

Restore pre-3.28 behavior for which_package #5141

Restore pre-3.28 behavior for which_package #5141

Conversation

mbargull commented Jan 16, 2024 • edited Loading

Description

Checklist - did you ...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargull commented Jan 16, 2024

kenodegard left a comment

Choose a reason for hiding this comment

kenodegard left a comment

Choose a reason for hiding this comment

minrk commented Jan 18, 2024

Restore pre-`3.28` behavior for `which_package` #5141

Restore pre-`3.28` behavior for `which_package` #5141

mbargull commented Jan 16, 2024 •

edited

Loading