First patch & hello

First patch & hello

[L p R n d n]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

Hello,

First, thank you very much for all the work done on Guix, it's a
pleasure to use it. And also many thanks to Pierre Neidhardt for his
very nice packaging tutorial, it helped me a lot.

So, I wrote my first package for guix and as I'm no dev and not used to
work with patches so I wanted to get some feedbacks before trying to
submit it.

I tried to package font-manager based on the definition used in Nix. It
seems to work but I have a few questions.

1 . I put the package under fontutils.scm but maybe there's a better
place for a font viewer?
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?
3. I tried to get a clean patch but it needed some manual work. What
your workflow to produce patches? (I use emacs)

(It's also my first time using a mailing list, I hope I didn't butcher 
anything)

Thanks,
Lprndn

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-Add-font-manager.patch --]
[-- Type: text/x-patch; name=0001-gnu-Add-font-manager.patch, Size: 3039 bytes --]

From 32325ac307438712a3720259edd092c5e4e1e556 Mon Sep 17 00:00:00 2001
From: Lprndn <guix@lprndn.info>
Date: Tue, 13 Nov 2018 13:19:26 +0100
Subject: [PATCH] gnu: Add font-manager.

---
 gnu/packages/fontutils.scm | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/gnu/packages/fontutils.scm b/gnu/packages/fontutils.scm
index 09ba9b7e4..21eb54b29 100644
--- a/gnu/packages/fontutils.scm
+++ b/gnu/packages/fontutils.scm
@@ -43,6 +43,10 @@
   #:use-module (gnu packages xorg)
   #:use-module (gnu packages gtk)
   #:use-module (gnu packages xml)
+  #:use-module (gnu packages file)
+  #:use-module (gnu packages base)
+  #:use-module (gnu packages databases)
+  #:use-module (gnu packages gnome)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix packages)
   #:use-module (guix download)
@@ -619,6 +623,56 @@ generate bitmaps.")
    (license license:gpl3+)
    (home-page "https://fontforge.github.io/en-US/")))
 
