[bug#33598] Optimizations for emacs-clang-format and emacs-clang-rename

[Tim Gesthuizen <tim.gesthuizen@yahoo.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3888 bytes --]

Hi Pierre,
thank you for reviewing!
On 06.01.19 20:00, Pierre Neidhardt wrote:
> Hi Tim,
> 
> I just reviewed your patch. Looks pretty good overall, thanks!
> 
> A few things:
> 
>> +(export package-elisp-from-package)
> 
> This should be placed at the beginning of the file in the (define-module...
> See bootstrap.scm.

As the new function can be defined with a normal lambda and not a
lambda* I just used define-public.

>> +;;; Returns a package definition that packages an emacs-lisp file from the
>> +;;; SRCPKG source. The package has the name PKGNAME and packages the file
>> +;;; SRC-FILE from the source in its root directory as TARGET-FILE or the
>> +;;; basename of SRC-FILE where INPUTS NATIVE-INPUTS and PROPAGATED-INPUTS are
>> +;;; added as package inputs and SUBSTITUTIONS substitutions will be performed
>> +;;; on the elisp file and SYNOPSIS and DESCRIPTION as the package synopsis and
>> +;;; description.
>> +(define* (package-elisp-from-package
> 
> Move the ";;;" comment to a docstring, e.g.
> 
> --8<---------------cut here---------------start------------->8---
> (define* (package-elisp-from-package
>           ...)
>   "Return ..."
> --8<---------------cut here---------------end--------------->8---

Done.

>> +;;; Returns a package definition that packages an emacs-lisp file from the
> 
> "Return", not "Returns".
> 
>> +;;; SRCPKG source. The package has the name PKGNAME and packages the file
> 
> Separate sentences with two spaces.

Done.

>> +          srcpkg pkgname src-file
> 
> Prefer complete words over abbreviations.  Here I'd suggest
> 
>   source-package
>   name
>   source-file

Done. name is called package-name.

>> +      (synopsis (if synopsis
>> +                    synopsis
>> +                    (package-synopsis srcpkg)))
>> +      (description (if description
>> +                       description
>> +                       (package-description srcpkg))))))
> 
> A more Lispy way:
> 
> --8<---------------cut here---------------start------------->8---
>  +      (synopsis (or synopsis
>  +                    (package-synopsis srcpkg)))
>  +      (description (or description
>  +                       (package-description srcpkg))))))
> --8<---------------cut here---------------end--------------->8---

Obsolete as this is now moved again to the final package definition.
Thanks for the tip :) I'm still quite new to scheme.

> Regarding the function parameters, I would turn SOURCE-FILE into SOURCE-FILES to
> make it more generic.  Indeed, the Emacs library could very well be split over
> multiple files.
> 
> One thing I'm not too sure about is the replication of the structure fields as
> keys.
> I think it's easier to ignore those and let the user define them as follows:
> 
> --8<---------------cut here---------------start------------->8---
> (define-public emacs-clang-rename
>   (package
>     (inherit (package-elisp-from-package
>              clang
>              "emacs-clang-rename"
>              "tools/clang-rename/clang-rename.el"))
>     (arguments ...)))
> --8<---------------cut here---------------end--------------->8---

I was also thinking about this. But with stuffing everything into the
function to evaluate to the final definition made multiple files
difficult as it would complicate the data structure for substitutions.
As this is not part of the function this is not a problem anymore.

Maybe we could make the function even more generic if we would just let
it modify the origin object.

> Makes sense?  This would also be more robust in case the package structure
> changes someday.
> 
> Finally, rebase your changes so that you directly use the last function, no
> need for the clang-specific function to appear in the history of commits.

Done. Patches are attached.

Tim.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-gnu-Add-package-elisp-from-package.patch --]
[-- Type: text/x-patch; name="0001-gnu-Add-package-elisp-from-package.patch", Size: 2812 bytes --]

