[bug#41040] [PATCH] Package Definition for QDirStat

[Thovthe]

> Thanks for contributing this patch to Guix! I have some suggestions for
> how it can be cleaned up so that it can be included in the distribution.

Sounds good, I'll try to address those suggestions.



> Guix uses a very structured changelog format for commit messages. I find
> that the best way to figure it out is to look at the existing git log. In
> this case the commit message should be something like:
>
>     gnu: Add qdirstat.
>
>     * gnu/packages/qdirstat.scm: New file.
>     * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.

I hope the new one works fine.  Do you want me to amend or smudge?



> As you may have guessed from my proposed commit message, when adding a new
> file, you should also add it to the appropriate section in gnu/local.mk

I have done, but I must admit it is somewhat confusing to me that it goes
under GNU_SYSTEM_MODULES.



> > @@ -0,0 +1,46 @@
> > +(define-module (qdirstat)
>
> The module name should be (gnu packages qdirstat) to match the file path
> of the module.
>
> If you'll permit me to speculate a little bit: I suspect that you
> developed this package outside of the main Guix repository.

Done.

It was originally called that but the linter told me I ought to change
it.  Did it do that because I worked on this outside of git?



> > -   #:use-module (gnu packages qt)
> > -   #:use-module (gnu packages compression)
> > -   #:use-module (guix build-system gnu)
> > -   #:use-module (guix git-download)
> > -   #:use-module ((guix licenses) #:prefix license:)
> > -   #:use-module (guix packages))
>
> Since this is a new file, let's go ahead and sort these alphabetically.

I have now but I'm not sure how to treat parens so the `licenses` line is at the
bottom.  If there is a convention feel free to change that around.



> > +(define-public qdirstat
> >
> > -   (package
> > -   (name "qdirstat")
> > -   (version "1.6.1")
> > -   (source
> > -       (origin
> > -         (method git-fetch)
> > -         (uri (git-reference
> > -               (url "https://github.com/shundhammer/qdirstat.git";)
> > -               (commit version)))
> > -         (file-name (git-file-name name version))
> > -         (sha256
> > -          (base32 
> > "0q77a347qv1aka6sni6l03zh5jzyy9s74aygg554r73g01kxczpb"))))
> > -   (build-system gnu-build-system)
> > -   (arguments
> > -       `(#:phases
> > -         (modify-phases %standard-phases
> > -           (replace 'configure
> > -             (lambda* (#:key outputs #:allow-other-keys)
> > -               (invoke "qmake"
> > -                       (string-append "PREFIX="
> > -                                      (assoc-ref outputs "out"))
> > -                       (string-append "INSTALL_PREFIX="
> > -                     (assoc-ref outputs "out"))))))))
>
> It is our custom to have all phases return #t

I don't really know what this means, looked through the documentation but all I 
can workout is that something needs to pass what I assume is 'true' to 
somewhere else.  I took this verbatim from another definition in guix.



> > -   (native-inputs
> > -       `(("zlib" ,zlib)
>
> I think zlib should be an input rather than a native-input since the code
> seems to link to it. Remember that the reason for native-inputs verses
> inputs is that native-inputs are needed on the host side when cross
> compiling.

Done.



> > -   (home-page "https://github.com/shundhammer/qdirstat";)
> > -   (synopsis "Graphical disk space inspection utility")
> > -   (description
> > -       "QDirStat is a graphical application to show where your disk space 
> > has
> > +gone and to help you to clean it up. Shaded boxes represent files and files
> > +are grouped by directory structure." )
>
> Some formatting nitpicks: you can go ahead and start the description on
> the same line as "(description".

Done, though I think it looks better to have it all columnised if it needs to be
broken up at 78chars.  Also makes M-q in emacs behave itself.

> Also there is an extra space between the closing quote and closing 
> parentheses.

Blaming this on my tools as well possibly even the linter but I cant exactly 
recall what did it.



> The best terms I found when searching for similar packages that we already
> have in Guix where, "disk usage", so I think it would be good idea to work
> usage into the synopsis or description somehow. Perhaps change the
> synopsis to say, "disk usage inspection" or the description to say, "show
> your disk usage and help you clean it up."

Done and added analys- root word in case that comes to someone's mind rather
than inspection.



> > -   (license license:gpl2+)))
>
> qdirstat-cache-writer has a different license in its header. It's an …
> interesting licence :) I think it's free software, so eligable for
> inclusion in Guix, but probably worth recording here, so that the licence
> field becomes:
>
> (license (list license:gpl2+
> license:non-copyleft)) ; scripts/qdirstat-cache-writer

Done.



> For bonus points, it might be nice to move qdirstat-cache-writer to its
> own output since it is made to be run independently of QDirStat and that
> way it could be installed without pulling in all the C++ and Graphical
> dependencies.

How do you think this should look?  Would I make another output in this
qdirstat.scm?



> > -
> > -   (inputs
> > -       `(("qtbase" ,qtbase)))
> >
> >
>
> I think that perl should be added as input, so that the #! line of
> qdirstat-cache-writer can be patched to refer to a perl in the store.

I'm leaving this for once I've moved qdirstat-cache-writer into a
separate package/output since this definition works for the essential
functionality.



> Can you send an updated patch?

Here and done!



```
>From f234ad07e06a5e97fdf1cad36e99e180ae6001f3 Mon Sep 17 00:00:00 2001
From: Thovthe <thovthe@protonmail.com>
Date: Thu, 4 Jun 2020 06:42:54 +0000
Subject: [PATCH]        gnu: Add qdirstat.

        * gnu/packages/qdirstat.scm: Modified according to feedback
        * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
---
 gnu/local.mk              |  1 +
 gnu/packages/qdirstat.scm | 24 ++++++++++++------------
 2 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/gnu/local.mk b/gnu/local.mk
index 3c9a10b6bc..d1adcbeaa2 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -440,6 +440,7 @@ GNU_SYSTEM_MODULES =                                \
   %D%/packages/toys.scm                                \
   %D%/packages/tryton.scm                      \
   %D%/packages/qt.scm                          \
+  %D%/packages/qdirstat.scm                    \
   %D%/packages/radio.scm                       \
   %D%/packages/ragel.scm                       \
   %D%/packages/rails.scm                       \
diff --git a/gnu/packages/qdirstat.scm b/gnu/packages/qdirstat.scm
index 91581a206e..a450b8a51a 100644
--- a/gnu/packages/qdirstat.scm
+++ b/gnu/packages/qdirstat.scm
@@ -1,10 +1,10 @@
-(define-module (qdirstat)
-  #:use-module (gnu packages qt)
+(define-module (gnu packages qdirstat)
   #:use-module (gnu packages compression)
+  #:use-module (gnu packages qt)
   #:use-module (guix build-system gnu)
   #:use-module (guix git-download)
-  #:use-module ((guix licenses) #:prefix license:)
-  #:use-module (guix packages))
+  #:use-module (guix packages)
+  #:use-module ((guix licenses) #:prefix license:))


 (define-public qdirstat
@@ -33,14 +33,14 @@
                                     (assoc-ref outputs "out"))))))))

     (inputs
-     `(("qtbase" ,qtbase)))
+     `(("qtbase" ,qtbase)
+       ("zlib" ,zlib)))
     (native-inputs
-     `(("zlib" ,zlib)
-       ("qttools" ,qttools)))
+     `(("qttools" ,qttools)))
     (home-page "https://github.com/shundhammer/qdirstat";)
     (synopsis "Graphical disk space inspection utility")
-    (description
-     "QDirStat is a graphical application to show where your disk space has
-gone and to help you to clean it up.  Shaded boxes represent files and files
-are grouped by directory structure."  )
-    (license license:gpl2+)))
+    (description "QDirStat is a graphical application for analysing disk 
usage.  It shows
+where your disk space has gone and helps you clean it up.  Shaded boxes
+represent files and files are grouped by directory structure.")
+    (license (list license:gpl2+
+                   license:non-copyleft)))) ; scripts/qdirstat-cache-writer
--
2.26.2

```