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

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

[Thovthe via Guix-patches via]

Here is a package definition for QDirStat, an excellent tool for
checking what files are taking up space on your system.  This is
my first guix module and it was made with help from Ryan Prior
(github.com/sponsors/ryanprior).  I have confirmed that it builds
and runs on aarch64 but the only system I have available is this
fairly anemic arm laptop so I haven't tested the other
architectures.  Suggestions, testing, questions, comments and
education welcomed.



From 9e56b8c2f5602c20ee5796de7af019ef640167cb Mon Sep 17 00:00:00 2001
From: Thovthe <thovthe@protonmail.com>
Date: Sat, 2 May 2020 22:48:30 +0000
Subject: [PATCH] 2020-05-02  Thovthe  thovthe@protonmail.com

* added new package definition for QDirStat at gnu/packages/qdirstat.scm
---
 gnu/packages/qdirstat.scm | 46 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 gnu/packages/qdirstat.scm

diff --git a/gnu/packages/qdirstat.scm b/gnu/packages/qdirstat.scm
new file mode 100644
index 0000000000..91581a206e
--- /dev/null
+++ b/gnu/packages/qdirstat.scm
@@ -0,0 +1,46 @@
+(define-module (qdirstat)
+  #: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))
+
+
+(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"))))))))
+
+    (inputs
+     `(("qtbase" ,qtbase)))
+    (native-inputs
+     `(("zlib" ,zlib)
+       ("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+)))
--
2.26.1

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

[Jack Hill]

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

Thovthe,

On Sun, 3 May 2020, Thovthe via Guix-patches via wrote:

> Here is a package definition for QDirStat, an excellent tool for
> checking what files are taking up space on your system.  This is
> my first guix module and it was made with help from Ryan Prior
> (github.com/sponsors/ryanprior).  I have confirmed that it builds
> and runs on aarch64 but the only system I have available is this
> fairly anemic arm laptop so I haven't tested the other
> architectures.  Suggestions, testing, questions, comments and
> education welcomed.

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.

> From 9e56b8c2f5602c20ee5796de7af019ef640167cb Mon Sep 17 00:00:00 2001
> From: Thovthe <thovthe@protonmail.com>
> Date: Sat, 2 May 2020 22:48:30 +0000
> Subject: [PATCH] 2020-05-02  Thovthe  thovthe@protonmail.com
>
> * added new package definition for QDirStat at gnu/packages/qdirstat.scm

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

> ---
> gnu/packages/qdirstat.scm | 46 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 gnu/packages/qdirstat.scm
>
> diff --git a/gnu/packages/qdirstat.scm b/gnu/packages/qdirstat.scm
> new file mode 100644
> index 0000000000..91581a206e
> --- /dev/null
> +++ b/gnu/packages/qdirstat.scm

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

> @@ -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. You can test 
the patches locally on your end by applying them to a git checkout of 
the guix repository and running guix from that checkout. See the manual 
pages on Building from Git and Running Guix Before it is Installed.

https://guix.gnu.org/manual/en/html_node/Building-from-Git.html
https://guix.gnu.org/manual/en/html_node/Running-Guix-Before-It-Is-Installed.html

> +  #: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.

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

> +
> +    (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.

> +    (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.

> +       ("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."  )

Some formatting nitpicks: you can go ahead and start the description on 
the same line as "(description". Also there is an extra space between the 
closing quote and closing parentheses.

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

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

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.

Can you send an updated patch?

Thanks again,
Jack

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

[Jack Hill]

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

On Tue, 5 May 2020, Jack Hill wrote:

> Thovthe,
>
> On Sun, 3 May 2020, Thovthe via Guix-patches via wrote:
>
>> Here is a package definition for QDirStat, an excellent tool for
>> checking what files are taking up space on your system.  This is
>> my first guix module and it was made with help from Ryan Prior
>> (github.com/sponsors/ryanprior).  I have confirmed that it builds
>> and runs on aarch64 but the only system I have available is this
>> fairly anemic arm laptop so I haven't tested the other
>> architectures.  Suggestions, testing, questions, comments and
>> education welcomed.
>
> 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.

Thovthe,

I'd like to follow up to see if you've had a chance to consider my 
comments. No pressure to work on it right now, but if you need help or 
have additional questions, just let us know. It will be great to get this 
package in Guix.

Best,
Jack

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

[Thovthe via Guix-patches via]

> 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

```

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

[Jack Hill]

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

On Thu, 4 Jun 2020, Thovthe via Guix-patches via wrote:

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

Thanks! The patch looks improved to me. However, I'm still new to 
reviewing patches, so I don't know the answers to some of your questions. 
I also realized that I gave you bad advice is some places. Apologies, and 
thanks for bearing with me.

A more experienced person will be the one to commit the patch, and I'm 
sure that they will correct anything else I got wrong.

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

I'm not sure what you mean by smudge, but I think the answer is to amend. 
The idea is to have one commit that goes from having no QDirStat at all to 
working QDirStat package reflecting the improvements made during this 
review.

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

I don't know why it is called 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?

Yes, I think that's exactly it. Since the module name mirrors the relative 
file system path it is sensitive to those types of changes.

>>> -   #: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.

Thanks. How we prefer to handle the guix licenses case is another thing 
that I don't know.

[…]

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

What I meant was that the lambda should return the value #t. Sometime we 
ensure this by adding a literal #t as the last form in the body of the 
lambda like:

(lambda (arg)
   (do-interesting-stuff …) ; this returns some value other than #t
   #t)

However, it turns out that invoke always returns #t on its own, so what 
you had originally was fine. Sorry for the confusion.

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

Great!

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

This is another thing that I got wrong. It looks like having the newline 
is a common thing to do in Guix, probably for the reasons you describe. 
Feel free to put this back as it was. Sorry for creating extra work.

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

Great, thank you.

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

Thanks.

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

Yes, adding another output is what I was thinking. The bind package is an 
example of this.

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

Ok.

To summarize:

Can you squash your squash all your changes together to send one patch?

A more experienced Guix reviewer will have to answer the questions about 
the ordering of the module imports and the right thing to do with 
qdirstat-cache-writer.

Once that's all done, they should be able to commit QDirStat to Guix!

Thanks again for your patience,
Jack

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

[Thovthe via Guix-patches via]

> This is another thing that I got wrong. It looks like having the newline
> is a common thing to do in Guix, probably for the reasons you describe.
> Feel free to put this back as it was. Sorry for creating extra work.

Not a problem.  I'm happy that it's ok for it to closer match my sense of
aesthetics.

I've also moved some things around and added a line of white space for readability
and separation of concerns.



> > > 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.
>
> Ok.
> -------------------------
> > > 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?
>
> Yes, adding another output is what I was thinking. The bind package is an
> example of this.

I've added some comments about this but right now I'm not interested in figuring
out how this package is built at a lower level since with this definition
provides the essential functionality and all tests pass.



> To summarize:
>
> Can you squash your squash all your changes together to send one patch?
>
> A more experienced Guix reviewer will have to answer the questions about
> the ordering of the module imports and the right thing to do with
> qdirstat-cache-writer.
>
> Once that's all done, they should be able to commit QDirStat to Guix!

Alright, it should look like it's popped into being from the very aether.





From 159a4f8b44537c8b76db52b65cd9571962cb58e4 Mon Sep 17 00:00:00 2001
From: Thovthe <thovthe@protonmail.com>
Date: Sat, 13 Jun 2020 21:42:38 +0000
Subject: [PATCH] gnu: Add qdirstat.

* gnu/packages/qdirstat.scm: New file.
* gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
---
 gnu/local.mk              |  1 +
 gnu/packages/qdirstat.scm | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 gnu/packages/qdirstat.scm

diff --git a/gnu/local.mk b/gnu/local.mk
index aa5b67ab5f..2d06d1030c 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -447,6 +447,7 @@ GNU_SYSTEM_MODULES =				\
   %D%/packages/python-xyz.scm			\
   %D%/packages/toys.scm				\
   %D%/packages/tryton.scm			\
+  %D%/packages/qdirstat.scm			\
   %D%/packages/qt.scm				\
   %D%/packages/radio.scm			\
   %D%/packages/ragel.scm			\
diff --git a/gnu/packages/qdirstat.scm b/gnu/packages/qdirstat.scm
new file mode 100644
index 0000000000..8188357855
--- /dev/null
+++ b/gnu/packages/qdirstat.scm
@@ -0,0 +1,49 @@
+(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 packages)
+  #:use-module ((guix licenses) #:prefix license:))
+
+
+(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"))))
+
+    (outputs `("out"))			; "cache-writer" could be made an output
+    (inputs
+     `(("qtbase" ,qtbase)		; Could add ("perl" ,perl) as an input
+       ("zlib" ,zlib)))			; for the cache-writer see debbug#41040
+    (native-inputs
+     `(("qttools" ,qttools)))
+    (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"))))))))
+
+    (home-page "https://github.com/shundhammer/qdirstat")
+    (synopsis "Graphical disk space inspection utility")
+    (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

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

[Jakub Kądziołka]

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

Thanks for your patch!

The repository includes man pages, but the built package doesn't.
Interestingly, the man1 directory is there, it's just empty. Could you
investigate why this happens?

On Sat, Jun 13, 2020 at 09:51:29PM +0000, Thovthe wrote:
> > > > 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.
> >
> > Ok.
> > -------------------------
> > > > 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?
> >
> > Yes, adding another output is what I was thinking. The bind package is an
> > example of this.
> 
> I've added some comments about this but right now I'm not interested in figuring
> out how this package is built at a lower level since with this definition
> provides the essential functionality and all tests pass.

I don't think it makes much sense to separate it into another output,
given that the data produced by the script would be consumed by qdirstat
proper anyway.

It does make sense, however, to include perl as an input. This makes the
script work even though /usr/bin/perl doesn't exist on Guix System.

> >From 159a4f8b44537c8b76db52b65cd9571962cb58e4 Mon Sep 17 00:00:00 2001
> From: Thovthe <thovthe@protonmail.com>
> Date: Sat, 13 Jun 2020 21:42:38 +0000
> Subject: [PATCH] gnu: Add qdirstat.
> 
> * gnu/packages/qdirstat.scm: New file.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> ---
>  gnu/local.mk              |  1 +
>  gnu/packages/qdirstat.scm | 49 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>  create mode 100644 gnu/packages/qdirstat.scm

Applying this patch was a bit problematic. If you put your comments
after the --- here and send the mail with git send-email (from the
send-email output of the git package), it would get picked up
automatically. Sending the patch as an attachment also works. However,
pasting the patch into the message requires quite some fiddling with
'git am'...

> diff --git a/gnu/packages/qdirstat.scm b/gnu/packages/qdirstat.scm
> new file mode 100644
> index 0000000000..8188357855
> --- /dev/null
> +++ b/gnu/packages/qdirstat.scm
> @@ -0,0 +1,49 @@
> +(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 packages)
> +  #:use-module ((guix licenses) #:prefix license:))

Make sure to include the copyright header that's present in all other
files.

> +(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"))))
> +
> +    (outputs `("out"))			; "cache-writer" could be made an output

There's no point in specifying OUTPUTS if we're going to use the default
value.

Regards,
Jakub Kądziołka

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]