From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dmy3f-00073x-7M for guix-patches@gnu.org; Wed, 30 Aug 2017 04:09:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dmy3b-0001TI-6F for guix-patches@gnu.org; Wed, 30 Aug 2017 04:09:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:52794) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dmy3a-0001Sw-Rg for guix-patches@gnu.org; Wed, 30 Aug 2017 04:09:03 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dmy3a-0003Yw-Ax for guix-patches@gnu.org; Wed, 30 Aug 2017 04:09:02 -0400 Subject: [bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful. Resent-Message-ID: Message-Id: MIME-Version: 1.0 From: Arun Isaac Date: Wed, 30 Aug 2017 13:37:49 +0530 In-reply-to: <20170822171303.21754-1-mail@cbaines.net> References: <20170822171303.21754-1-mail@cbaines.net> Content-Type: text/plain Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Christopher Baines Cc: 28185@debbugs.gnu.org Ok. Ludo has spoken. :-) Let us proceed with the patch review. > - (define (install-file? file stat) > - (let ((stripped-file (string-trim (string-drop file (string-length s= ource)) #\/))) > - (and (any (cut string-match <> stripped-file) include) > - (not (any (cut string-match <> stripped-file) exclude))))) > + (define* (install-file? file stat #:key verbose?) > + (let* ((stripped-file (string-trim > + (string-drop file (string-length source)) #\/= ))) > + (define (match-stripped-file action regex) > + (let ((result (string-match regex stripped-file))) > + (if (and result verbose?) > + (format #t "info: ~A ~A as it matches \"~A\"\n" > + stripped-file action regex)) > + result)) > + > + (if verbose? > + (format #t "info: considering installing ~A\n" stripped-file)) > + > + (and (any (cut match-stripped-file "included" <>) include) > + (not (any (cut match-stripped-file "excluded" <>) exclude))))= ) If I understand this correctly, `install-file?' will be called with `#:verbose #t' only when no files were installed. In that case, we would simply get a long list of "excluded" statements. This seems a bit redundant. Do we really need this? ... continued below > + (if (null? files-to-install) > + (begin > + (format #t "error: No files found to install.\n") > + (find-files source (lambda (file stat) > + (install-file? file stat #:verbose? #t))) I understand that the verbose output also shows the regexp due to which the file was excluded, and that has debugging value. But, perhaps, it'll be better to just print that here instead of a call back to `install-file?'. WDYT? > + #f) > + (begin > + (for-each > + (lambda (file) > + (let* ((stripped-file (string-drop file (string-length sour= ce))) > + (target-file (string-append target-directory strippe= d-file))) > + (format #t "`~a' -> `~a'~%" file target-file) > + (install-file file (dirname target-file)))) > + files-to-install) > + #t)))) Could you please rewrite this section using `cond' instead of `if'? That way, we can get rid of the `begin'. Also, it might be better if we used "positive logic" -- meaning, we first check for truthiness of `files-to-install' instead of checking for (null? files-to-install). Then, the else statement would take care of the empty files-to-install case, and we would never have to call `null?' explicitly. =