[PATCH] gnu: youtube-dl: Add native-search-paths.

[PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

[-- Attachment #1: Type: text/plain, Size: 1176 bytes --]

Hi,

I notice youtube-dl comes with a man page, bash completion file and
fish completion file. However, since it is located in a very werid
path, the programs normally wouldn't find it. So, I add
native-search-paths.

I can only confirm that MANPATH is working correctly. I don't know if
fish_complete_path works. I am only following what the documentation
says.

BASH_COMPLETION doesn't work. I think it has to do with the bash
completion package. From the bash completion README
<https://anonscm.debian.org/cgit/bash-completion/bash-completion.git/plain/README>,
it says package X install the bash completion file into the path
return by `pkg-config --variable=completionsdir bash-completion' or
`pkg-config --variable=compatdir bash-completion'. Both commands
return `/gnu/store/jyj96nxaw415yqi8vg6r73i1yib34k7i-bash-completion-2.1/share/bash-completion/completions'.
I think it should be point to something like
`~/.guix-profile/etc/bash_completion.d' instead so that other packages
could install their own bash completion file into
`/etc/bash_completion.d'. For instance, git install its own bash
completion file into that directory by default.

Any ideas?

Cheers,
Alex

[-- Attachment #2: 0001-gnu-youtube-dl-Add-native-search-paths.patch --]
[-- Type: text/x-patch, Size: 4540 bytes --]

From 5b04230e30fd594ebab43c50e84f989503dc3535 Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1995@gmail.com>
Date: Sat, 12 Dec 2015 11:20:44 +0800
Subject: [PATCH] gnu: youtube-dl: Add native-search-paths.

* gnu/packages/video.scm (youtube-dl): Add native-search-paths.
[native-search-paths]: Add fish_complete_path and MANPATH.
---
 gnu/packages/video.scm | 73 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index 76374e2..e30c1ce 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -24,6 +24,7 @@
 
 (define-module (gnu packages video)
   #:use-module (ice-9 match)
+  #:use-module (srfi srfi-26)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix utils)
   #:use-module (guix packages)
@@ -808,25 +809,61 @@ projects while introducing many more.")
     (home-page "http://www.webmproject.org/")))
 
 (define-public youtube-dl
-  (package
-    (name "youtube-dl")
-    (version "2015.12.09")
-    (source (origin
-              (method url-fetch)
-              (uri (string-append "http://youtube-dl.org/downloads/"
-                                  version "/youtube-dl-"
-                                  version ".tar.gz"))
-              (sha256
-               (base32
-                "11rzb30ik4all43r7bnsnm35mvs37y7xj3g9r7ig9jr7qlbhllwk"))))
-    (build-system python-build-system)
-    (native-inputs `(("python-setuptools" ,python-setuptools)))
-    (home-page "http://youtube-dl.org")
-    (synopsis "Download videos from YouTube.com and other sites")
-    (description
-     "Youtube-dl is a small command-line program to download videos from
+  (let* ((python-version "3.4")
+         (yt-dl-year "2015")
+         (yt-dl-month "12")
+         (yt-dl-day "09")
+         (make-version-string (lambda (string-list)
+                                (string-join string-list ".")))
+         (make-zero-trimmed-version-string (lambda (string-list)
+                                             (make-version-string
+                                              (map (cut string-trim <> #\0)
+                                                   string-list))))
+         (with-base-dir (lambda (relative-path)
+                          (string-append "lib/python"
+                                         python-version
+                                         "/site-packages/youtube_dl-"
+                                         (make-zero-trimmed-version-string
+                                          (list yt-dl-year
+                                                yt-dl-month
+                                                yt-dl-day))
+                                         "-py"
+                                         python-version
+                                         ".egg/"
+                                         relative-path))))
+    (package
+      (name "youtube-dl")
+      (version
+       (make-version-string (list yt-dl-year
+                                  yt-dl-month
+                                  yt-dl-day)))
+      (source (origin
+                (method url-fetch)
+                (uri (string-append "http://youtube-dl.org/downloads/"
+                                    version "/youtube-dl-"
+                                    version ".tar.gz"))
+                (sha256
+                 (base32
+                  "11rzb30ik4all43r7bnsnm35mvs37y7xj3g9r7ig9jr7qlbhllwk"))))
+      (build-system python-build-system)
+      (native-inputs `(("python-setuptools" ,python-setuptools)))
+      (native-search-paths
+       (list (search-path-specification
+              (variable "fish_complete_path")
+              (files `(,(with-base-dir "etc/fish/completions"))))
+             ;; TODO: fix bash completion
+             ;;(search-path-specification
+             ;; (variable "BASH_COMPLETION") ;we need to fix it
+             ;; (files `(,(with-base-dir "etc/bash_completion.d"))))
+             (search-path-specification
+              (variable "MANPATH")
+              (files `(,(with-base-dir "share/man"))))))
+      (home-page "http://youtube-dl.org")
+      (synopsis "Download videos from YouTube.com and other sites")
+      (description
+       "Youtube-dl is a small command-line program to download videos from
 YouTube.com and a few more sites.")
-    (license license:public-domain)))
+      (license license:public-domain))))
 
 (define-public libbluray
   (package
-- 
2.6.3

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Ricardo Wurmus]

Alex Vong <alexvong1995@gmail.com> writes:

> I notice youtube-dl comes with a man page, bash completion file and
> fish completion file. However, since it is located in a very werid
> path, the programs normally wouldn't find it. So, I add
> native-search-paths.

Would it not be better to patch the build system such that the files are
installed in the expected locations?

I don’t like to have to needlessly set environment variables for
programmes to work.

~~ Ricardo

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

On 13/12/2015, Ricardo Wurmus <rekado@elephly.net> wrote:
>
> Alex Vong <alexvong1995@gmail.com> writes:
>
>> I notice youtube-dl comes with a man page, bash completion file and
>> fish completion file. However, since it is located in a very werid
>> path, the programs normally wouldn't find it. So, I add
>> native-search-paths.
>
> Would it not be better to patch the build system such that the files are
> installed in the expected locations?
>
> I don’t like to have to needlessly set environment variables for
> programmes to work.
>
> ~~ Ricardo
>
>

OK I would try to patch build system, but I am not very familiar with
custom build system like pip.

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

On 13/12/2015, Alex Vong <alexvong1995@gmail.com> wrote:
> On 13/12/2015, Ricardo Wurmus <rekado@elephly.net> wrote:
>>
>> Alex Vong <alexvong1995@gmail.com> writes:
>>
>>> I notice youtube-dl comes with a man page, bash completion file and
>>> fish completion file. However, since it is located in a very werid
>>> path, the programs normally wouldn't find it. So, I add
>>> native-search-paths.
>>
>> Would it not be better to patch the build system such that the files are
>> installed in the expected locations?
>>
>> I don’t like to have to needlessly set environment variables for
>> programmes to work.
>>
>> ~~ Ricardo
>>
>>
>
> OK I would try to patch build system, but I am not very familiar with
> custom build system like pip.
>
Actually, setuptools not pip is used in guix. I have tried a manual
install using pip. It works perfectly. Now I need to investigate why
setuptools does not work as expected.

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

From 72c15c31d56c0b0e75766869060aba9693fc074e Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1995@gmail.com>
Date: Sun, 13 Dec 2015 22:03:39 +0800
Subject: [PATCH] gnu: youtube-dl: Add phase to fix data_files paths so that
 man page and completion files are installed to the right place.

* gnu/packages/video.scm (youtube-dl) [arguments]: Add fix-data-files-paths
phase.
---
 gnu/packages/video.scm | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index 76374e2..730348c 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -822,6 +822,29 @@ projects while introducing many more.")
     (build-system python-build-system)
     (native-inputs `(("python-setuptools" ,python-setuptools)))
     (home-page "http://youtube-dl.org")
+    (arguments
+     ;; The problem here is that the data_files paths are relative. Normally,
+     ;; this is fine. However, for some reason, setup.py uses the auto-detected
+     ;; sys.prefix instead of the user-defined "--prefix=FOO". So, we need to
+     ;; get the user-defined prefix in python and patch the data_files paths.
+     `(#:phases (modify-phases %standard-phases
+                  (add-before 'install 'fix-data-files-paths
+                    (lambda* _
+                      (substitute* "setup.py"
+                        (("import sys")
+                         "import sys
+import re
+
+map_ = lambda proc, ls: list(map(proc, ls))
+find = lambda pred, ls: next(filter(pred, ls))
+prefix = find(lambda x: bool(x),
+              map_(lambda string: re.match(r'^--prefix=(.*)$', string),
+                   sys.argv)).group(1)
+")
+                        (("'etc/")
+                         "prefix + '/etc/")
+                        (("'share/")
+                         "prefix + '/share/")))))))
     (synopsis "Download videos from YouTube.com and other sites")
     (description
      "Youtube-dl is a small command-line program to download videos from
-- 
2.6.3

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Ludovic Courtès]

Alex Vong <alexvong1995@gmail.com> skribis:

> From 72c15c31d56c0b0e75766869060aba9693fc074e Mon Sep 17 00:00:00 2001
> From: Alex Vong <alexvong1995@gmail.com>
> Date: Sun, 13 Dec 2015 22:03:39 +0800
> Subject: [PATCH] gnu: youtube-dl: Add phase to fix data_files paths so that
>  man page and completion files are installed to the right place.

It’s enough to write:

  gnu: youtube-dl: Install man pages and completion files.

because apparently none of them is currently installed, right?

> +     ;; The problem here is that the data_files paths are relative. Normally,
> +     ;; this is fine. However, for some reason, setup.py uses the auto-detected
> +     ;; sys.prefix instead of the user-defined "--prefix=FOO". So, we need to
> +     ;; get the user-defined prefix in python and patch the data_files paths.

It seems that --prefix is at least partly honored, currently, because we
currently get binaries under bin/ and Python modules under lib/python/.

What about changing the comment to explicitly mention man pages and
completions?

> +     `(#:phases (modify-phases %standard-phases
> +                  (add-before 'install 'fix-data-files-paths
> +                    (lambda* _
> +                      (substitute* "setup.py"
> +                        (("import sys")
> +                         "import sys
> +import re
> +
> +map_ = lambda proc, ls: list(map(proc, ls))
> +find = lambda pred, ls: next(filter(pred, ls))
> +prefix = find(lambda x: bool(x),
> +              map_(lambda string: re.match(r'^--prefix=(.*)$', string),
> +                   sys.argv)).group(1)
> +")
> +                        (("'etc/")
> +                         "prefix + '/etc/")
> +                        (("'share/")
> +                         "prefix + '/share/")))))))

I think it’s simpler to omit the Python ‘prefix’ variable definition,
and so:

  (("'etc/")
   (string-append (assoc-ref outputs "out") "/etc"))

WDYT?

Note that bash-completion expects it to be under one of:

  share/bash-completion/completions/youtube-dl
  etc/bash_completion.d/youtube-dl

Thanks!

Ludo’.

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Andreas Enge]

On Sun, Dec 13, 2015 at 05:16:10AM +0100, Ricardo Wurmus wrote:
> Would it not be better to patch the build system such that the files are
> installed in the expected locations?

And maybe forward a patch upstream so that it works out of the box
with the next release.

Andreas

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

Andreas Enge <andreas@enge.fr> writes:

> On Sun, Dec 13, 2015 at 05:16:10AM +0100, Ricardo Wurmus wrote:
>> Would it not be better to patch the build system such that the files are
>> installed in the expected locations?
>
> And maybe forward a patch upstream so that it works out of the box
> with the next release.

I don't know if I know enough of python to patch it. It looks like a bug
of setuptools since pip install the data files properly.

>
> Andreas

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

From 21db93d3caa4846882720a610a7fa0e0a2fe3162 Mon Sep 17 00:00:00 2001
From: Alex Vong <alexvong1995@gmail.com>
Date: Mon, 14 Dec 2015 20:00:52 +0800
Subject: [PATCH] gnu: youtube-dl: Install the man page and completion files.

* gnu/packages/video.scm (youtube-dl) [arguments]: Add
fix-the-data-files-path phase.
---
 gnu/packages/video.scm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index 76374e2..154befd 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -822,6 +822,20 @@ projects while introducing many more.")
     (build-system python-build-system)
     (native-inputs `(("python-setuptools" ,python-setuptools)))
     (home-page "http://youtube-dl.org")
