GNU bug report logs - #56770
[PATCH] gnu: Add grimshot.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 56770 in the body.
You can then email your comments to 56770 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: guix-patches <at> gnu.org
Cc: Antero Mejr <antero <at> mailbox.org>
Subject: [PATCH] gnu: Add grimshot.
Date: Mon, 25 Jul 2022 16:54:12 -0400

* gnu/packages/wm.scm (grimshot): New variable.
---
 gnu/packages/wm.scm | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/gnu/packages/wm.scm b/gnu/packages/wm.scm
index 8fef7de77b..9aad6c1c37 100644
--- a/gnu/packages/wm.scm
+++ b/gnu/packages/wm.scm
@@ -2742,3 +2742,38 @@ (define-public avizo
      "Avizo is a simple notification daemon for Sway, mainly intended to be
 used for multimedia keys.")
     (license license:gpl3+)))
+
+(define-public grimshot
+  (package
+    (inherit sway)
+    (name "grimshot")
+    (build-system trivial-build-system)
+    (arguments
+     (list
+      #:modules '((guix build utils))
+      #:builder
+      #~(begin
+          (use-modules (guix build utils))
+          (copy-recursively
+           (string-append #$(package-source this-package) "/contrib") ".")
+          (substitute* "grimshot"
+            (("date ") (string-append #$coreutils "/bin/date "))
+            (("jq ") (string-append #$jq "/bin/jq "))
+            (("swaymsg ") (string-append #$sway "/bin/swaymsg "))
+            (("notify-send ") (string-append #$libnotify "/bin/notify-send "))
+            (("grim ") (string-append #$grim "/bin/grim "))
+            (("slurp ") (string-append #$slurp "/bin/slurp "))
+            (("wl-copy ") (string-append #$wl-clipboard "/bin/wl-copy ")))
+          (delete-file "grimshot.1")
+          (system
+           (string-append #$scdoc "/bin/scdoc < grimshot.1.scd > grimshot.1"))
+          (install-file "grimshot" (string-append #$output "/bin"))
+          (install-file "grimshot.1"
+                        (string-append #$output "/usr/share/man/man1")))))
+    (native-inputs (list scdoc))
+    (inputs (list coreutils grim jq libnotify sway wl-clipboard))
+    (synopsis "Screenshot utility for the Sway window manager")
+    (description "Grimshot is a screenshot utility for @code{sway}.  It provides
+an interface over @code{grim}, @code{slurp} and @code{jq}, and supports storing
+the screenshot either directly to the clipboard using @code{wl-copy} or to a
+file.")))
-- 
2.37.0

Message #8 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: Re: [bug#56770] [PATCH] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 17:46:20 +0200

[Message part 1 (text/plain, inline)]

> +          (copy-recursively
> +           (string-append #$(package-source this-package) "/contrib") ".")
I would have expected trivial-build-system to automatically unpack the 
source, so maybe this can be simplified to (chdir "/contrib") (untested!)

On 25-07-2022 22:54, Antero Mejr via Guix-patches via wrote:
> +          (system
> +           (string-append #$scdoc "/bin/scdoc < grimshot.1.scd > grimshot.1"))

This ignores any errors coming from 'system'. Try (invoke ...) + 
with-input-from-file + with-output-to-file instead, which will report 
errors as exceptions:

(with-input-from-file "grimshot.1.scd"
  (lambda ()
    (with-output-to-file "grimshot.1"
      (lambda ()
        (invoke #+(file-append (this-package-native-input "scdoc") 
"/bin/scdoc")))))

Here it is important to use #+ instead of #$ for cross-compilation.  I 
use this-package-native-input here to make the --with-input 
transformation work.

> +          (substitute* "grimshot"
> +            (("date ") (string-append #$coreutils "/bin/date "))
> +            (("jq ") (string-append #$jq "/bin/jq "))
> +            (("swaymsg ") (string-append #$sway "/bin/swaymsg "))
> +            (("notify-send ") (string-append #$libnotify "/bin/notify-send "))
> +            (("grim ") (string-append #$grim "/bin/grim "))
> +            (("slurp ") (string-append #$slurp "/bin/slurp "))
> +            (("wl-copy ") (string-append #$wl-clipboard "/bin/wl-copy ")))
Likewise, you should use this-package-input here (but unlike the 
previous case, not this-package-native-input).

(I only looked at the package definition, not the underlying source 
code, and did not test it)

Greetings,
Maxime

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #11 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: Re: [bug#56770] [PATCH] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 17:49:29 +0200

[Message part 1 (text/plain, inline)]

On 25-07-2022 22:54, Antero Mejr via Guix-patches via wrote:
> +          (delete-file "grimshot.1")

Why is it deleted?  If it's to build the man page from source instead of 
copying the pre-made binary, I think a snippet in the origin would be a 
slightly better fit, to clean up the result of "guix build --source" a 
bit, though it doesn't matter much I suppose.

Greetings,
Maxime.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #14 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: Re: [bug#56770] [PATCH] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 17:52:00 +0200

[Message part 1 (text/plain, inline)]

On 26-07-2022 17:46, Maxime Devos wrote:
>
>> + (copy-recursively
>> +           (string-append #$(package-source this-package) 
>> "/contrib") ".")
> I would have expected trivial-build-system to automatically unpack the 
> source, so maybe this can be simplified to (chdir "/contrib") (untested!) 

Or even simpler: remove this line, and add (source (file-append 
(package-source sway) "/contrib")) as a field.

A quick  "git grep -F '(source (file-append" doesn't result in any 
packages already doing that, but it seems a perfect fit here to me ...  
OTOH, this probably interferes with --with-git-url and such, so I'd 
guess better not.

Greetings,
Maxime.

[Message part 2 (text/html, inline)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #17 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: 56770 <at> debbugs.gnu.org
Cc: Antero Mejr <antero <at> mailbox.org>, maximedevos <at> telenet.be
Subject: [PATCH v2] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 13:58:17 -0400

* gnu/packages/wm.scm (grimshot): New variable.
---
changes for v2:
1. using copy-build-system instead of trivial-build-system because it is
simpler, copy-build-system handles unpacking the source
2. using snippet to delete the precompiled grimshot.1 (from review)
3. using this-package-input when substituting the script (from review)
4. using invoke to build man page (from review)
5. put inputs on separate lines (from guix style)

Maxime, this code: (source (file-append (package-source sway) "/contrib"))
didn't work for me, but inheriting the package source worked
with --with-git-url.

 gnu/packages/wm.scm | 65 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/gnu/packages/wm.scm b/gnu/packages/wm.scm
index 8fef7de77b..1e60ceb27b 100644
--- a/gnu/packages/wm.scm
+++ b/gnu/packages/wm.scm
@@ -2742,3 +2742,68 @@ (define-public avizo
      "Avizo is a simple notification daemon for Sway, mainly intended to be
 used for multimedia keys.")
     (license license:gpl3+)))
+
+(define-public grimshot
+  (package
+    (inherit sway)
+    (name "grimshot")
+    (source (origin
+              (inherit (package-source sway))
+              (snippet #~(begin
+                           (delete-file "contrib/grimshot.1")))))
+    (build-system copy-build-system)
+    (arguments
+     (list #:install-plan #~`(("grimshot" "bin/")
+                              ("grimshot.1" "usr/share/man/man1/"))
+           #:phases #~(modify-phases %standard-phases
+                        (add-after 'unpack 'chdir
+                          (lambda _
+                            (chdir "contrib")))
+                        (add-after 'chdir 'patch-script-deps
+                          (lambda _
+                            (substitute* "grimshot"
+                              (("date ")
+                               (string-append #$(this-package-input "coreutils")
+                                              "/bin/date "))
+                              (("jq ")
+                               (string-append #$(this-package-input "jq")
+                                              "/bin/jq "))
+                              (("swaymsg ")
+                               (string-append #$(this-package-input "sway")
+                                              "/bin/swaymsg "))
+                              (("notify-send ")
+                               (string-append #$(this-package-input "libnotify")
+                                              "/bin/notify-send "))
+                              (("grim ")
+                               (string-append #$(this-package-input "grim")
+                                              "/bin/grim "))
+                              (("slurp ")
+                               (string-append #$(this-package-input "slurp")
+                                              "/bin/slurp "))
+                              (("wl-copy ")
+                               (string-append
+                                #$(this-package-input "wl-clipboard")
+                                "/bin/wl-copy ")))))
+                        (add-after 'patch-script-deps 'build-man-page
+                          (lambda _
+                            (with-input-from-file "grimshot.1.scd"
+                              (lambda _
+                                (with-output-to-file "grimshot.1"
+                                  (lambda _
+                                    (invoke #+(file-append
+                                               (this-package-native-input
+                                                "scdoc")
+                                               "/bin/scdoc")))))))))))
+    (native-inputs (list scdoc))
+    (inputs (list coreutils
+                  grim
+                  jq
+                  libnotify
+                  slurp
+                  sway
+                  wl-clipboard))
+    (synopsis "Screenshot utility for the Sway window manager")
+    (description "Grimshot is a screenshot utility for @code{sway}.  It provides
+an interface over @code{grim}, @code{slurp} and @code{jq}, and supports storing
+the screenshot either directly to the clipboard using @code{wl-copy} or to a
+file.")))
-- 
2.37.0

Message #20 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: Re: [PATCH v2] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 20:17:33 +0200

[Message part 1 (text/plain, inline)]

On 26-07-2022 19:58, Antero Mejr wrote:
> Maxime, this code: (source (file-append (package-source sway) "/contrib"))
> didn't work for me, but inheriting the package source worked
> with --with-git-url.

It works for me, see attachment (*). However, I've also tested out 
--with-git-url, and it appears that --with-git-url does work when using 
file-append, but the 'file-append' is removed, so I still don't think we 
should go for 'file-append' (unless you want to change --with-git-url to 
support file-append first).

(*) not properly styled, as "guix style" only supports packages, not files.

(Also, I'll write some other comments on the v2)

Greetings,
Maxime.

[foo.scm (text/x-scheme, attachment)]

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #23 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: Re: [PATCH v2] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 20:29:53 +0200

[Message part 1 (text/plain, inline)]

On 26-07-2022 19:58, Antero Mejr wrote:
> * gnu/packages/wm.scm (grimshot): New variable.
> ---
> changes for v2:
> 1. using copy-build-system instead of trivial-build-system because it is
> simpler, copy-build-system handles unpacking the source
Now that you are using copy-build-system, using phases, the 
'this-package-input' can be changed to something more robust ...
> +(define-public grimshot
> +  (package
> +    (inherit sway)
> +    (name "grimshot")
> +    (source (origin
> +              (inherit (package-source sway))
> +              (snippet #~(begin
> +                           (delete-file "contrib/grimshot.1")))))
Some people have a preference for writing #~(begin [a single 
invocation]) anyway, but I'd like to note that wrapping the delete-file 
in a (begin ...) is not technically required.
> +    (build-system copy-build-system)
> +    (arguments
> +     (list #:install-plan #~`(("grimshot" "bin/")
> +                              ("grimshot.1" "usr/share/man/man1/"))
> +           #:phases #~(modify-phases %standard-phases
> +                        (add-after 'unpack 'chdir
> +                          (lambda _
> +                            (chdir "contrib")))
> +                        (add-after 'chdir 'patch-script-deps
No need to abbreviate dependencies -> deps, not that it matters much I 
suppose.
> +                          (lambda _
> +                            (substitute* "grimshot"
> +                              (("date ")
> +                               (string-append #$(this-package-input "coreutils")
> +                                              "/bin/date "))

Now that this is written as a phase, it becomes possible to use 
'search-input-file', like this:

> (lambda* (#:key inputs #:allow-other-keys)
>   (substitute*
>     (("\\b(date|jq|swaymsg|...)\\b" _ binary)
>      (search-input-file inputs (string-append "bin/" binary)))))
>
This way, you are not referring to input labels anymore, which has as 
benefit that package transformations become more robust. E.g., if you do 
it this way instead of using input labels (which this-package-input) 
does, it becomes possible to do things like 
--with-input=coreutils=busybox or the Scheme equivalent.
> +                        (add-after 'patch-script-deps 'build-man-page
> +                          (lambda _
> +                            (with-input-from-file "grimshot.1.scd"
> +                              (lambda _
> +                                (with-output-to-file "grimshot.1"
> +                                  (lambda _
> +                                    (invoke #+(file-append
> +                                               (this-package-native-input
> +                                                "scdoc")
> +                                               "/bin/scdoc")))))))))))

'invoke' does not need the absolute file name, so you the #+(file-append 
...) can be simplified to just "scdoc": [all the surrounding 
with-input/output-...+lambda (invoke "scdoc")], 'invoke' will 
automatically figure out the absolute file name.

Greetings,
Maxime.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #26 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Antero Mejr <antero <at> mailbox.org>
To: 56770 <at> debbugs.gnu.org
Cc: Antero Mejr <antero <at> mailbox.org>, maximedevos <at> telenet.be
Subject: [PATCH v3] gnu: Add grimshot.
Date: Tue, 26 Jul 2022 16:48:58 -0400

* gnu/packages/wm.scm (grimshot): New variable.
---
 gnu/packages/wm.scm | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/gnu/packages/wm.scm b/gnu/packages/wm.scm
index 8fef7de77b..aa5aa8d1ec 100644
--- a/gnu/packages/wm.scm
+++ b/gnu/packages/wm.scm
@@ -2742,3 +2742,46 @@ (define-public avizo
      "Avizo is a simple notification daemon for Sway, mainly intended to be
 used for multimedia keys.")
     (license license:gpl3+)))
+
+(define-public grimshot
+  (package
+    (inherit sway)
+    (name "grimshot")
+    (source (origin
+              (inherit (package-source sway))
+              (snippet #~(delete-file "contrib/grimshot.1"))))
+    (build-system copy-build-system)
+    (arguments
+     (list #:install-plan #~`(("grimshot" "bin/")
+                              ("grimshot.1" "usr/share/man/man1/"))
+           #:phases #~(modify-phases %standard-phases
+                        (add-after 'unpack 'chdir
+                          (lambda _
+                            (chdir "contrib")))
+                        (add-after 'chdir 'patch-script-dependencies
+                          (lambda* (#:key inputs #:allow-other-keys)
+                            (substitute* "grimshot"
+                              (("\\b(date|grim|jq|notify-send|slurp|swaymsg|wl-copy)\\b"
+                                _ binary)
+                               (search-input-file
+                                inputs (string-append "bin/" binary))))))
+                        (add-after 'patch-script-dependencies 'build-man-page
+                          (lambda _
+                            (with-input-from-file "grimshot.1.scd"
+                              (lambda _
+                                (with-output-to-file "grimshot.1"
+                                  (lambda _
+                                    (invoke "scdoc"))))))))))
+    (native-inputs (list scdoc))
+    (inputs (list coreutils
+                  grim
+                  jq
+                  libnotify
+                  slurp
+                  sway
+                  wl-clipboard))
+    (synopsis "Screenshot utility for the Sway window manager")
+    (description "Grimshot is a screenshot utility for @code{sway}.  It provides
+an interface over @code{grim}, @code{slurp} and @code{jq}, and supports storing
+the screenshot either directly to the clipboard using @code{wl-copy} or to a
+file.")))
-- 
2.37.0

Message #29 received at 56770 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Antero Mejr <antero <at> mailbox.org>, 56770 <at> debbugs.gnu.org
Subject: LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)
Date: Wed, 27 Jul 2022 00:06:02 +0200

[Message part 1 (text/plain, inline)]

On 26-07-2022 22:48, Antero Mejr wrote:

> +(define-public grimshot
> +  (package
> +    (inherit sway)
> +    (name "grimshot")
> +    (source (origin
> +              (inherit (package-source sway))
> +              (snippet #~(delete-file "contrib/grimshot.1"))))
> +    (build-system copy-build-system)
> +    (arguments
> +     (list #:install-plan #~`(("grimshot" "bin/")
> +                              ("grimshot.1" "usr/share/man/man1/"))
> +           #:phases #~(modify-phases %standard-phases
> +                        (add-after 'unpack 'chdir
> +                          (lambda _
> +                            (chdir "contrib")))
> +                        (add-after 'chdir 'patch-script-dependencies
> +                          (lambda* (#:key inputs #:allow-other-keys)
> +                            (substitute* "grimshot"
> +                              (("\\b(date|grim|jq|notify-send|slurp|swaymsg|wl-copy)\\b"
> +                                _ binary)
> +                               (search-input-file
> +                                inputs (string-append "bin/" binary))))))
> +                        (add-after 'patch-script-dependencies 'build-man-page
> +                          (lambda _
> +                            (with-input-from-file "grimshot.1.scd"
> +                              (lambda _
> +                                (with-output-to-file "grimshot.1"
> +                                  (lambda _
> +                                    (invoke "scdoc"))))))))))
> +    (native-inputs (list scdoc))
> +    (inputs (list coreutils
> +                  grim
> +                  jq
> +                  libnotify
> +                  slurp
> +                  sway
> +                  wl-clipboard))
> +    (synopsis "Screenshot utility for the Sway window manager")
> +    (description "Grimshot is a screenshot utility for @code{sway}.  It provides
> +an interface over @code{grim}, @code{slurp} and @code{jq}, and supports storing
> +the screenshot either directly to the clipboard using @code{wl-copy} or to a
> +file.")))

That's what I had in mind, thanks.

LGTM, with the caveat that I only looked at the package definition 
during reviewing.

To be clear, I only review things, someone else will have to commit this 
(assuming they agree).

(Also, trying out a new convention for indicating that a patch appears 
ready: prefix the subject with LGTM)

Greetings,
Maxime.

[OpenPGP_0x49E3EE22191725EE.asc (application/pgp-keys, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #34 received at 56770-done <at> debbugs.gnu.org (full text, mbox):

From: Liliana Marie Prikler <liliana.prikler <at> gmail.com>
To: Maxime Devos <maximedevos <at> telenet.be>, Antero Mejr <antero <at> mailbox.org>,
 56770-done <at> debbugs.gnu.org
Subject: Re: [bug#56770] LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)
Date: Wed, 27 Jul 2022 20:42:45 +0200

Am Mittwoch, dem 27.07.2022 um 00:06 +0200 schrieb Maxime Devos:
> LGTM, with the caveat that I only looked at the package definition 
> during reviewing.
Well, I also built the package, so together this must mean it's fine,
right?  Either way, I trust you, so I pushed it.

> (Also, trying out a new convention for indicating that a patch
> appears ready: prefix the subject with LGTM)
IMHO one LGTM per header suffices.  I think the prefix is better than
the suffix because the latter will likely be truncated.

Cheers

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.