REF: Use soname package for Windows too by carterbox · Pull Request #25 · conda-forge/libavif-feedstock

carterbox · 2024-01-02T00:31:06Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

conda-forge-webservices · 2024-01-02T00:31:13Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the '_libavif_api' output doesn't have any tests.

…nda-forge-pinning 2023.12.31.22.41.20

carterbox · 2024-02-05T23:20:10Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2024.02.05.13.52.48

…nda-forge-pinning 2024.01.31.01.24.54

…nda-forge-pinning 2024.02.05.13.52.48

conda-forge-webservices · 2024-02-05T23:25:11Z

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

conda-forge-webservices · 2024-02-05T23:26:08Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

It looks like the '_libavif_api' output doesn't have any tests.

hmaarrfk · 2024-02-06T11:26:47Z

do you actually need my review?

It looks pretty good. not sure i love the install.py script but willing to see where it goes!

carterbox · 2024-02-06T18:59:56Z

Yes, I want your review because this is a new idea that I plan to copy to other feedstocks!

The install.py script is supposed to be reusable across multiple recipes. It saves recipe writing maintenance time because it allows the author to avoid writing custom file lists like:

      - include/avif             # [unix]
      - Library\\include\\avif   # [win]
      - lib/libavif.so     # [linux]
      - lib/libavif.dylib  # [osx]
      - Library\\bin\\avif.dll   # [win]
      - Library\\lib\\avif.lib   # [win]
      - lib/pkgconfig/libavif.pc             # [unix]
      - Library\\lib\\pkgconfig\\libavif.pc  # [win]
      - lib/cmake/libavif             # [unix]
      - Library\\lib\\cmake\\libavif  # [win]

for each output of a multi-output recipe.

Do you have any more specific feedback about the install.py script? Any way that I could make it simpler or easier to understand?

hmaarrfk · 2024-02-07T01:48:01Z

+
+    # libfoo-static
+    # libdav1d-static, libv8-static, libm-static, libsecp256k1-static
+    if re.match(f"^lib{ basename }-static$", PKG_NAME):


Suggested change

if re.match(f"^lib{ basename }-static$", PKG_NAME):

if "lib{ basename }-static" == PKG_NAME:

keep it simple?

I agree, for the non-complicated pattern matches, I should probably just use equality instead of regex.

hmaarrfk · 2024-02-07T01:49:32Z

+    print(f"Installing {PKG_NAME} to {PREFIX} for {target_platform}")
+    print("Based on the package name, ", end="")
+
+    if re.match(r"^.+\-split$", PKG_NAME):


Suggested change

if re.match(r"^.+\-split$", PKG_NAME):

if PKG_NAME.endswith("-split"):

hmaarrfk · 2024-02-07T01:51:20Z

+    )
+    for match in sorted(included - excluded):
+        match = pathlib.Path(match)
+        if match.exists():


Suggested change

if match.exists():

if match.is_dir():

continue

if not match.exists():

# runtime error? or just continue?

I can't recall now why glob was returning paths that did not exist. I think a summary of the things that I want to install (copy) are files and symlinks to files (even if broken). I don't want to copy directories.

I have updated the script to explicitly check for file or symlink.

hmaarrfk · 2024-02-07T01:52:24Z

