NEW: Better package splitting for multi-output recipes with negative glob in outputs/files #5216

carterbox · 2024-03-06T01:20:20Z

Description

Ressurects #4197. Closes #4196.

The purpose of this PR is to make it easier to split packages into multiple outputs using glob expressions. This is accomplished in two ways:

A negative pattern match is provided. This makes it easier to use fewer glob expressions if you need to include an entire directory tree except for a single file (type). For example, you might want to glob lib/**/libfoo*, but not lib/**/*.a
Only the files installed to the PREFIX in the top-level build are considered. This removes the need to craft your glob expressions to avoid the artifacts installed to the PREFIX by host dependencies. For example, you can now glob include/**/* without pulling in the headers of other packages. It is already the behavior of a single-output recipe to ignore files added to the prefix by host dependencies, so I'm not sure why this feature didn't make it into multi-output recipes.

These new behaviors are only used if the include/exclude keywords are used under the files key. The previous behavior is retained.

outputs:
  - name: foo
    files:
      - bin/*  # old behavior; includes artifacts from other packages
  - name: bar
    files:
      include:
        - bin/*  # new behavior; only matches artifacts from this recipe
      exclude:  # optional
        - bin/*.exe # new behavior
  - name: zee
    script: install.py  # old behavior

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?

closes conda#4196

as keys in `files`

conda-bot · 2024-03-06T01:20:42Z

We require contributors to sign our Contributor License Agreement and we don't have one on file for @carterbox.

In order for us to review and merge your code, please e-sign the Contributor License Agreement PDF. We then need to manually verify your signature, merge the PR (conda/infrastructure#891), and ping the bot to refresh the PR.

docs/source/resources/define-metadata.rst

codspeed-hq · 2024-03-20T17:10:49Z

CodSpeed Performance Report

Merging #5216 will not alter performance

_{Comparing carterbox:files-exclude (4f988f9) with main (3f9346f)}

Summary

✅ 3 untouched benchmarks

carterbox · 2024-03-20T19:49:28Z

I'm not sure that the failure for linux 3.8 23.5.0 serial is related to this PR.

----------------------------- Captured stderr call -----------------------------
Traceback (most recent call last):
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/conda/exception_handler.py", line 16, in __call__
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/conda/cli/main.py", line 66, in main_subshell
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/conda/cli/conda_argparse.py", line 31, in <module>
  File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/conda/base/context.py", line 24, in <module>
ModuleNotFoundError: No module named 'conda._vendor.appdirs'

jaimergp

I like this a lot. The PR is super clear and the documentation is on-point. I only added a couple comments to clarify my own doubts. The new positional argument could be considered API breaking, so let's see how the rest of the team feels about it. I don't have strong feelings, just applying an abundance of caution Just In Case ✨ .

Once we have discussed that, I'll be super happy to approve. 👍

jaimergp · 2024-03-21T14:17:50Z

conda_build/build.py

+    metadata: MetaData,
+    env,
+    stats,
+    new_prefix_files: set[str],


Since this is adding a new positional argument to a (let's assume) public signature, what about making it a None kwarg. This way downstream users won't run into number of arguments errors:

Suggested change

new_prefix_files: set[str],

new_prefix_files: set[str] = None,

This is a good point; it is API breaking assuming these functions are part of the public API. I need to mark the new argument as typing.Optional and/or set appropriate defaults.

conda_build/build.py

jaimergp · 2024-03-21T14:22:05Z

conda_build/build.py

+    metadata: MetaData,
+    env,
+    stats,
+    new_prefix_files: set[str],


Same comment here about the API breakage. Not sure how prominent the usage of wheel outputs is, though.

#5216 (comment)

jaimergp · 2024-03-21T14:23:27Z

docs/source/resources/define-metadata.rst

@@ -1299,9 +1299,10 @@ You can specify files to be included in the package in 1 of
 Explicit file lists are relative paths from the root of the
 build prefix. Explicit file lists support glob expressions.
 Directory names are also supported, and they recursively include
-contents.
+contents. Files installed to the prefix by host dependencies will
+be matched by glob expressions.


I would maybe even highlight this further with an admonition or something similar. I wasn't aware of that design issue!

with an admonition or something similar.

I don't know what you mean. You want stronger language? For example: "WARNING! Files installed to the prefix by host dependencies also will be matched by glob expressions."

I wasn't aware of that design issue!

Yeah, I'm not sure whether this is a bug/oversight or a feature. It's definitely unexpected to me. The most active contributor in this section is @msarahan; maybe they know?

I meant an admonition in the RST sense. Triple-backtick + warning kind of syntax. Equivalent to this:

Warning
Files installed to the prefix by host dependencies also will be matched by glob expressions.

Done in 642906b

news/files-exclude.rst

jaimergp · 2024-03-21T14:26:27Z

tests/test-recipes/split-packages/copying_files/meta.yaml

    files:
-      - subpackage_file1
-      - somedir
-      - "*.ext"
+      include:
+        - subpackage_file1
+        - somedir
+        - "*.ext"
+      exclude:
+        - "*3.ext"


Should we replace the old test with a new one instead of adding a new case? Maybe I would add a new output in this meta.yaml and then test the different behaviors differently, if possible (e.g. how the list-of-str behaviour does copy anything in PREFIX, but include/exclude doesn't?).

I have updated the test recipe to include multiple outputs: one uses the old file matching and one uses the new file matching.

mbargull · 2024-03-21T14:55:56Z

docs/source/resources/define-metadata.rst

+Files can be excluded by specifying `files` as a dictionary separating
+files to `include` from those to `exclude`. Files installed to the prefix
+by host dependencies are automatically excluded when the include/exclude
+syntax is used:


We need to mention the precedence here, i.e., exclude entries have higher priority as per the implementation.

This mentions the new behavior, i.e., "Files [...] excluded when the include/exclude syntax is used".
But since this is a deviation from the previous "include" logic (which would be more of a force-include since it allows packaging pre-existing files.), I think we should make the difference more prevalent/explain the behavior for the old non-dict case more explicitly.

We need to mention the precedence here, i.e., exclude entries have higher priority as per the implementation.

exclude has a higher priority than which operation? It's unclear to me what ambiguity we would be removing by mentioning priority.

This mentions the new behavior, i.e., "Files [...] excluded when the include/exclude syntax is used". But since this is a deviation from the previous "include" logic (which would be more of a force-include since it allows packaging pre-existing files.), I think we should make the difference more prevalent/explain the behavior for the old non-dict case more explicitly.

Suggested change

Files can be excluded by specifying `files` as a dictionary separating

files to `include` from those to `exclude`. Files installed to the prefix

by host dependencies are automatically excluded when the include/exclude

syntax is used:

When defining `outputs/files` as a list, any file in the prefix (including those

installed by host dependencies) matching one of the glob expressions is

included in the output. Greater control over file matching may be

achieved by defining `files` as a dictionary separating files to

`include` from those to `exclude`.

When using include/exclude, only files installed by

the current recipe are considered. i.e. files in the prefix installed

by host dependencies are not matched. include/exclude may not be used

simultaneously with glob expressions listed directly in `outputs/files`.

We need to mention the precedence here, i.e., exclude entries have higher priority as per the implementation.

exclude has a higher priority than which operation?

The kind of precedence where if you do

files: include: - lib/libfoo.so exclude: - lib/libfoo.so

the exclusion wins.

It is now explained that when files match both exclude and include, they are excluded. d1b12a1

jaimergp · 2024-03-21T17:31:08Z

conda_build/build.py

+            include = files.get("include", [])
+            exclude = files.get("exclude", [])


This syntax will be affected by the "key is set to an empty block, which means None in YAML" kind of issue. We should change it to:

Suggested change

include = files.get("include", [])

exclude = files.get("exclude", [])

include = files.get("include") or []

exclude = files.get("exclude") or []

It might also be a good opportunity to sneak @jakirkham's changes added by #4971.

See line 1818:

- files = output.get("files", []) + files = output.get("files") or []

My understanding is that files.get("include") may return None, but include must be a list. So we use the falsiness of None to replace it with []. Equivalent to:

include = files['include'] if ('include' in files and files['include'] != None) else []

You might have

- output: something files: include: - path exclude: - not-this-path # [linux]

and then you'll have files["exclude"] = None on macOS, for example (linux is false). Not sure at what point we are ensuring that include is a list.

Under that circumstance files.get("exclude", []) will be None instead of the expected []. Is that what you mean? I think we are saying the same 😬

Yes. We are saying the same thing. I'm just trying to figure out if we should be doing something stronger than converting Falsy values into the empty list. Perhaps convert any non-list into a list? Something like:

include = files.get("include") if isinstance(files.get("include"), list) else []

That might silence errors when users populate it with a dict or something accidentally. I think we can just play it simple with falsy promotions.

jaimergp · 2024-03-21T17:44:34Z

conda_build/build.py

@@ -2817,7 +2848,7 @@ def build(
                    # This is wrong, files has not been expanded at this time and could contain
                    # wildcards.  Also well, I just do not understand this, because when this
                    # does contain wildcards, the files in to_remove will slip back in.
-                    if "files" in output_d:
+                    if "files" in output_d and not isinstance(output_d["files"], dict):


Suggested change

if "files" in output_d and not isinstance(output_d["files"], dict):

if not isinstance(output_d.get("files") or (), dict):

I have updated this condition to:

if ( "files" in output_d and output_d["files"] is not None and not isinstance(output_d["files"], dict) ):

docs/source/resources/define-metadata.rst

jaimergp · 2024-04-10T07:40:05Z

news/files-exclude.rst

+Enhancements:
+-------------
+
+* implemented new include/exclude sections for glob expressions in multi-output `outputs/files`. (#4196 via #5216)


I don't think this is the following the template file for news items. It should be markdown. Can you update it, please? 🙏

e.g. https://github.com/conda/conda-build/blob/main/news/5222-deprecating-conda_interface

The headers should have ###, and the filename is extension-less.

jaimergp

LGTM after fixing the little MD/RST nits.

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

jaimergp · 2024-04-22T20:43:40Z

Traceback (most recent call last):
  File "/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/popen-gw0/test_subpackage_recipes_copyin0/split_packages_file_list_1713435205299/test_tmp/run_test.py", line 51, in <module>
    assert os.path.isfile(filename)
AssertionError
WARNING: Tests failed for my_script_subpackage_files-1.0-0.tar.bz2 - moving package to /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/pytest-of-runner/pytest-0/popen-gw0/test_subpackage_recipes_copyin0/broken

:(

carterbox · 2024-04-22T21:42:54Z

What is the difference between the parallel and serial tests? Only the macos parallel tests are failing. It looks like it only changes parameters related to pytest and codecov, not conda itself?

jaimergp · 2024-04-22T23:11:25Z

Serial jobs run tests that can't be run concurrently. Parallel jobs take rhe rest and run them with two processes wt the same time. The tests need to be marked as serial with a decorator to be considered as such, so the default is parallel.

Package collisions cause errors.

carterbox · 2024-04-25T16:02:33Z

I think the errors are coming from the fact that the subpackage tests use the same artifacts for both outputs, so the subpackages are colliding when they are installed to the same environment?

carterbox · 2024-04-25T17:01:42Z

tests/test_subpackages.py

@@ -18,6 +18,7 @@


 @pytest.mark.slow
+@pytest.mark.serial


Suggested change

@pytest.mark.serial

Try reenabling parallel tests once tests are fixed.

carterbox · 2024-04-30T18:41:59Z

Needs approval to run the refactored tests.

duncanmmacleod and others added 7 commits November 9, 2021 15:15

conda_build.build: simple implementation of files exclusion

a545986

closes conda#4196

tests: add test for glob exclusion in files

14e3548

add docs for negative file matches

7fbc142

news: add negative file match news entry

67904ce

conda_build.build: support separate include/exclude file lists

e56c573

as keys in `files`

Merge branch 'main' into files-exclude

be19766

REF: Only consider new files to the PREFIX when file picking

b667724

conda-bot mentioned this pull request Mar 6, 2024

Adding CLA signee carterbox conda/infrastructure#891

Merged

carterbox commented Mar 6, 2024

View reviewed changes

docs/source/resources/define-metadata.rst Outdated Show resolved Hide resolved

carterbox added 2 commits March 5, 2024 20:30

Update docs/source/resources/define-metadata.rst

063ff89

Merge branch 'main' into files-exclude

1ab7638

carterbox marked this pull request as ready for review March 8, 2024 19:21

carterbox requested a review from a team as a code owner March 8, 2024 19:21

carterbox added 2 commits March 8, 2024 13:27

Update define-metadata.rst

de7715d

Update files-exclude.rst

f56e201

carterbox changed the title ~~NEW: Negative matching 'files' for outputs~~ NEW: Better package splitting for multi-output recipes with negative glob in outputs/files Mar 8, 2024

Merge branch 'main' into files-exclude

0537c19

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

carterbox added 3 commits March 20, 2024 12:47

STY: Add missing __future__ import

30a5e1b

STY: Address RUFF lints

a92964f

STY: Remove extra whitespace in news

a2e3963

Merge branch 'main' into files-exclude

bfac4e0

jaimergp reviewed Mar 21, 2024

View reviewed changes

mbargull reviewed Mar 21, 2024

View reviewed changes

jaimergp reviewed Mar 21, 2024

View reviewed changes

BUG: Use default value to make API backwards-compatible

3c0b090

carterbox dismissed jaimergp’s stale review via 9dbd17c April 2, 2024 14:15

carterbox added 5 commits April 2, 2024 09:21

DOC: Fix RST syntax

c3a26dd

DOC: Fix RST syntax

84aad48

DOC: Fix RST syntax

249a065

Merge branch 'main' into files-exclude

55248d4

TST: Prevent missing glob match error in test

e42b6f0

jaimergp reviewed Apr 10, 2024

View reviewed changes

docs/source/resources/define-metadata.rst Outdated Show resolved Hide resolved

jaimergp reviewed Apr 10, 2024

View reviewed changes

docs/source/resources/define-metadata.rst Outdated Show resolved Hide resolved

jaimergp reviewed Apr 10, 2024

View reviewed changes

jaimergp previously approved these changes Apr 10, 2024

View reviewed changes

carterbox dismissed jaimergp’s stale review via a62e4b9 April 10, 2024 15:33

carterbox and others added 4 commits April 10, 2024 10:33

DOC: Fix RST syntax

a62e4b9

Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>

Update news to new template style

e28565f

DOC: Fixup syntax highlighting in adjacent section

ee89c04

Merge branch 'main' into files-exclude

b0645c2

jaimergp previously approved these changes Apr 18, 2024

View reviewed changes

TST: Mark subpackage tests as serial tests to avoid errors on osx

822b477

carterbox dismissed jaimergp’s stale review via 822b477 April 23, 2024 17:36

carterbox added 2 commits April 23, 2024 12:36

Merge branch 'main' into files-exclude

475c0ec

TST: Avoid package collisions in subpackage tests

4f988f9

Package collisions cause errors.

TST: Avoid more package collisions in tests

b33f6f6

carterbox commented Apr 25, 2024

View reviewed changes

carterbox requested a review from jaimergp April 26, 2024 18:07

carterbox mentioned this pull request Apr 29, 2024

REF: Split libraries into separate outputs conda-forge/cairo-feedstock#74

Draft

8 tasks

h-vetinari mentioned this pull request May 9, 2024

add recipe extension cep conda/ceps#72

Open

	new_prefix_files: set[str],
	new_prefix_files: set[str] = None,

-Files can be excluded by specifying `files` as a dictionary separating
-files to `include` from those to `exclude`. Files installed to the prefix
-by host dependencies are automatically excluded when the include/exclude
-syntax is used:
+When defining `outputs/files` as a list, any file in the prefix (including those
+installed by host dependencies) matching one of the glob expressions is
+included in the output. Greater control over file matching may be
+achieved by defining `files` as a dictionary separating files to
+`include` from those to `exclude`.
+When using include/exclude, only files installed by
+the current recipe are considered. i.e. files in the prefix installed
+by host dependencies are not matched. include/exclude may not be used
+simultaneously with glob expressions listed directly in `outputs/files`.

		include = files.get("include", [])
		exclude = files.get("exclude", [])

	if "files" in output_d and not isinstance(output_d["files"], dict):
	if not isinstance(output_d.get("files") or (), dict):

NEW: Better package splitting for multi-output recipes with negative glob in outputs/files #5216

Are you sure you want to change the base?

NEW: Better package splitting for multi-output recipes with negative glob in outputs/files #5216

Conversation

carterbox commented Mar 6, 2024 • edited

Description

Checklist - did you ...

conda-bot commented Mar 6, 2024

codspeed-hq bot commented Mar 20, 2024 • edited

Merging #5216 will not alter performance

Summary

carterbox commented Mar 20, 2024

jaimergp left a 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

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

carterbox Mar 21, 2024 • edited

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

jaimergp Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimergp Mar 21, 2024 • edited

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

jaimergp left a comment

Choose a reason for hiding this comment

jaimergp commented Apr 22, 2024

carterbox commented Apr 22, 2024

jaimergp commented Apr 22, 2024

carterbox commented Apr 25, 2024

carterbox Apr 25, 2024 • edited

Choose a reason for hiding this comment

carterbox commented Apr 30, 2024

carterbox commented Mar 6, 2024 •

edited

codspeed-hq bot commented Mar 20, 2024 •

edited

carterbox Mar 21, 2024 •

edited

jaimergp Mar 21, 2024 •

edited

jaimergp Mar 21, 2024 •

edited

carterbox Apr 25, 2024 •

edited