From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: [PATCH] gnu: Add scmutils. Date: Thu, 17 Sep 2015 15:03:08 +0200 Message-ID: <87a8slb3lf.fsf@gnu.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:49119) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcYqK-0002Eg-ST for guix-devel@gnu.org; Thu, 17 Sep 2015 09:03:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcYqE-00043W-Vj for guix-devel@gnu.org; Thu, 17 Sep 2015 09:03:16 -0400 In-Reply-To: (Federico Beffa's message of "Tue, 15 Sep 2015 19:34:20 +0200") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Federico Beffa Cc: Guix-devel Federico Beffa skribis: > From 1aacd03326def7b52b8166a80cc18c8e8226aa62 Mon Sep 17 00:00:00 2001 > From: Federico Beffa > Date: Thu, 13 Aug 2015 18:58:01 +0200 > Subject: [PATCH] gnu: Add scmutils. > > * gnu/packages/scheme.scm (scmutils): New variable. Overall LGTM. The comments below are largely cosmetic: > +;; FIXME: This function is temporarily in the engineering module and not > +;; exported. It will be moved to an utility module for general use. On= ce > +;; this is done, we should remove this definition. > +(define broken-tarball-fetch > + (@@ (gnu packages engineering) broken-tarball-fetch)) Not important for this patch, but eventually we could always make a (guix tarbomb-fetch) module that would export this procedure. > + (modify-phases %standard-phases Since the phases are nontrivial, I would suggest adding a comment right before the =E2=80=98lambda*=E2=80=99 line of each phase to briefly mention = what it does. > + (add-before 'install 'fix-bin Maybe =E2=80=98fix-directory-names=E2=80=99? > + (lambda* (#:key outputs #:allow-other-keys) > + (define* (copy-files-to-directory files dir > + #:optional (dele= te? #f)) > + (for-each (lambda (f) > + (copy-file f (string-append dir "/" = f)) Note for later: once =E2=80=98core-updates=E2=80=99 is merged, we should wr= ite this as: (install-file f dir) > + (when delete? (delete-file f))) > + files)) > + > + (let* ((out (assoc-ref outputs "out")) > + (bin (string-append out "/bin")) > + (doc (string-append out "/share/doc/" > + ,name "-" ,version)) > + (lib (string-append out "/lib/mit-scheme-" > + ,(system-suffix) > + "/scmutils"))) > + (for-each (lambda (d) (mkdir-p d)) (list lib doc= bin)) This can be written as: (for-each mkdir-p (list =E2=80=A6)) > + (with-directory-excursion "scmutils/scmutils" > + (copy-files-to-directory '("COPYING" "LICENSE") > + doc #t) > + (for-each (lambda (f) (delete-file f)) > + (find-files "." "\\.bin")) (for-each delete-file (find-files =E2=80=A6)) This phase deletes a bunch of pre-compiled files, which are those Mark and you were referring to, IIUC. Could you move the removal of all the pre-compiled files to a =E2=80=98snippet=E2=80=99? This will ensure that =E2=80=98guix build -S scmutils=E2=80=99 provides onl= y source code. > + (emacs-lisp-dir > + (string-append out "/share/emacs/site-li= sp")) This should be: (string-append out "/share/emacs/site-lisp/guix.d/" ,name "-" ,version) That way, when installing the package from Emacs, it will automatically be found and loaded (I realize the manual doesn=E2=80=99t explain it, but it probably should.) > + (supported-systems '("x86_64-linux" "i686-linux")) Please add a comment above explaining why this is the case. > + (synopsis "The Scmutils library for MIT Scheme") -The Well, this was not an easy package. Thanks for working on it! Ludo=E2=80=99.