+            if not match.is_dir():
+                print(relative)
+                os.makedirs((PREFIX / relative).parent, exist_ok=True)
+                shutil.copy(


does this work for synlinks too?

i mean, .so -> .so.0 type of symlinks are quite common.

Yes, it does. Because follow_symlinks=False, the link itself is copied and not the file to which the link points.

https://docs.python.org/3/library/shutil.html#shutil.copy

hmaarrfk · 2024-02-07T01:54:20Z

I could make it simpler

I think re is likely overused in these cases. startswith, endwith, and straightup equality seem like they suffice in many cases.

hmaarrfk · 2024-02-07T22:32:16Z

this looks much easier to use.

There was some discussion on trying to standardize the multi-output names. did you consider that discussion in this script?

carterbox · 2024-02-07T23:00:28Z

That's a good idea! I remember reading that discussion a while back, but I didn't actively refer to it while writing this script.

Looking at it now, it seems that main conclusions were to use libfoo instead of foo-cpp and to split binaries into a separate package. avif is library only, so there is no avif package. This script conforms to that advice and also the feedback from my attempt at a policy regarding SONAME packages.

carterbox · 2024-02-07T23:05:20Z

So actually, the one deviation from my script and the output naming discussion is that here the install script assumes the library has a SONAME. In another library without a SONAME, it was suggested that libfoo should not contain headers. The headers should be located in foo-dev or libfoo-dev. But I didn't want to make that change here because it would break downstream recipes. The changes in this PR are backwards compatible.

hmaarrfk · 2024-02-07T23:15:51Z

ok great. well i think that we considered things to death at this point! ^_^

All good on my side.

carterbox · 2024-02-08T02:00:30Z

Thanks!

REF: Use soname package for Windows too

587d792

carterbox added 3 commits January 1, 2024 18:31

Bump the build

c88defa

MNT: Re-rendered with conda-build 3.27.0, conda-smithy 3.30.2, and co…

3064fbc

…nda-forge-pinning 2023.12.31.22.41.20

REF: Use install script instead of clunky file globs

c2c8b6b

carterbox force-pushed the soname-package-windows branch from 61732d5 to c2c8b6b Compare January 5, 2024 05:17

carterbox added 4 commits January 5, 2024 11:15

REF: Update install script to be more general

e360965

Only copy files, not directories

977b281

Add license to install script

c310d7b

Update installer script

064ecb2

carterbox marked this pull request as ready for review February 5, 2024 23:19

carterbox requested a review from hmaarrfk as a code owner February 5, 2024 23:19

conda-forge-webservices[bot] and others added 4 commits February 5, 2024 23:21

MNT: Re-rendered with conda-build 3.28.4, conda-smithy 3.30.4, and co…

9400999

…nda-forge-pinning 2024.02.05.13.52.48

Rebuild for aom 3.8

4b3f065

MNT: Re-rendered with conda-build 3.28.4, conda-smithy 3.30.4, and co…

b35a204

…nda-forge-pinning 2024.01.31.01.24.54

MNT: Re-rendered with conda-build 3.28.3, conda-smithy 3.30.4, and co…

7afec9b

…nda-forge-pinning 2024.02.05.13.52.48

Merge branch 'main' into soname-package-windows

b524803

hmaarrfk reviewed Feb 7, 2024

View reviewed changes

carterbox added 3 commits February 7, 2024 11:49

DOC: Add note about copying links

25bb1e1

REF: Use simpler package name matching

b8e2832

REF: Explicitly state that files and symlinks are copied

ab5dacc

REF: Use pathlib.glob instead of glob.glob

10a5a95

carterbox requested a review from hmaarrfk February 7, 2024 18:59

carterbox merged commit 5f538ad into conda-forge:main Feb 8, 2024

traversaro mentioned this pull request May 26, 2024

Missing migrations for libavif pinned in conda-forge pinnings? #32

Closed

	if re.match(f"^lib{ basename }-static$", PKG_NAME):
	if "lib{ basename }-static" == PKG_NAME:

	if re.match(r"^.+\-split$", PKG_NAME):
	if PKG_NAME.endswith("-split"):

-        if match.exists():
+        if match.is_dir():
+            continue
+        if not match.exists():
+            # runtime error? or just continue?

Uh oh!

Uh oh!

Conversation

carterbox commented Jan 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conda-forge-webservices Bot commented Jan 2, 2024

Uh oh!

carterbox commented Feb 5, 2024

Uh oh!

conda-forge-webservices Bot commented Feb 5, 2024

Uh oh!

conda-forge-webservices Bot commented Feb 5, 2024

Uh oh!

hmaarrfk commented Feb 6, 2024

Uh oh!

carterbox commented Feb 6, 2024

Uh oh!

hmaarrfk Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

carterbox Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carterbox Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

carterbox Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

hmaarrfk Feb 7, 2024

Choose a reason for hiding this comment

Uh oh!

carterbox Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hmaarrfk commented Feb 7, 2024

Uh oh!

hmaarrfk commented Feb 7, 2024

Uh oh!

carterbox commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carterbox commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hmaarrfk commented Feb 7, 2024

Uh oh!

carterbox commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

carterbox commented Jan 2, 2024 •

edited

Loading

hmaarrfk Feb 7, 2024 •

edited

Loading

carterbox Feb 7, 2024 •

edited

Loading

carterbox commented Feb 7, 2024 •

edited

Loading

carterbox commented Feb 7, 2024 •

edited

Loading