[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

Modify the install phase to detect when nothing has been installed, and error
if this happens. This is preferable to continuing, and allowing the next phase
to fail.

Also, when nothing can be found to be installed, print out each file that was
considered, along with the regular expressions that were used to include and
exclude it.

* gnu/build/emacs-build-system.scm (install-file?): Add additional error
  checking and logging.
---
 guix/build/emacs-build-system.scm | 45 ++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index bda699ddf..d67d7b49c 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -110,22 +110,41 @@ store in '.el' files."
 
   (define source (getcwd))
 
-  (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)))))
 
   (let* ((out (assoc-ref outputs "out"))
          (elpa-name-ver (store-directory->elpa-name-version out))
-         (target-directory (string-append out %install-suffix "/" elpa-name-ver)))
-    (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))))
-     (find-files source install-file?)))
-  #t)
+         (target-directory (string-append out %install-suffix "/" elpa-name-ver))
+         (files-to-install (find-files source install-file?)))
+    (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)))
+          #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))))
 
 (define* (move-doc #:key outputs #:allow-other-keys)
   "Move info files from the ELPA package directory to the info directory."
-- 
2.13.1

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Arun Isaac]

Christopher Baines writes:

> Modify the install phase to detect when nothing has been installed, and error
> if this happens. This is preferable to continuing, and allowing the next phase
> to fail.
>
> Also, when nothing can be found to be installed, print out each file that was
> considered, along with the regular expressions that were used to include and
> exclude it.
>
> * gnu/build/emacs-build-system.scm (install-file?): Add additional error
>   checking and logging.
> ---
>  guix/build/emacs-build-system.scm | 45 ++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 13 deletions(-)

I feel that this adds a lot of complexity (lines of code) to the
emacs-build-system checking for an error that can be quite easily
identified and fixed otherwise.

WDYT? Maybe, others can comment on this as well.

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Jelle Licht]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

2017-08-29 8:25 GMT+02:00 Arun Isaac <arunisaac@systemreboot.net>:

>
> Christopher Baines writes:
>
> > Modify the install phase to detect when nothing has been installed, and
> error
> > if this happens. This is preferable to continuing, and allowing the next
> phase
> > to fail.
> >
> > Also, when nothing can be found to be installed, print out each file
> that was
> > considered, along with the regular expressions that were used to include
> and
> > exclude it.
> >
> > * gnu/build/emacs-build-system.scm (install-file?): Add additional error
> >   checking and logging.
> > ---
> >  guix/build/emacs-build-system.scm | 45 ++++++++++++++++++++++++++++--
> ---------
> >  1 file changed, 32 insertions(+), 13 deletions(-)
>
> I feel that this adds a lot of complexity (lines of code) to the
> emacs-build-system checking for an error that can be quite easily
> identified and fixed otherwise.
>
> WDYT? Maybe, others can comment on this as well.
>
>
>
One the one hand, I agree with Arun, though errors in Guix can be a bit
intimidating for newcomers.
Do we want to focus on clear and correct error messages over concise code?

[-- Attachment #2: Type: text/html, Size: 1627 bytes --]

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Tue, 29 Aug 2017 11:55:08 +0530
Arun Isaac <arunisaac@systemreboot.net> wrote:

> Christopher Baines writes:
> 
> > Modify the install phase to detect when nothing has been installed,
> > and error if this happens. This is preferable to continuing, and
> > allowing the next phase to fail.
> >
> > Also, when nothing can be found to be installed, print out each
> > file that was considered, along with the regular expressions that
> > were used to include and exclude it.
> >
> > * gnu/build/emacs-build-system.scm (install-file?): Add additional
> > error checking and logging.
> > ---
> >  guix/build/emacs-build-system.scm | 45
> > ++++++++++++++++++++++++++++----------- 1 file changed, 32
> > insertions(+), 13 deletions(-)  
> 
> I feel that this adds a lot of complexity (lines of code) to the
> emacs-build-system checking for an error that can be quite easily
> identified and fixed otherwise.
> 
> WDYT? Maybe, others can comment on this as well.

In my personal experience, I didn't find this easy to identify and fix.
For packaging emacs-minitest, I ended up writing this to pin down why
the emacs-build-system wasn't installing the key file.

I think validating that something has been installed is really
important, as otherwise the later phases fail in a very unclear way.

The extra functionality about explaining why each file hasn't been
installed is useful for debugging, and I agree that it adds significant
complexity.

But, I'd like for packaging emacs things to be really easy in the
general case, and I think making the build system more helpful when it
fails is one way to improve this. I wouldn't like to expect that you'd
need to read the implementation of the build system, or add in your own
debugging code just to package a emacs module.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Arun Isaac]