+    (arguments
+     ;; The problem here is that the path for the man page and completion
+     ;; files is relative. Normally, this is fine. However, for some reason,
+     ;; setup.py uses the auto-detected sys.prefix instead of the user-defined
+     ;; "--prefix=FOO". So, we need pass the prefix directly.
+     `(#:phases (modify-phases %standard-phases
+                  (add-before 'install 'fix-the-data-files-path
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (let ((prefix (assoc-ref outputs "out")))
+                        (substitute* "setup.py"
+                          (("'etc/")
+                           (string-append "'" prefix "/etc/"))
+                          (("'share/")
+                           (string-append "'" prefix "/share/")))))))))
     (synopsis "Download videos from YouTube.com and other sites")
     (description
      "Youtube-dl is a small command-line program to download videos from
-- 
2.6.3

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Ludovic Courtès]

Alex Vong <alexvong1995@gmail.com> skribis:

> From 21db93d3caa4846882720a610a7fa0e0a2fe3162 Mon Sep 17 00:00:00 2001
> From: Alex Vong <alexvong1995@gmail.com>
> Date: Mon, 14 Dec 2015 20:00:52 +0800
> Subject: [PATCH] gnu: youtube-dl: Install the man page and completion files.
>
> * gnu/packages/video.scm (youtube-dl) [arguments]: Add
> fix-the-data-files-path phase.

Applied, thanks.  While at it, I made few changes: added a copyright
line for you, changed “path” to “directory” (GNU has the convention that
“path” means “search path”, not file/directory), and rename the Bash
completion file so that it is picked up.

Let me know what you think.

Thanks!

Ludo’.

Re: [PATCH] gnu: youtube-dl: Add native-search-paths.

[Alex Vong]

ludo@gnu.org (Ludovic Courtès) writes:

> Alex Vong <alexvong1995@gmail.com> skribis:
>
>> From 21db93d3caa4846882720a610a7fa0e0a2fe3162 Mon Sep 17 00:00:00 2001
>> From: Alex Vong <alexvong1995@gmail.com>
>> Date: Mon, 14 Dec 2015 20:00:52 +0800
>> Subject: [PATCH] gnu: youtube-dl: Install the man page and completion files.
>>
>> * gnu/packages/video.scm (youtube-dl) [arguments]: Add
>> fix-the-data-files-path phase.
>
> Applied, thanks.  While at it, I made few changes: added a copyright
> line for you, changed “path” to “directory” (GNU has the convention that
> “path” means “search path”, not file/directory), and rename the Bash
> completion file so that it is picked up.
>
Thanks too!

So the bash completion file has the wrong name, should we report it to
upstream? Is bash picky about the completion file name or is it just a
matter of style? I cannot test it since I am not using guixsd.

Currently, I need to put the following to my ".bashrc" to make bash
completion works:

if test "x$BASH" != "x"
then
    for bash_completion_file in \
        $(find -L \
               "/usr/local/etc/bash_completion.d" \
               "$HOME/.guix-profile/etc/bash_completion.d" \
               "$HOME/.guix-profile/share/bash-completion/completions" \
               -type f)
    do
        . "$bash_completion_file"
    done
fi

But this makes bash slower to start. Should we put some instruction on
the manual?

> Let me know what you think.
>
> Thanks!
>
> Ludo’.

Cheers,
Alex