Subject: Re: [PATCH 0/5] gnu: yt-dlp: Update to 2023.12.30

From: Ian Eure <[email-masked]>
Date: Tue, 12 Mar 2024 16:23:30 -0700

Hi, thanks for doing this.

Few things:

I don’t think it makes sense to add version 2023.11.16 in one 
patch, then replace it with 2023.12.13 in the next.  I’d go 
straight to 2023.03.10, which was just released.

Do you know how many packages python-requests would require 
rebuilding?  Since it’s a minor version bump, I’d expect it 
wouldn’t break builds, just require a lot of rebuilds, since it’s 
a fairly popular library.  If possible, I think we should update 
and rebuild instead of adding a second version.

It’s not clear to me why urllib3 2.x is getting added.  The 
requirements for 2023.12.13[1] say that 1.26.17 or newer is 
acceptable; 1.26.15 is in Guix presently.  It seems inadvisable to 
use a library with a major version greater than yt-dlp expects.  I 
think it’d be simpler to update to 1.26.18 (the latest 1.x 
release) and not maintain two versions.  It looks like this was 
last updated around a year ago, in commit 
3e8d608dac2eb9f387431bd8976f78de9ea7a701, and I don’t see anything 
special about that, so that makes me think this would be a 
relatively safe course.

> -    (arguments
> -     ;; FIXME: Some tests require network access.
> -     '(#:tests? #f))
> +    (arguments (list #:tests? #f))

> +(define-public python-requests
> +  (package
> +    (inherit python-requests-2.31)
> +    (name "python-requests")
> +    (version "2.28.1")
> +    (source (origin
> +             (method url-fetch)
> +             (uri (pypi-uri "requests" version))
> +             (sha256
> +              (base32
> + 
> "10vrr7bijzrypvms3g2sgz8vya7f9ymmcv423ikampgy0aqrjmbw"))))
> +    (build-system python-build-system) ; Tests require network 
> access.

Looks like this comment migrated away from the context it’s 
helpful for.

> @@ -3609,7 +3632,7 @@ (define-public python-urllib3
>           (base32
>            "01dkqv0rsjqyw4wrp6yj8h3bcnl7c678qkj845596vs7p4bqff4a"))))
>      (build-system python-build-system)
> -    (arguments `(#:tests? #f))
> +    (native-inputs '())

A comment on the empty (native-inputs) would be good, I think.

(I’m not a Guix committer, so I can’t actually move this forward / 
my thoughts are just my own.  I’d just like to see a yt-dlp 
update.)

Thanks,

  — Ian

[1]: 
https://github.com/yt-dlp/yt-dlp/blob/9f40cd289665b2fb8a05ccaf9721b3b2ca0f39c7/requirements.txt#L7