From 738d70333c5f09d4f5914e9e524999e800f9f37a Mon Sep 17 00:00:00 2001
From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
Date: Fri, 4 Jan 2019 22:34:36 +0100
Subject: [PATCH 1/2] gnu: Add package-elisp-from-package

Add a function to generate package definitions that packages single elisp
files from other packages.

* gnu/packages/emacs.scm (package-elisp-from-package): New function
---
 gnu/packages/emacs.scm | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index df0d6144b..a70c9dd9c 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -35,7 +35,7 @@
 ;;; Copyright © 2018 Sohom Bhattacharjee <soham.bhattacharjee15@gmail.com>
 ;;; Copyright © 2018 Mathieu Lirzin <mthl@gnu.org>
 ;;; Copyright © 2018 Pierre Neidhardt <mail@ambrevar.xyz>
-;;; Copyright © 2018 Tim Gesthuizen <tim.gesthuizen@yahoo.de>
+;;; Copyright © 2018, 2019 Tim Gesthuizen <tim.gesthuizen@yahoo.de>
 ;;; Copyright © 2018 Jack Hill <jackhill@jackhill.us>
 ;;; Copyright © 2018 Pierre-Antoine Rouby <pierre-antoine.rouby@inria.fr>
 ;;; Copyright © 2018 Alex Branham <alex.branham@gmail.com>
@@ -336,6 +336,36 @@ editor (without an X toolkit)" )
              (lambda _
                (invoke "mkdir" "-p" "src/deps")))))))))
 