Jelle Licht writes:

> 2017-08-29 8:25 GMT+02:00 Arun Isaac <arunisaac@systemreboot.net>:
>
>>
>> Christopher Baines writes:
>>
>> Modify the install phase to detect when nothing has been installed,
>> and error if this happens. This is preferable to continuing, and
>> allowing the next phase to fail.
>>
>> Also, when nothing can be found to be installed, print out each
>> file that was considered, along with the regular expressions that
>> were used to include and exclude it.
>> >
>> > * gnu/build/emacs-build-system.scm (install-file?): Add additional error
>> >   checking and logging.
>> > ---
>> >  guix/build/emacs-build-system.scm | 45 ++++++++++++++++++++++++++++--
>> ---------
>> >  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> I feel that this adds a lot of complexity (lines of code) to the
>> emacs-build-system checking for an error that can be quite easily
>> identified and fixed otherwise.
>>
>> WDYT? Maybe, others can comment on this as well.
>>
> One the one hand, I agree with Arun, though errors in Guix can be a bit
> intimidating for newcomers.

> Do we want to focus on clear and correct error messages over concise code?

It's a design choice, and I am unable to make up my mind. I think
somebody with more experience than me should take a call.

Let's ask Ludo. CCing him...

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Ludovic Courtès]

Hello!

Christopher Baines <mail@cbaines.net> skribis:

> On Tue, 29 Aug 2017 11:55:08 +0530
> Arun Isaac <arunisaac@systemreboot.net> wrote:
>
>> Christopher Baines writes:
>> 
>> > Modify the install phase to detect when nothing has been installed,
>> > and error if this happens. This is preferable to continuing, and
>> > allowing the next phase to fail.
>> >
>> > Also, when nothing can be found to be installed, print out each
>> > file that was considered, along with the regular expressions that
>> > were used to include and exclude it.
>> >
>> > * gnu/build/emacs-build-system.scm (install-file?): Add additional
>> > error checking and logging.
>> > ---
>> >  guix/build/emacs-build-system.scm | 45
>> > ++++++++++++++++++++++++++++----------- 1 file changed, 32
>> > insertions(+), 13 deletions(-)  
>> 
>> I feel that this adds a lot of complexity (lines of code) to the
>> emacs-build-system checking for an error that can be quite easily
>> identified and fixed otherwise.
>> 
>> WDYT? Maybe, others can comment on this as well.
>
> In my personal experience, I didn't find this easy to identify and fix.
> For packaging emacs-minitest, I ended up writing this to pin down why
> the emacs-build-system wasn't installing the key file.
>
> I think validating that something has been installed is really
> important, as otherwise the later phases fail in a very unclear way.
>
> The extra functionality about explaining why each file hasn't been
> installed is useful for debugging, and I agree that it adds significant
> complexity.

I agree.  I’m guessing you wrote this after spending a while debugging a
build, despite being experienced with Guix, which to me suggests that
this is a welcome improvement, in spite of the extra complexity.

> But, I'd like for packaging emacs things to be really easy in the
> general case, and I think making the build system more helpful when it
> fails is one way to improve this. I wouldn't like to expect that you'd
> need to read the implementation of the build system, or add in your own
> debugging code just to package a emacs module.

