[PATCH] Package Definition for QDirStat

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

---

gnu/packages/qdirstat.scm | 46 +++++++++++++++++++++++++++++++++++++++

1 file changed, 46 insertions(+)

create mode 100644 gnu/packages/qdirstat.scm

--

2.26.1

Thovthe,

On Sun, 3 May 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.

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.

```

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

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.

Since this is a new file, let's go ahead and sort these alphabetically.

It is our custom to have all phases return #t

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

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

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

On Tue, 5 May 2020, Jack Hill wrote:

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

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

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

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

under GNU_SYSTEM_MODULES.

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?

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.

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.

Done.

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.

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

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

than inspection.

Done.

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

qdirstat.scm?

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.

Here and done!

```

From f234ad07e06a5e97fdf1cad36e99e180ae6001f3 Mon Sep 17 00:00:00 2001

* 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(-)

--

2.26.2

```

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

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.

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.

I don't know why it is called GNU_SYSTEM_MODULES.

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.

Thanks. How we prefer to handle the guix licenses case is another thing

that I don't know.

[…]

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.

Great!

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.

:)

Great, thank you.

Thanks.

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

example of this.

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

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

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

From 159a4f8b44537c8b76db52b65cd9571962cb58e4 Mon Sep 17 00:00:00 2001

---

gnu/local.mk | 1 +

gnu/packages/qdirstat.scm | 49 +++++++++++++++++++++++++++++++++++++++

2 files changed, 50 insertions(+)

create mode 100644 gnu/packages/qdirstat.scm

--

2.26.2

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

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

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

files.

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

value.

Regards,

Jakub K?dzio?ka