+(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"))))
+   (build-system gnu-build-system)
+   (arguments
+    `(#:configure-flags
+      '("--with-file-roller" "--disable-pycompile")
+      #:phases
+      (modify-phases %standard-phases
+                     (add-after 'unpack 'noconfigure
+                      (lambda _
+                        (setenv "NOCONFIGURE" "true")
+                        #t)))))
+   (native-inputs
+    `(("pkg-config" ,pkg-config)
+      ("automake" ,automake)
+      ("autoconf" ,autoconf)
+      ("libtool" ,libtool)
+      ("file" ,file)
+      ("vala" ,vala)
+      ("yelp-tools" ,yelp-tools)
+      ("gobject-introspection" ,gobject-introspection)))
+   (inputs
+    `(("file-roller" ,file-roller)
+      ("libxml2" ,libxml2)
+      ("json-glib" ,json-glib)
+      ("sqlite" ,sqlite)
+      ("itstool" ,itstool)
+      ("librsvg" ,librsvg)
+      ("gtk+" ,gtk+)
+      ("glib" ,glib "bin")
+      ("intltool" ,intltool)
+      ("gucharmap" ,gucharmap)
+      ("libgee" ,libgee)))
+   (home-page "https://fontmanager.github.io/")
+   (synopsis "Simple font management for GTK+ desktop environments.")
+   (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.")
+   (license license:gpl3)))
+
 (define-public python2-ufolib
   (package
     (name "python2-ufolib")
-- 
2.19.1

Re: First patch & hello

[Ricardo Wurmus]

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

Re: First patch & hello

[L p R n d n]

Ricardo Wurmus <rekado@elephly.net> writes:

> 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:
> 
> ;; 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.
> 
> 
>> 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 :)

Thanks! It was exactly what I was looking for!

>> Subject: [PATCH] gnu: Add font-manager.
>> 
>> ---
> 
> It seems that the commit message is missing here.  In this case the
> complete commit message would be:
> 
> gnu: Add font-manager.
> 
> * gnu/packages/fontutils.scm (font-manager): New variable.
> 

Do you know why it wasn't added? I used magit format-patch.

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

Is there a way to specify a custom location for "guix lint"? A little
bit like the "-L" option from "guix build" for example?
I suppose the right way to invoke 'guix lint' should be to use 
"./pre-inst-env" in a local
guix repository, but I didn't find where I was supposed to find it. :/

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

I will soon. I just tested to build the package with "rounds=2" and it
seems the package isn't deterministic. Is it acceptable or a total
no-go?

Thanks again,

Lprndn

Re: First patch & hello

[swedebugia]

Hi :)

Welcome to the community.

On 2018-11-14 16:51, L p R n d n wrote:
> Ricardo Wurmus <rekado@elephly.net> writes:

snip

> 
> Do you know why it wasn't added? I used magit format-patch.

I did not get to learn magit yet. Did you commit first?

I commit first and then format-patch. (with the git CLI)

> 

snip

> 
> Is there a way to specify a custom location for "guix lint"? A little
> bit like the "-L" option from "guix build" for example?
> I suppose the right way to invoke 'guix lint' should be to use 
> "./pre-inst-env" in a local
> guix repository, but I didn't find where I was supposed to find it. :/

this script is generated by bootstrapping and configuring the source of 
guix.
(We should really note this in the manual. I stumbled on the same stone 
a year ago. I will prepare a patch).
you can specify a package as argument to guix lint.
"guix lint <package-name>"

> 
>> Could you please send an updated patch to guix-patches@gnu.org?
>>
> 
> I will soon. I just tested to build the package with "rounds=2" and it
> seems the package isn't deterministic. Is it acceptable or a total
> no-go?

Submit it after linting it and others can help you to find the cause of 
this. (usually timestamp inflicted)

-- 
Cheers
Swedebugia

Re: First patch & hello

[Ricardo Wurmus]

L p R n d n <guix@lprndn.info> writes:

>> It seems that the commit message is missing here.  In this case the
>> complete commit message would be:
>>
>> gnu: Add font-manager.
>>
>> * gnu/packages/fontutils.scm (font-manager): New variable.
>>
>
> Do you know why it wasn't added? I used magit format-patch.

This is not done automatically.  You need to write the commit message
yourself.  Since you’re using Emacs you may be interested in the
Yasnippet snippets we offer for magit.  After enabling them you only
need to inputs “add<TAB>” to get this kind of message (you then need to
complete the package name).

>> 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
>
> Is there a way to specify a custom location for "guix lint"? A little
> bit like the "-L" option from "guix build" for example?
> I suppose the right way to invoke 'guix lint' should be to use
> "./pre-inst-env" in a local
> guix repository, but I didn't find where I was supposed to find it. :/

“pre-inst-env” tells Guix to use your custom version of Guix in the same
directory.  “./pre-inst-env guix lint foo” operates on your local “foo”
package.

>> Could you please send an updated patch to guix-patches@gnu.org?
>>
>
> I will soon. I just tested to build the package with "rounds=2" and it
> seems the package isn't deterministic. Is it acceptable or a total
> no-go?

It would be good to check why it isn’t deterministic.  You can use “-K”
with “guix build --check --no-grafts” to keep the second output.  Then
you can run diffoscope on it.

You can also search the sources for __DATE__ or __TIME__ or maybe
“date”.  Often non-determinism is due to embedded build timestamps.

--
Ricardo

Re: First patch & hello

[L p R n d n]

Thanks to both of you for your help. I should be able to submit trivial
packages on my own from now on.

I finally succeeded at using ./pre-inst-env. Indeed, clarifying its
documentation is super nice!
The cleaned patch is submitted and I will now try investigating the
non-determinism issue (and maybe try other packages too...).

Lprndn