On Fri, Apr 24, 2020 at 07:20:23PM +1200, Boris A. Dekshteyn wrote:
Toggle quote (4 lines)
> ---> gnu/packages/xdisorg.scm | 38 ++++++++++++++++++++++++++++++++++++++> 1 file changed, 38 insertions(+)
In your future patches, please include a ChangeLog-style line in thebody of the commit message. You can look at the commit history for someexamples. In the case of new packages, it's customary to write
* gnu/packages/xdisorg.scm (kbdd): New variable.
Toggle quote (14 lines)
> +(define-public kbdd> + (package> + (name "kbdd")> + (version "0.7.1")> + (source> + (origin> + (method url-fetch)> + (uri (string-append> + "https://github.com/qnikst/kbdd/archive/v"> + version ".tar.gz"))> + (sha256> + (base32 "0nhn7cx1z4k1kfll325xjr5a31zjc4h5h8q0wxa9svz8aihfxcqp"))> + (file-name (string-append "kbdd" version))))
It's generally a bad idea to use GitHub's autogenerated tarball, sinceit is occasionally regenerated, which changes the hash. See point 13 of"(guix)Submitting Patches". This is also pointed out by `guix lint`,please make sure to run it on your packages.
Toggle quote (10 lines)
> + (build-system gnu-build-system)> + (arguments> + '(#:phases> + (modify-phases %standard-phases> + (add-before 'configure 'configure-fix> + (lambda* _> + (invoke "aclocal")> + (invoke "automake" "--add-missing")> + (invoke "autoreconf"))))))
I got surprised that this phase is necessary, as gnu-build-systemalready includes the 'bootstrap phase. I tried removing it and thepackage still builds. Was that not the case for you?
Toggle quote (10 lines)
> + (native-inputs> + `(("pkg-config" ,pkg-config)> + ("autoconf" ,autoconf)> + ("automake" ,automake)> + ("glib" ,glib "bin")))> + (inputs> + `(("glib" ,glib)> + ("dbus-glib", dbus-glib)> + ("libx11" ,libx11)))
Usually, alphabetical order is preferred unless there's a reason todeviate.
Toggle quote (4 lines)
> + (description "Kbdd is a simple keyboard layout manager.> ++ Features: WM / DE independant, Written in plain C (only glib dependant), > ++ has optional dbus interface")
The leading pluses almost certainly shouldn't be there. The descriptionis written somewhat oddly. When you're out of inspiration for adescription, you can adapt Debian's. In fact, the description used byDebian made it much more clear to me why I'd want to use the package:
(synopsis "Per-window keyboard layout switching daemon for X") (description "@command{kbdd} is a simple keyboard layout switchingprogram, which is designed to run in an X11 session and rememberkeyboard layouts on a per-window basis. That can be very handy for auser of a non-US keyboard who does not want to jump through layouts backand forth while typing in terminals (mostly in a latin alphabet) andsome kind of chat (in native language).
@command{kbdd} also supports D-Bus signals, which makes it possible tocreate layout indicator widgets.")
Thanks for your contributions! Would you mind sending an updated patch?(To the same bug number, 40810@debbugs.gnu.org. Also, please CC me, as Idon't subscribe to the guix-patches mailing list.)
Regards,Jakub Kądziołka