From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dnXE0-0008B9-2r for guix-patches@gnu.org; Thu, 31 Aug 2017 17:42:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dnXDu-0001gs-So for guix-patches@gnu.org; Thu, 31 Aug 2017 17:42:08 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:57775) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dnXDu-0001gm-Ot for guix-patches@gnu.org; Thu, 31 Aug 2017 17:42:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dnXDu-0003Ts-Fv for guix-patches@gnu.org; Thu, 31 Aug 2017 17:42:02 -0400 Subject: [bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful. Resent-Message-ID: Date: Thu, 31 Aug 2017 22:41:36 +0100 From: Christopher Baines Message-ID: <20170831224136.7499acc1@cbaines.net> In-Reply-To: <6f595802.AEMAPOK4d0UAAAAAAAAAAAPmDT4AAAACwQwAAAAAAAW9WABZpnJ1@mailjet.com> References: <20170822171303.21754-1-mail@cbaines.net> <6f595802.AEMAPOK4d0UAAAAAAAAAAAPmDT4AAAACwQwAAAAAAAW9WABZpnJ1@mailjet.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; boundary="Sig_/v=kqHcmqDwP8nAmHjJ95Coq"; protocol="application/pgp-signature" 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: Arun Isaac Cc: 28185@debbugs.gnu.org --Sig_/v=kqHcmqDwP8nAmHjJ95Coq Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable On Wed, 30 Aug 2017 13:37:49 +0530 Arun Isaac wrote: > Ok. Ludo has spoken. :-) Let us proceed with the patch review. >=20 > > - (define (install-file? file stat) > > - (let ((stripped-file (string-trim (string-drop file > > (string-length source)) #\/))) > > - (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))))) =20 >=20 > 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? Here is a real example of what the output can look like: error: No files found to install. info: considering installing .gitignore info: considering installing .travis.yml info: considering installing Cask info: considering installing README.md info: considering installing Rakefile info: considering installing minitest.el info: minitest.el included as it matches "^[^/]*\.el$" info: minitest.el excluded as it matches "^[^/]*tests?\.el$" info: considering installing tools/snippet_extractor.rb info: considering installing test/minitest-unit-test.el info: considering installing test/test-helper.el info: considering installing snippets/minitest-mode/assert info: considering installing snippets/minitest-mode/assert_empty info: considering installing snippets/minitest-mode/assert_equal info: considering installing snippets/minitest-mode/assert_in_delta info: considering installing snippets/minitest-mode/assert_in_epsilon info: considering installing snippets/minitest-mode/assert_includes info: considering installing snippets/minitest-mode/assert_instance_of info: considering installing snippets/minitest-mode/assert_kind_of info: considering installing snippets/minitest-mode/assert_match info: considering installing snippets/minitest-mode/assert_nil info: considering installing snippets/minitest-mode/assert_operator info: considering installing snippets/minitest-mode/assert_output info: considering installing snippets/minitest-mode/assert_predicate info: considering installing snippets/minitest-mode/assert_raises info: considering installing snippets/minitest-mode/assert_respond_to info: considering installing snippets/minitest-mode/assert_same info: considering installing snippets/minitest-mode/assert_send info: considering installing snippets/minitest-mode/assert_silent info: considering installing snippets/minitest-mode/assert_throws info: considering installing snippets/minitest-mode/flunk info: considering installing snippets/minitest-mode/pass info: considering installing snippets/minitest-mode/refute info: considering installing snippets/minitest-mode/refute_empty info: considering installing snippets/minitest-mode/refute_equal info: considering installing snippets/minitest-mode/refute_in_delta info: considering installing snippets/minitest-mode/refute_in_epsilon info: considering installing snippets/minitest-mode/refute_includes info: considering installing snippets/minitest-mode/refute_instance_of info: considering installing snippets/minitest-mode/refute_kind_of info: considering installing snippets/minitest-mode/refute_match info: considering installing snippets/minitest-mode/refute_nil info: considering installing snippets/minitest-mode/refute_operator info: considering installing snippets/minitest-mode/refute_predicate info: considering installing snippets/minitest-mode/refute_respond_to info: considering installing snippets/minitest-mode/refute_same info: considering installing snippets/minitest-mode/skip > ... continued below >=20 > > + (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))) =20 >=20 > 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?'. >=20 > WDYT? I put this in install-file? as I didn't want to duplicate the behaviour inside the function, firstly to avoid duplication, but also to avoid the possiblily that if they were duplicated, they would diverge in behaviour. I'm happy to experiment with splitting the "verbose" output out of install-file though? > > + #f) > > + (begin > > + (for-each > > + (lambda (file) > > + (let* ((stripped-file (string-drop file > > (string-length source))) > > + (target-file (string-append target-directory > > stripped-file))) > > + (format #t "`~a' -> `~a'~%" file target-file) > > + (install-file file (dirname target-file)))) > > + files-to-install) > > + #t)))) =20 >=20 > 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. I tried this, but it seems that cond will treat '() as if it evaluates to true: scheme@(guile-user)> (cond ('() #t) (else #f)) $1 =3D #t So this might need to still use null?, or even (not (null? files-to-install))? --Sig_/v=kqHcmqDwP8nAmHjJ95Coq Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlmogpBfFIAAAAAALgAo aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE 9XcSchAAqqIrppCNK4BO6rQZ7tltlr0WyHZJyOs0eHlhP/y3Bw0WLWJovOpCWHOb bG961ZClu8L1kbRUSqFwUY1xRCU9/LExo50ScT1MEdrB1xzjOVJrrKlJwyDKqvfu ltnUbGTB93CcNS+6Q3I41W2NotkFfi/7tIYmMYBXTELjcAlRmU9lZvwftpPbcKcI uyIVJicOFd1Oj2yBXdLxjLn9YDl79USs4N70IHZht7YvXsMD5kGdJxrxtVrsSkwq LpyzRxWvc98DBcz91Jc8oz3ZKrN3vYYqd7s7aBgiTO06hxKPwHj7aa12/tUUff+2 lo+KAEXWyMXOEsH3gFlhJ94LjYNUcwU4p+ztD8Wy1djYanR5rN3OW8HAqWcZ+3+M ikfV22FgKSrWvC6B56fhC6uiIS26ncJ3W1FEgPlM2M0fa9wBfZ9Q+CEFrB9oJJ/B LxtH7EcluHxsBe9e1WKZ9ofEWCLl2ozKZgQdQSmTqwMvrDK6SkG7Mx6OerSn0VXe gMH8Fd8UNUCD9CUy019gDPy5PTfIY3w1jmwwpvigW9eNt/D76AeI6FPA39Vb4zDV VkAyW0kbUpTHOjOQnWzCLUQpqw14/7JFFvy15csVruyMDFKNQle1HjCsyWW5KLu8 RFNsRXoEMnr9abAIcLCFlOhjqIuFSCJK4AHh2WsWCGhyOGM+8m0= =FaQU -----END PGP SIGNATURE----- --Sig_/v=kqHcmqDwP8nAmHjJ95Coq--