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

guix-patches

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From:	Maxim Cournoyer
Subject:	[bug#31018] [PATCHv2] Improvements for our Emacs build system and fixes.
Date:	Mon, 16 Apr 2018 22:59:41 -0400
User-agent:	Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux)

Hello Arun,

Arun Isaac <address@hidden> writes:

> 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 (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.

I wanted to stress that we pass the input directory rather than a Guix
input (which is tuple of a package's name and its path in the store),
since both are used in this module. I've now made this more consistent.

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

I like it as a function better, if only for the expressiveness of its
name :).

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

I tried this before; there are two issues with that method:
1. Cluttering EMACSLOADPATH: Since we must support both
"/share/emacs/site-lisp" and "/share/emacs/site-lisp/guix.d", the first
would always be added when the later exists, which we don't want. This
is because we can only pass a PATTERN to `set-path-environment-variable'
that is tested against every file which is of the specified TYPE (we
cannot pass it some extra logic).
2. It is important for EMACSLOADPATH to contain the trailing colon (:),
otherwise Emacs wouldn't find its own Elisp modules. We could hack it at
this level by adding nil to the end of the INPUT-DIRS, but this isn't
general. In the past I played with attempting to define a
search-path-specification for our Emacs package, which would generalize
even more the way our Emacs packages are discovered in Guix. That ended
up looking like the following:

--8<---------------cut here---------------start------------->8---
+    (native-search-paths
+     (list (search-path-specification
+            (variable "EMACSLOADPATH")
+            (files '("share/emacs/site-lisp"))
+            (file-pattern ".*")
+            (append-separator? #t))))))
--8<---------------cut here---------------end--------------->8---

Where the new "append-separator?" argument was added by myself (this is
a world rebuilding change and also changes the manifest file). I didn't
pursue that for now given that it still suffers from 1. explained above.

The nice properties of this however was that it was now possible to have
the Emacs dependencies found in `guix environment' as well as in the
build system (anywhere), by using the native mechanism that Guix comes
with. If you have interest in going that way I could revive those two
old patches.

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

Done.

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

Done (although I find it a bit verbose now :).

>> +  (let* ((test-program (car test-command))
>> +         (test-args (cdr test-command))
>
> Use match-let here instead of car and cdr.

Done, although match-let appears to be undocumented :/.

>
>>      (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.

Done.

[...]

>>  (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.

Done.

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

Done.

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

Done.

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

Done.

>>         (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.

I believe it already does, looking at the definition of substitute*
(which in turn calls substitute).

>> +       `(#:tests? #t
>> +         #:test-command '("emacs" "-batch"
>
> Nitpick: Use "--batch" here instead of "-batch".

Done.

>> +                          "-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.

Done.

>>  (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".

Done.

>> +                        "-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.

Good point, but memoize-test.el does not have the necessary (require
'memoize) statement required for it to work.

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

Done.

>
>>      (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.

Good point. Done.

>> +     `(#: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?

Removed.

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

I'm not sure :). Corrected to set-emacs-load-path.

>> Subject: [PATCH 12/27] gnu: emacs-idris-mode: Fix hash.
>>
>>         (sha256
>>          (base32
>> -         "0ld4kfwnyyhlsnj5f6cbn4is4mpxdqalk2aifkw02r00mbr9n294"))))
>> +         "0qm4gngl6lqvra7kmivz99y3ymrg36nvqh5y9gy6gq0aa0v8az46"))))
>
> The hash seems to have changed again upstream. Please verify.

Indeed, it is now
02r1qqsxi6qk7q4cj6a6pygbj856dcw9vcmhfh0ib92j41v77q6y. Fixed.

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

I might be possible, but given that the latest v0.4 release dates back
from Jul 18 2015, I think we're better off using a recent git commit
anyway.

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

I didn't, on purpose. I wanted the Elisp files to be processed the same
as most of our other Emacs packages (with the patch-el-files phase
patching binaries, for example) that we would miss otherwise).

>> From 9a3a449121246a2d26a92ee5c19d905768789a10 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> 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.

Done.

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

No, for the same reasons explained above for the emacs-scel package.

A big thank you for reviewing this lengthy set of patches :)

You'll find the edited patches attached.

Maxim