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

[Christopher Baines]

On Wed, 30 Aug 2017 13:37:49 +0530
Arun Isaac <address@hidden> 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))?

pgp7FTVDj_snh.pgp
Description: OpenPGP digital signature