+(define-public (package-elisp-from-package
+                source-package package-name source-files)
+  "Returns a package definition that packages emacs-lisp files from the
+SOURCE-PACKAGEs source.  The package has the name PACKAGE-NAME and packages
+the files SOURCE-FILES from the source in its root directory."
+  (let ((orig (package-source source-package)))
+    (package
+      (inherit source-package)
+      (name package-name)
+      (build-system emacs-build-system)
+      (source (origin
+                (method (origin-method orig))
+                (uri (origin-uri orig))
+                (sha256 (origin-sha256 orig))
+                (file-name (string-append package-name "-"
+                                          (package-version source-package)))
+                (modules '((guix build utils)
+                           (srfi srfi-1)
+                           (ice-9 ftw)))
+                (snippet
+                 `(let* ((source-files (quote ,source-files))
+                         (basenames (map basename source-files)))
+                    (map copy-file
+                         source-files basenames)
+                    (map delete-file-recursively
+                         (fold delete
+                               (scandir ".")
+                               (append '("." "..") basenames)))
+                    #t)))))))
+
 \f
 ;;;
 ;;; Emacs hacking.
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.3: 0002-gnu-Use-package-elisp-from-package-for-clangs-emacs-.patch --]
[-- Type: text/x-patch; name="0002-gnu-Use-package-elisp-from-package-for-clangs-emacs-.patch", Size: 5195 bytes --]

From 012ba2f96b16d54e0e1c23ee912c8a219355216c Mon Sep 17 00:00:00 2001
From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
Date: Fri, 4 Jan 2019 22:34:56 +0100
Subject: [PATCH 2/2] gnu: Use package-elisp-from-package for clangs emacs lisp
 files

Use package-elisp-from-package for emacs-clang-format and emacs-clang-rename.
Also remove package-from-clang-elisp-file as it is not needed anymore.

* gnu/packages/llvm.scm (emacs-clang-format): Use package-elisp-from-package
* gnu/packages/llvm.scm (emacs-clang-rename): Use package-elisp-from-package
* gnu/packages/llvm.scm (package-from-clang-elisp-file): Remove function
---
 gnu/packages/llvm.scm | 65 +++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/gnu/packages/llvm.scm b/gnu/packages/llvm.scm
index 6dab9c519..32d7ef81b 100644
--- a/gnu/packages/llvm.scm
+++ b/gnu/packages/llvm.scm
@@ -8,7 +8,7 @@
 ;;; Copyright © 2018 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2018 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
-;;; Copyright © 2018 Tim Gesthuizen <tim.gesthuizen@yahoo.de>
+;;; Copyright © 2018, 2019 Tim Gesthuizen <tim.gesthuizen@yahoo.de>
 ;;; Copyright © 2018 Pierre Neidhardt <mail@ambrevar.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
@@ -39,6 +39,7 @@
   #:use-module (gnu packages gcc)
   #:use-module (gnu packages bootstrap)           ;glibc-dynamic-linker
   #:use-module (gnu packages compression)
+  #:use-module (gnu packages emacs)
   #:use-module (gnu packages libffi)
   #:use-module (gnu packages perl)
   #:use-module (gnu packages python)
@@ -481,22 +482,21 @@ code analysis tools.")
 
 (define-public emacs-clang-format
   (package
-    (inherit clang)
-    (name "emacs-clang-format")
-    (build-system emacs-build-system)
-    (inputs
-     `(("clang" ,clang)))
-    (arguments
-     `(#:phases
-       (modify-phases %standard-phases
-         (add-after 'unpack 'configure
-           (lambda* (#:key inputs #:allow-other-keys)
-             (let ((clang (assoc-ref inputs "clang")))
-               (copy-file "tools/clang-format/clang-format.el" "clang-format.el")
-               (emacs-substitute-variables "clang-format.el"
-                 ("clang-format-executable"
-                  (string-append clang "/bin/clang-format"))))
-             #t)))))
+    (inherit (package-elisp-from-package
+              clang
+              "emacs-clang-format"
+              '("tools/clang-format/clang-format.el")))
+    (inputs `(("clang" ,clang)))
+    (arguments `(#:phases
+                 (modify-phases %standard-phases
+                                (add-after 'unpack 'configure
+                                  (lambda* (#:key inputs #:allow-other-keys)
+                                    (chmod "clang-format.el" #o644)
+                                    (emacs-substitute-variables "clang-format.el"
+                                      ("clang-format-executable"
+                                       (string-append (assoc-ref inputs "clang")
+                                                      "/bin/clang-format")))
+                                    #t)))))
     (synopsis "Format code using clang-format")
     (description "This package allows to filter code through @code{clang-format}
 to fix its formatting.  @code{clang-format} is a tool that formats
@@ -505,22 +505,21 @@ C/C++/Obj-C code according to a set of style options, see
 
 (define-public emacs-clang-rename
   (package
-    (inherit clang)
-    (name "emacs-clang-rename")
-    (build-system emacs-build-system)
-    (inputs
-     `(("clang" ,clang)))
-    (arguments
-     `(#:phases
-       (modify-phases %standard-phases
-         (add-after 'unpack 'configure
-           (lambda* (#:key inputs #:allow-other-keys)
-             (let ((clang (assoc-ref inputs "clang")))
-               (copy-file "tools/clang-rename/clang-rename.el" "clang-rename.el")
-               (emacs-substitute-variables "clang-rename.el"
-                 ("clang-rename-binary"
-                  (string-append clang "/bin/clang-rename"))))
-             #t)))))
+    (inherit (package-elisp-from-package
+              clang
+              "emacs-clang-rename"
+              '("tools/clang-rename/clang-rename.el")))
+    (inputs `(("clang" ,clang)))
+    (arguments `(#:phases
+                 (modify-phases %standard-phases
+                   (add-after 'unpack 'configure
+                     (lambda* (#:key inputs #:allow-other-keys)
+                       (chmod "clang-rename.el" #o644)
+                       (emacs-substitute-variables "clang-rename.el"
+                         ("clang-rename-binary"
+                          (string-append (assoc-ref inputs "clang")
+                                         "/bin/clang-rename")))
+                       #t)))))
     (synopsis "Rename every occurrence of a symbol using clang-rename")
     (description "This package renames every occurrence of a symbol at point
 using @code{clang-rename}.")))
-- 
2.20.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]