Sounds reasonable.

To me this looks like a step in the right direction.

Thanks,
Ludo’.

PS: Thanks Arun for pinging me.  :-)

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> Modify the install phase to detect when nothing has been installed, and error
> if this happens. This is preferable to continuing, and allowing the next phase
> to fail.
>
> Also, when nothing can be found to be installed, print out each file that was
> considered, along with the regular expressions that were used to include and
> exclude it.
>
> * gnu/build/emacs-build-system.scm (install-file?): Add additional error
>   checking and logging.

Nitpicking:

> +  (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))

Use ‘when’ here, to clarify that this is for-effect.

Ludo’.

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

Modify the install phase to detect when nothing has been installed, and error
if this happens. This is preferable to continuing, and allowing the next phase
to fail.

Also, when nothing can be found to be installed, print out each file that was
considered, along with the regular expressions that were used to include and
exclude it.

* gnu/build/emacs-build-system.scm (install-file?): Add additional error
  checking and logging.
---
 guix/build/emacs-build-system.scm | 45 ++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index bda699ddf..ceaa085ce 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -110,22 +110,41 @@ store in '.el' files."
 
   (define source (getcwd))
 
-  (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)))
+          (when (and result verbose?)
+                (format #t "info: ~A ~A as it matches \"~A\"\n"
+                        stripped-file action regex))
+          result))
+
+      (when 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)))))
 
   (let* ((out (assoc-ref outputs "out"))
          (elpa-name-ver (store-directory->elpa-name-version out))
-         (target-directory (string-append out %install-suffix "/" elpa-name-ver)))
-    (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))))
-     (find-files source install-file?)))
-  #t)
+         (target-directory (string-append out %install-suffix "/" elpa-name-ver))
+         (files-to-install (find-files source install-file?)))
+    (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)))
+          #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))))
 
 (define* (move-doc #:key outputs #:allow-other-keys)
   "Move info files from the ELPA package directory to the info directory."
-- 
2.14.1

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 2462 bytes --]

On Tue, 29 Aug 2017 23:46:00 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Hello!
> 
> Christopher Baines <mail@cbaines.net> skribis:
> 
> > On Tue, 29 Aug 2017 11:55:08 +0530
> > Arun Isaac <arunisaac@systemreboot.net> wrote:
> >  
> >> Christopher Baines writes:
> >>   
> >> > Modify the install phase to detect when nothing has been
> >> > installed, and error if this happens. This is preferable to
> >> > continuing, and allowing the next phase to fail.
> >> >
> >> > Also, when nothing can be found to be installed, print out each
> >> > file that was considered, along with the regular expressions that
> >> > were used to include and exclude it.
> >> >
> >> > * gnu/build/emacs-build-system.scm (install-file?): Add
> >> > additional error checking and logging.
> >> > ---
> >> >  guix/build/emacs-build-system.scm | 45
> >> > ++++++++++++++++++++++++++++----------- 1 file changed, 32
> >> > insertions(+), 13 deletions(-)    
> >> 
> >> I feel that this adds a lot of complexity (lines of code) to the
> >> emacs-build-system checking for an error that can be quite easily
> >> identified and fixed otherwise.
> >> 
> >> WDYT? Maybe, others can comment on this as well.  
> >
> > In my personal experience, I didn't find this easy to identify and
> > fix. For packaging emacs-minitest, I ended up writing this to pin
> > down why the emacs-build-system wasn't installing the key file.
> >
> > I think validating that something has been installed is really
> > important, as otherwise the later phases fail in a very unclear way.
> >
> > The extra functionality about explaining why each file hasn't been
> > installed is useful for debugging, and I agree that it adds
> > significant complexity.  
> 
> I agree.  I’m guessing you wrote this after spending a while
> debugging a build, despite being experienced with Guix, which to me
> suggests that this is a welcome improvement, in spite of the extra
> complexity.
> 
> > But, I'd like for packaging emacs things to be really easy in the
> > general case, and I think making the build system more helpful when
> > it fails is one way to improve this. I wouldn't like to expect that
> > you'd need to read the implementation of the build system, or add
> > in your own debugging code just to package a emacs module.  
> 
> Sounds reasonable.
> 
> To me this looks like a step in the right direction.

Ok, thanks for your review Ludo :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On Tue, 29 Aug 2017 23:50:07 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Christopher Baines <mail@cbaines.net> skribis:
> 
> > Modify the install phase to detect when nothing has been installed,
> > and error if this happens. This is preferable to continuing, and
> > allowing the next phase to fail.
> >
> > Also, when nothing can be found to be installed, print out each
> > file that was considered, along with the regular expressions that
> > were used to include and exclude it.
> >
> > * gnu/build/emacs-build-system.scm (install-file?): Add additional
> > error checking and logging.  
> 
> Nitpicking:
> 
> > +  (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))  
> 
> Use ‘when’ here, to clarify that this is for-effect.

