From debbugs-submit-bounces@debbugs.gnu.org Thu Jun 04 03:41:01 2020 Received: (at 41040) by debbugs.gnu.org; 4 Jun 2020 07:41:01 +0000 Received: from localhost ([127.0.0.1]:45051 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgkUj-0000rT-Bt for submit@debbugs.gnu.org; Thu, 04 Jun 2020 03:41:01 -0400 Received: from mail-40130.protonmail.ch ([185.70.40.130]:22357) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jgkUd-0000rA-09 for 41040@debbugs.gnu.org; Thu, 04 Jun 2020 03:40:55 -0400 Date: Thu, 04 Jun 2020 07:40:35 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail; t=1591256443; bh=DJx/vMCOt9CI0E8EIHL0D41lYzyKzaFHFQyoOlzHd34=; h=Date:To:From:Reply-To:Subject:In-Reply-To:References:From; b=d04XsOvuaYzGSuk3D4bAamrTKLFK2AcM8okGPpVgAb1oy3nUOgZJ9xy+0z3kUonNE 7ld4L/dEJoA7vf7bI78lI6QEWfAjxpei9HCf0vfMA2+dF4B0yvMwjYPUpVT/FrT4vw EoYtf9K3kniEX/2tOWTAESh7cmrQ+M4plmkZursM= To: "41040@debbugs.gnu.org" <41040@debbugs.gnu.org> From: Thovthe Subject: Re: [bug#41040] [PATCH] Package Definition for QDirStat Message-ID: In-Reply-To: References: <27uwM7nMqpAz-RfKtxXzIOJWs0UnmDO3vqxoqkN1VeXR7GHQJTCgM5B0f8bCMuG2y5QsbCDj_AScqxM-4c1NO5YTf-ZAg3psDt-_FC27LJI=@protonmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.2 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM shortcircuit=no autolearn=disabled version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on mail.protonmail.ch X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 41040 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Thovthe Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.7 (-) > 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 ne= w > 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 a= t 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 "0q77a347qv1aka6sni6l03zh5jzyy9s74aygg554r73g01kxczp= b")))) > > - (build-system gnu-build-system) > > - (arguments > > - `(#:phases > > - (modify-phases %standard-phases > > - (replace 'configure > > - (lambda* (#:key outputs #:allow-other-keys) > > - (invoke "qmake" > > - (string-append "PREFIX=3D" > > - (assoc-ref outputs "out")) > > - (string-append "INSTALL_PREFIX=3D" > > - (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 a= ll 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 sp= ace 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 parent= heses. Blaming this on my tools as well possibly even the linter but I cant exactl= y recall what did it. > The best terms I found when searching for similar packages that we alread= y > have in Guix where, "disk usage", so I think it would be good idea to wor= k > 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 rathe= r than inspection. > > - (license license:gpl2+))) > > qdirstat-cache-writer has a different license in its header. It's an = =E2=80=A6 > 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 Date: Thu, 4 Jun 2020 06:42:54 +0000 Subject: [PATCH] =09gnu: Add qdirstat. =09* gnu/packages/qdirstat.scm: Modified according to feedback =09* 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 =3D=09=09=09=09\ %D%/packages/toys.scm=09=09=09=09\ %D%/packages/tryton.scm=09=09=09\ %D%/packages/qt.scm=09=09=09=09\ + %D%/packages/qdirstat.scm=09=09=09\ %D%/packages/radio.scm=09=09=09\ %D%/packages/ragel.scm=09=09=09\ %D%/packages/rails.scm=09=09=09\ 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 ha= s -gone and to help you to clean it up. Shaded boxes represent files and fil= es -are grouped by directory structure." ) - (license license:gpl2+))) + (description "QDirStat is a graphical application for analysing disk u= sage. 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-write= r -- 2.26.2 ```