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

Message ID 14de0933-fddc-94d0-d75a-cdf4b49fce1a@yahoo.de
State Accepted
Headers show
Series [bug#33598] Optimizations for emacs-clang-format and emacs-clang-rename | expand

Checks

Context Check Description
cbaines/applying patch fail Apply failed

Commit Message

Tim Gesthuizen Jan. 4, 2019, 10 p.m. UTC
On 19.12.2018 18:50, Pierre Neidhardt wrote:
> Hi Tim,
> 
> Yeah, that makes sense.  Please send a new patch updated with the aforementioned
> recommendations and I'll merge.
> Thanks for the good work!
> 

Hi,
I had some problems packaging the changes because I had some really
weird issues with the guile modules (please validate that the patches
compile as expected).
They implement the discussed function for generating package definitions
for packages that contain only a single elisp file from another packages
source.
If the attached patches compile, they can be merged and work as
expected. If they cause weird errors to happen Pierre tries to find some
time to investigate into this.

Tim Gesthuizen.

Comments

Pierre Neidhardt Jan. 6, 2019, 7 p.m. UTC | #1
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.

> +;;; 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---

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

> +          srcpkg pkgname src-file

Prefer complete words over abbreviations.  Here I'd suggest

  source-package
  name
  source-file

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

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

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.

Thank you again for the good work!

Patch

From fda77fbd33933065a3d18f4db60fbf53f5908970 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 5/5] 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 | 70 ++++++++++---------------------------------
 1 file changed, 15 insertions(+), 55 deletions(-)

diff --git a/gnu/packages/llvm.scm b/gnu/packages/llvm.scm
index 05147b665..c846e2758 100644
--- a/gnu/packages/llvm.scm
+++ b/gnu/packages/llvm.scm
@@ -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)
@@ -479,69 +480,28 @@  code analysis tools.")
      "This package provides a Python binding to LLVM for use in Numba.")
     (license license:bsd-3)))
 
-;;; Returns a package definition that packages an emacs-lisp file from the
-;;; clang source. The package has the name PKGNAME and packages the file
-;;; SRC-FILE from the clang source in its root directory with the name
-;;; TARGET-FILE where SUBST substitutions will be performed on the elisp file
-;;; and SYN and DESC as the package synopsis an description.
-(define (package-from-clang-elisp-file pkgname src-file target-file subst syn desc)
-  (package
-    (inherit clang)
-    (name pkgname)
-    (source (let ((orig (package-source clang)))
-              (origin
-                (method (origin-method orig))
-                (uri (origin-uri orig))
-                (sha256 (origin-sha256 orig))
-                (file-name (string-append pkgname "-" (package-version clang)))
-                (modules '((guix build utils)
-                           (srfi srfi-1)
-                           (ice-9 ftw)))
-                (snippet
-                 `(begin
-                    ;; Copy target file to source root and delete all other files
-                    (copy-file (string-append ,src-file)
-                               ,target-file)
-                    (map delete-file-recursively
-                         (fold delete
-                               (scandir ".")
-                               '("." ".." ,target-file)))
-                    #t)))))
-    (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")))
-               (emacs-substitute-variables ,target-file
-                 ,subst))
-             #t)))))
-    (synopsis syn)
-    (description desc)))
-
 (define-public emacs-clang-format
-  (package-from-clang-elisp-file
+  (package-elisp-from-package
+   clang
    "emacs-clang-format"
    "tools/clang-format/clang-format.el"
-   "clang-format.el"
-   '("clang-format-executable"
-                    (string-append clang "/bin/clang-format"))
-   "Format code using clang-format"
-   "This package allows to filter code through @code{clang-format}
+   #:inputs `(("clang" ,clang))
+   #:substitutions '("clang-format-executable"
+                     (string-append (assoc-ref inputs "clang") "/bin/clang-format"))
+   #: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
 C/C++/Obj-C code according to a set of style options, see
 @url{http://clang.llvm.org/docs/ClangFormatStyleOptions.html}."))
 
 (define-public emacs-clang-rename
-  (package-from-clang-elisp-file
+  (package-elisp-from-package
+   clang
    "emacs-clang-rename"
    "tools/clang-rename/clang-rename.el"
-   "clang-rename.el"
-   '("clang-rename-binary"
-                   (string-append clang "/bin/clang-rename"))
-   "Rename every occurrence of a symbol using clang-rename"
-   "This package renames every occurrence of a symbol at point
+   #:inputs `(("clang" ,clang))
+   #:substitutions '("clang-rename-binary"
+                     (string-append (assoc-ref inputs "clang") "/bin/clang-rename"))
+   #: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