[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#31018] [PATCH] Improvements for our Emacs build system and fixes.
|
From: |
Arun Isaac |
|
Subject: |
[bug#31018] [PATCH] Improvements for our Emacs build system and fixes. |
|
Date: |
Sat, 14 Apr 2018 22:21:10 +0530 |
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 <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.
> 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?