* [bug#56770] [PATCH] gnu: Add grimshot.
2022-07-25 20:54 [bug#56770] [PATCH] gnu: Add grimshot Antero Mejr via Guix-patches via
@ 2022-07-26 15:46 ` Maxime Devos
2022-07-26 15:52 ` Maxime Devos
2022-07-26 15:49 ` Maxime Devos
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 15:46 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* [bug#56770] [PATCH] gnu: Add grimshot.
2022-07-26 15:46 ` Maxime Devos
@ 2022-07-26 15:52 ` Maxime Devos
0 siblings, 0 replies; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 15:52 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#56770] [PATCH] gnu: Add grimshot.
2022-07-25 20:54 [bug#56770] [PATCH] gnu: Add grimshot Antero Mejr via Guix-patches via
2022-07-26 15:46 ` Maxime Devos
@ 2022-07-26 15:49 ` Maxime Devos
2022-07-26 17:58 ` [bug#56770] [PATCH v2] " Antero Mejr via Guix-patches via
2022-07-26 20:48 ` [bug#56770] [PATCH v3] " Antero Mejr via Guix-patches via
3 siblings, 0 replies; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 15:49 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#56770] [PATCH v2] gnu: Add grimshot.
2022-07-25 20:54 [bug#56770] [PATCH] gnu: Add grimshot Antero Mejr via Guix-patches via
2022-07-26 15:46 ` Maxime Devos
2022-07-26 15:49 ` Maxime Devos
@ 2022-07-26 17:58 ` Antero Mejr via Guix-patches via
2022-07-26 18:17 ` Maxime Devos
2022-07-26 18:29 ` Maxime Devos
2022-07-26 20:48 ` [bug#56770] [PATCH v3] " Antero Mejr via Guix-patches via
3 siblings, 2 replies; 10+ messages in thread
From: Antero Mejr via Guix-patches via @ 2022-07-26 17:58 UTC (permalink / raw)
To: 56770; +Cc: Antero Mejr, maximedevos
* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [bug#56770] [PATCH v2] gnu: Add grimshot.
2022-07-26 17:58 ` [bug#56770] [PATCH v2] " Antero Mejr via Guix-patches via
@ 2022-07-26 18:17 ` Maxime Devos
2022-07-26 18:29 ` Maxime Devos
1 sibling, 0 replies; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 18:17 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* [bug#56770] [PATCH v2] gnu: Add grimshot.
2022-07-26 17:58 ` [bug#56770] [PATCH v2] " Antero Mejr via Guix-patches via
2022-07-26 18:17 ` Maxime Devos
@ 2022-07-26 18:29 ` Maxime Devos
1 sibling, 0 replies; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 18:29 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug#56770] [PATCH v3] gnu: Add grimshot.
2022-07-25 20:54 [bug#56770] [PATCH] gnu: Add grimshot Antero Mejr via Guix-patches via
` (2 preceding siblings ...)
2022-07-26 17:58 ` [bug#56770] [PATCH v2] " Antero Mejr via Guix-patches via
@ 2022-07-26 20:48 ` Antero Mejr via Guix-patches via
2022-07-26 22:06 ` [bug#56770] LGTM: [PATCH v3] gnu: Add grimshot. (LGTM) Maxime Devos
3 siblings, 1 reply; 10+ messages in thread
From: Antero Mejr via Guix-patches via @ 2022-07-26 20:48 UTC (permalink / raw)
To: 56770; +Cc: Antero Mejr, maximedevos
* 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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [bug#56770] LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)
2022-07-26 20:48 ` [bug#56770] [PATCH v3] " Antero Mejr via Guix-patches via
@ 2022-07-26 22:06 ` Maxime Devos
2022-07-27 18:42 ` bug#56770: " Liliana Marie Prikler
0 siblings, 1 reply; 10+ messages in thread
From: Maxime Devos @ 2022-07-26 22:06 UTC (permalink / raw)
To: Antero Mejr, 56770
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* bug#56770: LGTM: [PATCH v3] gnu: Add grimshot. (LGTM)
2022-07-26 22:06 ` [bug#56770] LGTM: [PATCH v3] gnu: Add grimshot. (LGTM) Maxime Devos
@ 2022-07-27 18:42 ` Liliana Marie Prikler
0 siblings, 0 replies; 10+ messages in thread
From: Liliana Marie Prikler @ 2022-07-27 18:42 UTC (permalink / raw)
To: Maxime Devos, Antero Mejr, 56770-done
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
^ permalink raw reply [flat|nested] 10+ messages in thread