From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34970) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f7OPH-0006yU-AB for guix-patches@gnu.org; Sat, 14 Apr 2018 12:52:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f7OPC-0000fC-OF for guix-patches@gnu.org; Sat, 14 Apr 2018 12:52:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:44646) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1f7OPC-0000es-Hq for guix-patches@gnu.org; Sat, 14 Apr 2018 12:52:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1f7OPC-0003Do-5h for guix-patches@gnu.org; Sat, 14 Apr 2018 12:52:02 -0400 Subject: [bug#31018] [PATCH] Improvements for our Emacs build system and fixes. Resent-Message-ID: From: Arun Isaac In-Reply-To: <87y3i6538o.fsf@gmail.com> References: <87y3i6538o.fsf@gmail.com> Date: Sat, 14 Apr 2018 22:21:10 +0530 Message-ID: MIME-Version: 1.0 Content-Type: text/plain List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Maxim Cournoyer , 31018@debbugs.gnu.org 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 > 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?