[PATCH] Package Definition for QDirStat

  • Open
  • quality assurance status badge
Details
3 participants
  • Thovthe
  • Jack Hill
  • Jakub K?dzio?ka
Owner
unassigned
Submitted by
Thovthe
Severity
normal
T
T
Thovthe wrote on 3 May 2020 02:28
(name . guix-patches@gnu.org)(address . guix-patches@gnu.org)
27uwM7nMqpAz-RfKtxXzIOJWs0UnmDO3vqxoqkN1VeXR7GHQJTCgM5B0f8bCMuG2y5QsbCDj_AScqxM-4c1NO5YTf-ZAg3psDt-_FC27LJI=@protonmail.com
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

Toggle diff (52 lines)
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
J
J
Jack Hill wrote on 5 May 2020 06:01
(name . Thovthe)(address . Thovthe@protonmail.com)(address . 41040@debbugs.gnu.org)
alpine.DEB.2.20.2005042239310.5735@marsh.hcoop.net
Thovthe,

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

Toggle quote (9 lines)
> 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.

Toggle quote (7 lines)
> 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.
```

Toggle quote (11 lines)
> ---
> 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

Toggle quote (3 lines)
> @@ -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.


Toggle quote (7 lines)
> + #: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.

Toggle quote (25 lines)
> +(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

Toggle quote (4 lines)
> +
> + (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.

Toggle quote (3 lines)
> + (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.

Toggle quote (8 lines)
> + ("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."

Toggle quote (2 lines)
> + (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
J
J
Jack Hill wrote on 14 May 2020 02:36
(name . Thovthe)(address . Thovthe@protonmail.com)(address . 41040@debbugs.gnu.org)
alpine.DEB.2.20.2005132028580.5735@marsh.hcoop.net
On Tue, 5 May 2020, Jack Hill wrote:

Toggle quote (16 lines)
> 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
T
T
Thovthe wrote on 4 Jun 2020 09:40
(name . 41040@debbugs.gnu.org)(address . 41040@debbugs.gnu.org)
Nc6dd4WaVM3RhEkgX7S588RvUDKStBk52aTYK0lPdSpgZDRY7FVK-PxNfNVy8OUaKJcYe9eYszXFCIOS7ppMQvaVdyVxUy-ByiHoCjSTsf0=@protonmail.com
Toggle quote (3 lines)
> 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.



Toggle quote (9 lines)
> 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?



Toggle quote (3 lines)
> 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.



Toggle quote (9 lines)
> > @@ -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?



Toggle quote (9 lines)
> > - #: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.



Toggle quote (28 lines)
> > +(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.



Toggle quote (8 lines)
> > - (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.



Toggle quote (10 lines)
> > - (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.

Toggle quote (2 lines)
> 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.



Toggle quote (6 lines)
> 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.



Toggle quote (10 lines)
> > - (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.



Toggle quote (5 lines)
> 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?



Toggle quote (9 lines)
> > -
> > - (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.



Toggle quote (2 lines)
> 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(-)

Toggle diff (54 lines)
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

```
J
J
Jack Hill wrote on 7 Jun 2020 06:23
(name . Thovthe)(address . Thovthe@protonmail.com)(name . 41040@debbugs.gnu.org)(address . 41040@debbugs.gnu.org)
alpine.DEB.2.20.2006062341220.5735@marsh.hcoop.net
On Thu, 4 Jun 2020, Thovthe via Guix-patches via wrote:

Toggle quote (5 lines)
>> 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.

Toggle quote (11 lines)
>> 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.

Toggle quote (6 lines)
>> 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.

Toggle quote (14 lines)
>>> @@ -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.

Toggle quote (12 lines)
>>> - #: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.

[…]

Toggle quote (18 lines)
>>> - (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.

Toggle quote (10 lines)
>>> - (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!

Toggle quote (13 lines)
>>> - (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.

Toggle quote (4 lines)
>> 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.

:)

Toggle quote (9 lines)
>> 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.

Toggle quote (12 lines)
>>> - (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.

Toggle quote (8 lines)
>> 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.

Toggle quote (13 lines)
>>> -
>>> - (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
T
T
Thovthe wrote on 13 Jun 2020 23:51
(name . 41040@debbugs.gnu.org)(address . 41040@debbugs.gnu.org)
zAApckKRhqvy2njcWIMRnuUCNPFhXjm8fmOPU63f2CiFaXZY21i30yHZ1MUcnKXngDew7bcus_Z72EFO4L5Ty3z1rc-Bbj24AB6y-QXCx5o=@protonmail.com
Toggle quote (4 lines)
> 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.



Toggle quote (20 lines)
> > > 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.



Toggle quote (10 lines)
> 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

Toggle diff (67 lines)
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
J
J
Jakub K?dzio?ka wrote on 28 Jun 2020 14:56
(name . Thovthe)(address . Thovthe@protonmail.com)(name . 41040@debbugs.gnu.org)(address . 41040@debbugs.gnu.org)
20200628125624.3vr2borezix5ij4s@gravity
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:
Toggle quote (24 lines)
> > > > 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.

Toggle quote (13 lines)
> >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'...

Toggle quote (14 lines)
> 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.

Toggle quote (16 lines)
> +(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
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEE5Xa/ss9usT31cTO54xWnWEYTFWQFAl74k3gACgkQ4xWnWEYT
FWTLUA/+PkJz7GjI6VxjT9OLRy7bwGXIGq0CRJ4e1HSe1Idkfc2xAn+fzhbsRE9A
Tx5UeuHGQcWf0kuWJGS1wQ/aCp2+6I9QHsCGivjxEoqq5V7jrnnjGHCz3gJQ/e2r
6wUQ0uO7RMau+uaSYK/0LlNt1eATyO02EbGFv7eBbQ4KkhP3mVocQ52TEv6+1aq9
uQn1UBJGqZ9DJqhCdou6kaGVzxaeUQv1Fy30mYAUPRLkFpdHZM6ZdWfv58gLCKTi
ciXINDGLxtikhW4Ab3863AXpgJ6AYu3JijM5yFLjd3D2FzJFMUcsLSfnPxRf6WbO
QSYNlTj+fOT+Kph5L9F7Zh2nwuuBsvsZIi+1bKrNDl5teWsRngckOeBZp1+r5rvS
eneSIMZd4I+6xXuRIGkh1iv76CApnOVqKSprkab7VuGsdTNv2iS2iHecMdp5AD7b
a3AcxgG1ayYToqa1hb10FiD3dQ7bNyxzCAXrNgLVt+UZukDaapUuTP8MikSdIBnK
EAiMDVVdA5rpJGGTQnT+qIAW/Bx8g76cZ7Rk2MhIejCkNuci1VyfD5V0xfuENVok
okpK1IrHSD3AG5ORnfXFXcVkLUiggdlI3v7frd2/pdoz0tmZnJi4sEZSIinmxTLz
25vkXO0PQBWeRV7zaRclbURp3K9GtgBHsZiTptoLueOg7JE60dU=
=k9dq
-----END PGP SIGNATURE-----


?