all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Leo Famulari <leo@famulari.name>
To: Pierre Neidhardt <ambrevar@gmail.com>
Cc: 31017@debbugs.gnu.org
Subject: [bug#31017] [PATCH] gnu: Add subdl
Date: Mon, 2 Apr 2018 17:02:29 -0400	[thread overview]
Message-ID: <20180402210229.GC3852@jasmine.lan> (raw)
In-Reply-To: <20180401151502.26423-1-ambrevar@gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 2358 bytes --]

On Sun, Apr 01, 2018 at 08:45:02PM +0530, Pierre Neidhardt wrote:
> * gnu/packages/video.scm (subdl): New variable.

Thanks!

> +(define-public subdl
> +  (let
> +      ((commit "4cf5789b11f0ff3f863b704b336190bf968cd471")
> +       (revision "1"))

It's okay to put 'commit' on the same line as 'let' :)

> +    (package
> +      (name "subdl")
> +      (version (string-append "1.0.3-" revision "." (string-take commit 7)))

Here we can use the git-version procedure and forget about how many
characters of the Git hash we are supposed to use:

(version (git-version "1.0.3" revision commit))

I was curious where the "1.0.3" comes from, since the upstream repo
lacks any version tags, but I found it in the subdl source code.

> +      (source (origin
> +                (method git-fetch)

And likewise in here, one should use git-file-name so that the source
archive is named descriptively:

(file-name (git-file-name name version))

> +      (build-system trivial-build-system)
> +      (arguments
> +       `(#:modules ((guix build utils))
> +         #:builder (begin
> +                     (use-modules (guix build utils))
> +                     (let* ((out (assoc-ref %outputs "out"))
> +                            (bin (string-append out "/bin"))
> +                            (source (assoc-ref %build-inputs "source"))
> +                            (python (assoc-ref %build-inputs "python")))
> +                       (mkdir-p bin)
> +                       (with-directory-excursion bin
> +                         (copy-file (string-append source "/subdl") "subdl")
> +                         (patch-shebang "subdl"
> +                                        (list (string-append python "/bin")))
> +                         (chmod "subdl" #o555))))))

You asked about this chmod in your followup message. In this case it's
unnecessary since the file is installed #o555 regardless. And we can
even simplify the code to this:

(install-file (string-append source "/subdl") bin) 
(patch-shebang (string-append bin "/subdl")
               (list (string-append python "/bin")))

The (install-file) procedure conveniently combines (mkdir-p) and
(copy-file).

I can commit with the changes in the attached diff, but which name and
email should I use for your copyright statement?

[-- Attachment #1.2: diff --]
[-- Type: text/plain, Size: 2922 bytes --]

diff --git a/gnu/packages/video.scm b/gnu/packages/video.scm
index fe1badee9..6ccced8fa 100644
--- a/gnu/packages/video.scm
+++ b/gnu/packages/video.scm
@@ -2824,18 +2824,20 @@ changed.  Or in other words, it can detect motion.")
     (license license:gpl2)))
 
 (define-public subdl
-  (let
-      ((commit "4cf5789b11f0ff3f863b704b336190bf968cd471")
-       (revision "1"))
+  (let ((commit "4cf5789b11f0ff3f863b704b336190bf968cd471")
+        (revision "1"))
     (package
       (name "subdl")
-      (version (string-append "1.0.3-" revision "." (string-take commit 7)))
+      (version (git-version "1.0.3" revision commit))
       (source (origin
                 (method git-fetch)
                 (uri (git-reference
                       (url "https://github.com/alexanderwink/subdl.git")
                       (commit commit)))
-                (sha256 (base32 "0kmk5ck1j49q4ww0lvas2767kwnzhkq0vdwkmjypdx5zkxz73fn8"))))
+                (file-name (git-file-name name version))
+                (sha256
+                 (base32
+                  "0kmk5ck1j49q4ww0lvas2767kwnzhkq0vdwkmjypdx5zkxz73fn8"))))
       (build-system trivial-build-system)
       (arguments
        `(#:modules ((guix build utils))
@@ -2845,18 +2847,14 @@ changed.  Or in other words, it can detect motion.")
                             (bin (string-append out "/bin"))
                             (source (assoc-ref %build-inputs "source"))
                             (python (assoc-ref %build-inputs "python")))
-                       (mkdir-p bin)
-                       (with-directory-excursion bin
-                         (copy-file (string-append source "/subdl") "subdl")
-                         (patch-shebang "subdl"
-                                        (list (string-append python "/bin")))
-                         (chmod "subdl" #o555))))))
+                       (install-file (string-append source "/subdl") bin)
+                       (patch-shebang (string-append bin "/subdl")
+                                      (list (string-append python "/bin")))))))
       (inputs `(("python" ,python)))
       (synopsis "Command-line tool for downloading subtitles from opensubtitles.org")
-      (description
-       "Subdl is a command-line tool for downloading subtitles from opensubtitles.org.
-By default, it will search for English subtitles, display the results,
-download the highest-rated result in the requested language and save it to the
-appropriate filename.")
+      (description "Subdl is a command-line tool for downloading subtitles from
+opensubtitles.org.  By default, it will search for English subtitles, display
+the results, download the highest-rated result in the requested language and
+save it to the appropriate filename.")
       (license license:gpl3+)
       (home-page "https://github.com/alexanderwink/subdl"))))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  parent reply	other threads:[~2018-04-02 21:03 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-01 15:15 [bug#31017] [PATCH] gnu: Add subdl Pierre Neidhardt
2018-04-01 15:24 ` Peter Neidhardt
2018-04-02 21:02 ` Leo Famulari [this message]
2018-04-03  4:19   ` Pierre Neidhardt
2018-04-04 11:43     ` Ludovic Courtès
2018-04-05  6:30     ` bug#31017: " Leo Famulari

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180402210229.GC3852@jasmine.lan \
    --to=leo@famulari.name \
    --cc=31017@debbugs.gnu.org \
    --cc=ambrevar@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.