From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp0 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id GDKcI+tr3F4OIwAA0tVLHw (envelope-from ) for ; Sun, 07 Jun 2020 04:24:11 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp0 with LMTPS id WGZyH+tr3F4PfAAA1q6Kng (envelope-from ) for ; Sun, 07 Jun 2020 04:24:11 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 68B7D940418 for ; Sun, 7 Jun 2020 04:24:10 +0000 (UTC) Received: from localhost ([::1]:38212 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jhmqt-0006YU-S4 for larch@yhetil.org; Sun, 07 Jun 2020 00:24:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:36460) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jhmqo-0006YN-Bi for guix-patches@gnu.org; Sun, 07 Jun 2020 00:24:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:41590) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jhmqo-0007Vk-32 for guix-patches@gnu.org; Sun, 07 Jun 2020 00:24:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jhmqn-0002v2-Ux for guix-patches@gnu.org; Sun, 07 Jun 2020 00:24:01 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#41040] [PATCH] Package Definition for QDirStat Resent-From: Jack Hill Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Sun, 07 Jun 2020 04:24:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41040 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Thovthe Cc: "41040@debbugs.gnu.org" <41040@debbugs.gnu.org> Received: via spool by 41040-submit@debbugs.gnu.org id=B41040.159150382911201 (code B ref 41040); Sun, 07 Jun 2020 04:24:01 +0000 Received: (at 41040) by debbugs.gnu.org; 7 Jun 2020 04:23:49 +0000 Received: from localhost ([127.0.0.1]:53136 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhmqY-0002uZ-Qi for submit@debbugs.gnu.org; Sun, 07 Jun 2020 00:23:49 -0400 Received: from minsky.hcoop.net ([104.248.1.95]:33468) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jhmqT-0002uJ-MN for 41040@debbugs.gnu.org; Sun, 07 Jun 2020 00:23:45 -0400 Received: from marsh.hcoop.net ([45.55.52.66]) by minsky.hcoop.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1jhmqN-0002mu-MD; Sun, 07 Jun 2020 00:23:35 -0400 Date: Sun, 7 Jun 2020 00:23:35 -0400 (EDT) From: Jack Hill X-X-Sender: jackhill@marsh.hcoop.net In-Reply-To: Message-ID: References: <27uwM7nMqpAz-RfKtxXzIOJWs0UnmDO3vqxoqkN1VeXR7GHQJTCgM5B0f8bCMuG2y5QsbCDj_AScqxM-4c1NO5YTf-ZAg3psDt-_FC27LJI=@protonmail.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="925712948-1163781102-1591503815=:5735" X-Spam-Score: 1.6 (+) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-Spam-Score: -1.0 (-) X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Spam-Score: -0.01 X-TUID: xck9Avuc5RcH This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --925712948-1163781102-1591503815=:5735 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8BIT 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 --925712948-1163781102-1591503815=:5735--