Re: First patch & hello

[Ricardo Wurmus <rekado@elephly.net>]

Hi,

thank you for your contribution!

> 1 . I put the package under fontutils.scm but maybe there's a better
> place for a font viewer?

This seems fine to me.

> 2. I found there is a glib-or-gtk-build-system, I hesitated to use as
> Im̀
> not sure what its purpose. Can someone clarify its use?

The glib-or-gtk-build-system is an extension of the gnu-build-system
that is useful for GNOME packages and other packages that require a
little more setup after installation.  Here’s what the comments in
“guix/build-system/glib-or-gtk.scm” say:

--8<---------------cut here---------------start------------->8---
;; This build system is an extension of the 'gnu-build-system'.  It
;; accomodates the needs of applications making use of glib or gtk+ (with "or"
;; to be interpreted in the mathematical sense).  This is achieved by adding
;; two phases run after the 'install' phase:
;;
;; 'glib-or-gtk-wrap' phase:
;;
;; a) This phase looks for GSettings schemas, GIO modules and theming data.
;; If any of these is found in any input package, then all programs in
;; "out/bin" are wrapped in scripts defining the nedessary environment
;; variables.
;;
;; b) Looks for the existence of "libdir/gtk-3.0" directories in all input
;; packages.  If any is found, then the environment variable "GTK_PATH" is
;; suitably set and added to the wrappers.  The variable "GTK_PATH" has been
;; preferred over "GTK_EXE_PREFIX" because the latter can only point to a
;; single directory, while we may need to point to several ones.
;;
;; 'glib-or-gtk-compile-schemas' phase:
;;
;; Looks for the presence of "out/share/glib-2.0/schemas".  If that directory
;; exists and does not include a file named "gschemas.compiled", then
;; "glib-compile-schemas" is run in that directory.
--8<---------------cut here---------------end--------------->8---

> 3. I tried to get a clean patch but it needed some manual work. What
> your workflow to produce patches? (I use emacs)

Usually you would make a local git commit and then run

    git format-patch -1

to generate a patch file from that commit.  You could also directly send
it to the mailing list with “git send-email”.

What follows are some comments about the patch.  I’m trying to be extra
thorough; please don’t let this discourage you.  Some of my comments
will just be matters of opinion :)

> Subject: [PATCH] gnu: Add font-manager.
>
> ---

It seems that the commit message is missing here.  In this case the
complete commit message would be:

--8<---------------cut here---------------start------------->8---
gnu: Add font-manager.

* gnu/packages/fontutils.scm (font-manager): New variable.
--8<---------------cut here---------------end--------------->8---

> +(define-public font-manager
> +  (package
> +   (name "font-manager")
> +   (version "0.7.3.1")
> +   (source (origin
> +            (method url-fetch)
> +            (uri (string-append "https://github.com/FontManager/master/archive/" version ".tar.gz"))
> +            (sha256
> +             (base32 "1zq2v299xqznj31brjh8nk1w4hb47qpxsyg4ngp01dh7f2sza146"))))

Please try to avoid using the generated tarballs on Github; the URLs
ending on “/archive/…” are generated on demand and cached for a very
long time, but they are not “stable”.  Over time the tarballs may be
regenerated in a non-reproducible fashion and then the hash would differ
(e.g. because of embedded timestamps).

In this case we can use the developer-uploaded tarball at

    https://github.com/FontManager/master/releases/download/0.7.3.1/font-manager-0.7.3.tar.bz2

In other cases there may not be a developer-uploaded tarball at all.  We
would use the “git-fetch” method then to fetch the sources via git using
the tagged commit (in this case the commit tag name is the same as the
version string).

Please make sure that the line is not too long.  “guix lint” will tell
you about overlong lines.

> +   (build-system gnu-build-system)
> +   (arguments
> +    `(#:configure-flags
> +      '("--with-file-roller" "--disable-pycompile")

Could you please comment on why these flags are required?
“with-file-roller” looks obvious, but I wonder why “disable-pycompile”
is necessary.

> +      #:phases
> +      (modify-phases %standard-phases
> +                     (add-after 'unpack 'noconfigure
> +                      (lambda _
> +                        (setenv "NOCONFIGURE" "true")
> +                        #t)))))

Here the indentation is off.  “modify-phases” is a macro and the
opening paren of the next line should be right under the “o” of
“modify”.  Emacs should do this automatically (check the manual for
hints on the recommended setup).

> +   (native-inputs
> +    `(("pkg-config" ,pkg-config)
> +      ("automake" ,automake)
> +      ("autoconf" ,autoconf)
> +      ("libtool" ,libtool)
> +      ("file" ,file)
> +      ("vala" ,vala)
> +      ("yelp-tools" ,yelp-tools)
> +      ("gobject-introspection" ,gobject-introspection)))

You might not need “automake”, “autoconf” and “libtool” when using the
bootstrapped tarball uploaded by the developers.

I think having “gobject-introspection” in the native inputs is a bit
suspicous.  Is it really not a regular input?  Does “guix gc -R $(guix
build font-manager)” show a reference to “gobject-introspection”?

> +   (inputs
> +    `(("file-roller" ,file-roller)
> +      ("libxml2" ,libxml2)
> +      ("json-glib" ,json-glib)
> +      ("sqlite" ,sqlite)
> +      ("itstool" ,itstool)

Should “itstool” be a native input instead?

> +      ("librsvg" ,librsvg)
> +      ("gtk+" ,gtk+)
> +      ("glib" ,glib "bin")
> +      ("intltool" ,intltool)

Should “intltool” be a native input instead?

> +   (synopsis "Simple font management for GTK+ desktop environments.")

Please remove the trailing period.

> +   (description "Font Manager is intended to provide a way for average users to
> +      easily manage desktop fonts, without having to resort to command
> +      line tools or editing configuration files by hand. While designed
> +      primarily with the Gnome Desktop Environment in mind, it should
> +      work well with other Gtk+ desktop environments.
> +      Font Manager is NOT a professional-grade font management
> solution.")

Please remove the last sentence.  Please also reindent the description
string (there should be no indentation for following lines).  Also
please use two spaces after sentences.  “guix lint” will remind you of
these things.

> +   (license license:gpl3)))

The license is actually gpl3+, i.e. GPL version 3 or later.  You can
tell by looking at the copyright header comments in the source files.

Finally, we prefer to have patches sent to guix-patches@gnu.org, because
that’s backed by the Debbugs bug tracker.  It makes it less likely that
your patch is overlooked.

Could you please send an updated patch to guix-patches@gnu.org?

Thanks!

--
Ricardo