I've sent an updated patch which uses when.

One final issue, as this changes the build system, every package that
uses it will need to be rebuilt. My count puts this at 162, which
suggests that this is fine to go straight on the master branch.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Arun Isaac]

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 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)))))

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 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))))

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.

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> On Tue, 29 Aug 2017 23:50:07 +0200
> ludo@gnu.org (Ludovic Courtès) wrote:
>
>> Christopher Baines <mail@cbaines.net> skribis:
>> 
>> > Modify the install phase to detect when nothing has been installed,
>> > and error if this happens. This is preferable to continuing, and
>> > allowing the next phase to fail.
>> >
>> > Also, when nothing can be found to be installed, print out each
>> > file that was considered, along with the regular expressions that
>> > were used to include and exclude it.
>> >
>> > * gnu/build/emacs-build-system.scm (install-file?): Add additional
>> > error checking and logging.  
>> 
>> Nitpicking:
>> 
>> > +  (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))  
>> 
>> Use ‘when’ here, to clarify that this is for-effect.
>
> I've sent an updated patch which uses when.

LGTM!

> One final issue, as this changes the build system, every package that
> uses it will need to be rebuilt. My count puts this at 162, which
> suggests that this is fine to go straight on the master branch.

Definitely, and these are very small packages anyway.

Ludo’.

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 6490 bytes --]

On Wed, 30 Aug 2017 13:37:49 +0530
Arun Isaac <arunisaac@systemreboot.net> wrote:

> 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 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)))))  
> 
> 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
> 
> > +    (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?

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))))  
> 
> 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 = #t

So this might need to still use null?, or even (not (null?
files-to-install))?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28185] [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Arun Isaac]

>> > +    (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?
>
> 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?

Yes, I meant that you should split the verbose output out of
`install-file?'. But, it's not a big deal. I'm only nitpicking. Do go
ahead with your original code if you think it's alright.

>> > +          #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))))
>>
>> 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 = #t
>
> So this might need to still use null?, or even (not (null?
> files-to-install))?

Ah, yes. Sorry, I'm confusing scheme's behaviour with that of other
lisps. We still need `null?'.

The patch LGTM.

bug#28185: [PATCH] build: emacs-build-system: Make the install phase more helpful.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]

On Fri, 01 Sep 2017 10:32:18 +0530
Arun Isaac <arunisaac@systemreboot.net> wrote:

> >> > +    (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?  
> >
> > 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?  
> 
> Yes, I meant that you should split the verbose output out of
> `install-file?'. But, it's not a big deal. I'm only nitpicking. Do go
> ahead with your original code if you think it's alright.
> 
> >> > +          #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))))  
> >>
> >> 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 = #t
> >
> > So this might need to still use null?, or even (not (null?
> > files-to-install))?  
> 
> Ah, yes. Sorry, I'm confusing scheme's behaviour with that of other
> lisps. We still need `null?'.
> 
> The patch LGTM.

Great, I've pushed this now :)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]