[bug#31018] [PATCH] Improvements for our Emacs build system and fixes.

[Arun Isaac <arunisaac@systemreboot.net>]

I've pushed the patches concerning ert-expectations, org-trello,
smartparens, request, polymode, mu4e-alert, evil-matchit, spark,
es-mode, eimp, dired-hacks and cdlatex to master with some minor
modifications here and there. I was careful not to break any packages on
master.

For the remaining patches, some comments follow.

> Subject: [PATCH 01/27] emacs-build-system: Consider all inputs for
> Elisp dependencies.

>  (define* (set-emacs-load-path #:key inputs #:allow-other-keys)
>    "Set the EMACSLOADPATH environment variable so that dependencies are found."
> -  (let* ((input-elisp-dirs (emacs-inputs-el-directories
> -                            (emacs-inputs-directories inputs)))
> +  (let* ((input-elisp-dirs (inputs->el-directories
> +                            (inputs->directories inputs)))
>           (emacs-load-path-value (string-join
>                                   input-elisp-dirs ":" 'suffix)))
>      (setenv "EMACSLOADPATH" emacs-load-path-value)

> @@ -210,39 +210,25 @@ store in '.el' files."
>    "Check if NAME correspond to the name of an Emacs package."
>    (string-prefix? "emacs-" name))
>  
> -(define (emacs-inputs inputs)
> -  "Retrieve the list of Emacs packages from INPUTS."
> -  (filter (match-lambda
> -            ((label . directory)
> -             (emacs-package? ((compose package-name->name+version
> -                                       strip-store-file-name)
> -                              directory)))
> -            (_ #f))
> -          inputs))
> +(define (inputs->directories inputs)
> +  "Extract the directory part from INPUTS."
> +  (match inputs
> +    (((names . directories) ...) directories)))
>  
> -(define (emacs-inputs-directories inputs)
> -  "Extract the list of Emacs package directories from INPUTS."
> -  (let ((inputs (emacs-inputs inputs)))
> -    (match inputs
> -      (((names . directories) ...) directories))))
> -
> -(define (emacs-input->el-directory emacs-input)
> -  "Return the correct Elisp directory location of EMACS-INPUT or #f if none."
> -  (let ((legacy-elisp-dir (string-append emacs-input %legacy-install-suffix))
> +(define (input->el-directory input-dir)
> +  "Return the correct Elisp directory location of INPUT-DIR-DIR or #f."

We can just call the "input-dir" argument as "input". Also, there is a
typo in the docstring where it is called INPUT-DIR-DIR.

> -(define (emacs-inputs-el-directories dirs)
> -  "Build the list of Emacs Lisp directories from the Emacs package directory
> -DIRS."
> -  (filter-map emacs-input->el-directory dirs))
> +(define (inputs->el-directories input-dirs)
> +  "Build the list of Emacs Lisp directories from the INPUT-DIRS."
> +  (filter-map input->el-directory input-dirs))

emacs-inputs-el-directories is a very short and simple function, and is
called only once. Why not insert its body directly into the caller
function?

> Subject: [PATCH 02/27] emacs-build-system: Add improved check phase, fixes.

> -(define* (set-emacs-load-path #:key inputs #:allow-other-keys)
> +(define* (set-emacs-load-path #:key source inputs #:allow-other-keys)
>    "Set the EMACSLOADPATH environment variable so that dependencies are found."
> -  (let* ((input-elisp-dirs (inputs->el-directories
> +  (let* ((source-dir (getcwd))
> +         (input-elisp-dirs (inputs->el-directories
>                              (inputs->directories inputs)))
>           (emacs-load-path-value (string-join
> -                                 input-elisp-dirs ":" 'suffix)))
> +                                 `(,@input-elisp-dirs
> +                                   ,source-dir) ":" 'suffix)))
>      (setenv "EMACSLOADPATH" emacs-load-path-value)

set-path-environment-variable could be used here instead of setenv. It
will make the code shorter. Also, modifications to the
set-emacs-load-path phase should be part of the first commit (Consider all
inputs for elisp dep). They don't belong in this commit along with the
check phase.

>      (format #t "environment variable `EMACSLOADPATH' set to ~a\n"
>              emacs-load-path-value)))
> @@ -133,6 +135,24 @@ store in '.el' files."
>            (substitute-program-names))))
>      #t))
>  
> +(define* (check #:key target (tests? (not target))
> +                (test-command '("make" "check")) (parallel-tests? #t)
> +                #:allow-other-keys)
> +  "Like the gnu-build-system check phase, but with a way to pass a custom test
> +program command line, using TEST-COMMAND."

I think this docstring should explicitly say what the check phase does,
instead of making a comparison to the gnu-build-system. WDYT?

> +  (let* ((test-program (car test-command))
> +         (test-args (cdr test-command))

Use match-let here instead of car and cdr.

>      (parameterize ((%emacs emacs))
> -      (emacs-generate-autoloads elpa-name el-dir))
> -    #t))
> +      (emacs-generate-autoloads elpa-name el-dir))))

These invoke related changes should be a separate commit.

>  (define-module (guix build emacs-utils)
> +  #:use-module (guix build utils)
>    #:export (%emacs
>              emacs-batch-eval
>              emacs-batch-edit-file
> @@ -39,16 +41,14 @@
>  
>  (define (emacs-batch-eval expr)
>    "Run Emacs in batch mode, and execute the elisp code EXPR."
> -  (unless (zero? (system* (%emacs) "--quick" "--batch"
> -                          (format #f "--eval=~S" expr)))
> -    (error "emacs-batch-eval failed!" expr)))
> +  (invoke (%emacs) "--quick" "--batch"
> +          (format #f "--eval=~S" expr)))

Likewise.

>  (define (emacs-batch-edit-file file expr)
>    "Load FILE in Emacs using batch mode, and execute the elisp code EXPR."
> -  (unless (zero? (system* (%emacs) "--quick" "--batch"
> -                          (string-append "--visit=" file)
> -                          (format #f "--eval=~S" expr)))
> -    (error "emacs-batch-edit-file failed!" file expr)))
> +  (invoke (%emacs) "--quick" "--batch"
> +          (string-append "--visit=" file)
> +          (format #f "--eval=~S" expr)))

Likewise.

>  (define (emacs-generate-autoloads name directory)
>    "Generate autoloads for Emacs package NAME placed in DIRECTORY."
> @@ -60,7 +60,9 @@
>  
>  (define* (emacs-byte-compile-directory dir)
>    "Byte compile all files in DIR and its sub-directories."
> -  (let ((expr `(byte-recompile-directory (file-name-as-directory ,dir) 0)))
> +  (let ((expr `(progn
> +                (setq byte-compile-debug t) ;for proper exit status
> +                (byte-recompile-directory (file-name-as-directory ,dir) 0 1))))
>      (emacs-batch-eval expr)))

Strict byte compilation should be a separate commit.

> Subject: [PATCH 03/27] gnu: Adjust ert-runner wrapper to honor EMACSLOADPATH.
>
>                   (wrap-program (string-append out "/bin/ert-runner")
> -                   (list "EMACSLOADPATH" ":" '=
> +                   (list "EMACSLOADPATH" ":" 'prefix
>                           (append
>                            ,(match dependencies
>                               (((labels packages) ...)

Couldn't we just use the EMACSLOADPATH environment variable as already
set by the set-emacs-load-path phase, instead of recomputing it?

> Subject: [PATCH 04/27] gnu: Adapt Emacs packages to use the new check phase.
>
> -     `(#:phases
> -       (modify-phases %standard-phases
> -         (add-before 'install 'check
> -                     (lambda _
> -                       (zero? (system* "./run-tests.sh")))))))
> +     `(#:tests? #t
> +       #:test-command '("sh" "run-tests.sh")))

Why '("sh" "run-tests.sh") and not just '("./run-tests.sh")?

> +     `(#:tests? #t
> +       #:test-command '("sh" "run-tests.sh")))

Likewise.

>         (modify-phases %standard-phases
> -         (add-before 'install 'check
> +         (add-before 'check 'fix-bin-dir
>             (lambda _
>               ;; The company-files-candidates-normal-root test looks
>               ;; for the /bin directory, but the build environment has
>               ;; no /bin directory. Modify the test to look for the
>               ;; /tmp directory.
>               (substitute* "test/files-tests.el"
> -               (("/bin/") "/tmp/"))
> -             (zero? (system* "make" "test-batch")))))))
> +               (("/bin/") "/tmp/")))))

The fix-bin-dir phase must return a #t.

> +       `(#:tests? #t
> +         #:test-command '("emacs" "-batch"

Nitpick: Use "--batch" here instead of "-batch".

> +                          "-l" "julia-mode.el"
> +                          "-l" "julia-mode-tests.el"
> +                          "-f" "ert-run-tests-batch-and-exit")))

julia-mode.el need not be loaded explicitly. The EMACSLOADPATH
environment variable knows where to find it.

>  (define-public emacs-memoize
>    (package
> -   (name "emacs-memoize")
> -   (version "20130421.b55eab0")
> -   (source
> -    (origin
> -     (method git-fetch)
> -     (uri (git-reference
> -           (url "https://github.com/skeeto/emacs-memoize")
> -           (commit "b55eab0cb6ab05d941e07b8c01f1655c0cf1dd75")))
> -     (file-name (string-append name "-" version ".tar.gz"))
> -     (sha256
> -      (base32
> -       "0fjwlrdm270qcrqffvarw5yhijk656q4lam79ybhaznzj0dq3xpw"))))
> -   (build-system emacs-build-system)
> -   (arguments
> -    `(#:phases
> -      (modify-phases %standard-phases
> -        (add-before 'install 'check
> -                    (lambda _
> -                      (zero? (system* "emacs" "-batch" "-l" "memoize.el"
> -                                      "-l" "memoize-test.el"
> -                                      "-f" "ert-run-tests-batch-and-exit")))))))
> -   (home-page "https://github.com/skeeto/emacs-memoize")
> -   (synopsis "Emacs lisp memoization library")
> -   (description "@code{emacs-memoize} is an Emacs library for
> +    (name "emacs-memoize")
> +    (version "20130421.b55eab0")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/skeeto/emacs-memoize")
> +             (commit "b55eab0cb6ab05d941e07b8c01f1655c0cf1dd75")))
> +       (file-name (string-append name "-" version ".tar.gz"))
> +       (sha256
> +        (base32
> +         "0fjwlrdm270qcrqffvarw5yhijk656q4lam79ybhaznzj0dq3xpw"))))
> +    (build-system emacs-build-system)
> +    (arguments
> +     `(#:tests? #t
> +       #:test-command '("emacs" "-batch"

Nitpick: Use "--batch" here instead of "-batch".

> +                        "-l" "memoize.el"
> +                        "-l" "memoize-test.el"
> +                        "-f" "ert-run-tests-batch-and-exit")))

memoize.el need not be loaded explicitly. The EMACSLOADPATH environment
variable knows where to find it.

> +    (home-page "https://github.com/skeeto/emacs-memoize")
> +    (synopsis "Emacs lisp memoization library")
> +    (description "@code{emacs-memoize} is an Emacs library for
>  memoizing functions.")
> -   (license license:unlicense)))
> +    (license license:unlicense)))

Re-indent emacs-memoize as a separate commit. Else, it's very hard to
make sense of the diff.

>      (arguments
> -     `(#:phases
> -       (modify-phases %standard-phases
> -         (add-before 'install 'check
> -           (lambda _
> -             (zero? (system* "emacs" "--batch" "-L" "."
> -                             "-l" "xmlgen-test.el"
> -                             "-f" "ert-run-tests-batch-and-exit")))))))
> +     `(#:tests? #t
> +       #:test-command '("emacs" "--batch" "-L" "."
> +                        "-l" "xmlgen-test.el"
> +                        "-f" "ert-run-tests-batch-and-exit")))

"-L" "." is not required. The EMACSLOADPATH environment variable already
includes the current directory.

> +     `(#:tests? #t
> +       #:test-command '("emacs" "--batch"
> +                        "-l" "which-key-tests.el"
> +                        "-f" "ert-run-tests-batch-and-exit")))

>      (home-page "https://github.com/lewang/ws-butler")
>      (synopsis "Trim spaces from end of lines")
>      (description
> @@ -6480,20 +6431,14 @@ editing RPM spec files.")
>          (base32
>           "17mqki6g0wx46fn7dcbcc2pjxik7vvrcb1j9jzxim8b9psbsbnp9"))))
>      (build-system emacs-build-system)
> +    (native-inputs
> +     `(("ert-runner" ,ert-runner)))

Why add ert-runner as native-input?

> Subject: [PATCH 05/27] gnu: emacs-pdf-tools: Fix byte compilation.
>
> -         (add-after 'emacs-patch-variables 'emacs-install
> +         (add-after 'emacs-patch-variables 'set-emacs-load-path
> +           (assoc-ref emacs:%standard-phases 'set-emacs-load-path))
> +         (add-after 'add-cwd-to-load-path 'emacs-install

What is the add-cwd-to-load-path phase, and where is it defined?

> Subject: [PATCH 12/27] gnu: emacs-idris-mode: Fix hash.
>
>         (sha256
>          (base32
> -         "0ld4kfwnyyhlsnj5f6cbn4is4mpxdqalk2aifkw02r00mbr9n294"))))
> +         "0qm4gngl6lqvra7kmivz99y3ymrg36nvqh5y9gy6gq0aa0v8az46"))))

The hash seems to have changed again upstream. Please verify.

> Subject: [PATCH 13/27] gnu: emacs-sx: Fix build issue.
>
> The package would fail building when attempting to create a cache
> directory.  This has been fixed upstream but not in a tagged release.

Is it possible to cherry pick relevant commits alone and include them as
patches? If possible, this would be preferable to switching to a git
checkout.

> Subject: [PATCH 15/27] gnu: Add emacs-scel.
>
> +(define-public emacs-scel
> +  (let ((version "20170629")
> +        (revision "1")
> +        (commit "aeea3ad4be9306d14c3a734a4ff54fee10ac135b"))
> +    (package
> +      (name "emacs-scel")
> +      (version (git-version version revision commit))
> +      (source (origin
> +                (method git-fetch)
> +                (uri (git-reference
> +                      (url "https://github.com/supercollider/scel.git")
> +                      (commit commit)))
> +                (file-name (string-append name "-" version "-checkout"))
> +                (sha256
> +                 (base32
> +                  "0jvmzs1lsjyndqshhii2y4mnr3wghai26i3p75453zrpxpg0zvvw"))))
> +      (build-system emacs-build-system)

This package seems to use a cmake-build-system. Did you try that?

> From 9a3a449121246a2d26a92ee5c19d905768789a10 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Wed, 28 Mar 2018 21:50:15 -0400
> Subject: [PATCH 19/27] gnu: emacs-org-contrib: Fix hash and byte compilation.

There is no hash change in the commit. Please fix the commit message.

> Subject: [PATCH 26/27] gnu: Add emacs-howm.
>
> +(define-public emacs-howm
> +  (package
> +    (name "emacs-howm")
> +    (version "1.4.4")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append "http://howm.sourceforge.jp/a/howm-"
> +                                  version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "0ddm91l6z58j7x59fa966j6q1rg4cinyza4r8ibg80hprn5h31qk"))))
> +    (build-system emacs-build-system)

This package seems to use the gnu-build-system. Did you try that?