[bug#56770] [PATCH] gnu: Add grimshot.

[bug#56770] [PATCH] gnu: Add grimshot.

[Antero Mejr via Guix-patches via]

* 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

[bug#56770] [PATCH] gnu: Add grimshot.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1744 bytes --]


> +          (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


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#56770] [PATCH] gnu: Add grimshot.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 402 bytes --]


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.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#56770] [PATCH] gnu: Add grimshot.

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 696 bytes --]


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.


[-- Attachment #1.1.1.2: Type: text/html, Size: 1253 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#56770] [PATCH v2] gnu: Add grimshot.

[Antero Mejr via Guix-patches via]

* 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

[bug#56770] [PATCH v2] gnu: Add grimshot.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 689 bytes --]


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.


[-- Attachment #1.1.2: foo.scm --]
[-- Type: text/x-scheme, Size: 3273 bytes --]

(use-modules (gnu packages wm) (guix packages) (guix gexp) (guix build-system copy) (gnu packages base) (gnu packages image) (gnu packages web) (gnu packages gnome) (gnu packages xdisorg) (gnu packages man))
(define-public grimshot
  (package
    (inherit sway)
    (name "grimshot")
    (source (file-append (origin
              (inherit (package-source sway))
              (snippet #~(begin
                           (delete-file "contrib/grimshot.1")))) "/contrib"))
    (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 '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.")))
grimshot

[-- Attachment #1.1.3: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#56770] [PATCH v2] gnu: Add grimshot.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 3187 bytes --]


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.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#56770] [PATCH v3] gnu: Add grimshot.

[Antero Mejr via Guix-patches via]

* 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

[bug#56770] LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2504 bytes --]

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.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56770: LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)

[Liliana Marie Prikler]

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