diff mbox series

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

Message ID ddb74c8a-b135-1edb-a09e-dca07823c0a8@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 Dec. 3, 2018, 1:47 p.m. UTC 
Hi,
the attached patches add optimizations to emacs-clang-format and
emacs-clang-rename:

- Only package the required elisp file in the source
- Add a function for generating package definitions of packages
  containing a single elisp file from the clang source.

The patches make clear that the elisp file is the only thing required
for building the package and should save some disk space as the packages
do not need the full clang source tree.

Tim.

Comments

Ludovic Courtès Dec. 13, 2018, 10:49 p.m. UTC | #1 
Hello Tim,

Sorry for the delay, it looks like these patches fell through the
cracks!

Tim Gesthuizen <tim.gesthuizen@yahoo.de> skribis:

> the attached patches add optimizations to emacs-clang-format and
> emacs-clang-rename:
>
> - Only package the required elisp file in the source
> - Add a function for generating package definitions of packages
>   containing a single elisp file from the clang source.
>
> The patches make clear that the elisp file is the only thing required
> for building the package and should save some disk space as the packages
> do not need the full clang source tree.

Nice!  Pierre, could you take a look and apply them if they look good to you?

Thanks!

Ludo’.
Pierre Neidhardt Dec. 14, 2018, 9:23 a.m. UTC | #2 
Hi Tim!

Thanks for working on this.

I'm just a little confused about the intention, so I have a few questions.
The current package re-uses Clang source and packages clang-format.el /
clang-rename.el only.

In your implementation, you've essentially moved the 'configure phase to a
snippet.  As I understand it, this does not change anything.  What is your
reason for doing this?

You are also deleting all non-required files.  Which saves some space in /tmp
indeed, but this won't change anything to the store or the resulting
substitutes, right?  I don't mind this change, I just want to be clear about
what we are trying to achieve here :)

Last, the factorization into package-from-clang-elisp-file: this is a good idea,
and I would make it even more generic.  Simply rename your function

	package-elisp-from-package

(or something like that) and make "clang" a parameter.  Then we can use this
function in other packages like emacs-agda2-mode.

Am I making sense?  What do you think?
Tim Gesthuizen Dec. 14, 2018, 10:06 a.m. UTC | #3 
Hi,

On 14.12.2018 10:23, Pierre Neidhardt wrote:
> In your implementation, you've essentially moved the 'configure phase to a
> snippet.  As I understand it, this does not change anything.  What is your
> reason for doing this?

1. Intention: Stripping the source with the snippet models more clearly
   that the building part is a function that maps the single elisp file
   to the final package.

> You are also deleting all non-required files.  Which saves some space in /tmp
> indeed, but this won't change anything to the store or the resulting
> substitutes, right?  I don't mind this change, I just want to be clear about
> what we are trying to achieve here :)

2. Efficiency: I am not quite sure about this - maybe I am just wrong:
   As far as I understand it at first guix builds a source derivation
   and then the actual package derivation. Even when the source
   derivation is not stored in the store it can be substituted. If this
   is the case we can save some bandwith / disk space / build time by
   moving the file stripping to the source snippet. On my machine the
   most time during the build process is spend unpacking the clang
   source.

> Last, the factorization into package-from-clang-elisp-file: this is a good idea,
> and I would make it even more generic.  Simply rename your function
> 
> 	package-elisp-from-package
> 
> (or something like that) and make "clang" a parameter.  Then we can use this
> function in other packages like emacs-agda2-mode.

I also thought about this but could not find another situation where
this was applicable.
In which module should such a function go?
What would definitely be nice is that such a function can encapsulate
the emacs stuff so that we do not need any other emacs related modules
imported in (gnu packages llvm) for example.

> Am I making sense?  What do you think?

Yes. Maybe we should add some reasoning to the commit message then?
Depends on whether we just want a description of the changes in a commit
message or also some reasoning if things might be unclear.
I think that package-elisp-from-package is a really good idea.
Without seeing the function definition it is really hard to figure out
why it needs the arguments it needs and what their purposes are.
Maybe we want to make the arguments keyword arguments if it becomes more
generic so its usage is more intuitive.
The first two patches are debatable though. If there is a particular
reason against them (unnecessary changing without benefits included) we
might want to not merge them.

Tim.
Pierre Neidhardt Dec. 14, 2018, 10:31 a.m. UTC | #4 
> 2. Efficiency: I am not quite sure about this - maybe I am just wrong:
>    As far as I understand it at first guix builds a source derivation
>    and then the actual package derivation. Even when the source
>    derivation is not stored in the store it can be substituted. If this
>    is the case we can save some bandwith / disk space / build time by
>    moving the file stripping to the source snippet. On my machine the
>    most time during the build process is spend unpacking the clang
>    source.

I am not sure sure about this.  Ludovic, do we have such a thing as "source
substitutes"?

> I also thought about this but could not find another situation where
> this was applicable.

Look for "emacs-build-system" in files other than emacs.scm.  It's used in quite
a few places.

> In which module should such a function go?

Hmmm, I guess either guix/build/emacs-utils.scm, or gnu/packages/emacs.scm.

> What would definitely be nice is that such a function can encapsulate
> the emacs stuff so that we do not need any other emacs related modules
> imported in (gnu packages llvm) for example.

What emacs stuff?  You mean the build system?

> Yes. Maybe we should add some reasoning to the commit message then?
> Depends on whether we just want a description of the changes in a commit
> message or also some reasoning if things might be unclear.

Well, the reasoning above is mostly a nit.  What matters most is
- Efficiency, if it really works.
- The abstraction function.

> Without seeing the function definition it is really hard to figure out
> why it needs the arguments it needs and what their purposes are.
> Maybe we want to make the arguments keyword arguments if it becomes more
> generic so its usage is more intuitive.

Sure, that's the very purpose of keyword arguments, make code readable.  If it
reads like an English sentence, it's a win!

> The first two patches are debatable though. If there is a particular
> reason against them (unnecessary changing without benefits included) we
> might want to not merge them.

Even if the efficiency point is moot, the abstraction is more than welcome: try
it on at least one more package, then it will save lots of package code.  I'm
all for it! :)
Tim Gesthuizen Dec. 14, 2018, 11 a.m. UTC | #5 
On 14.12.2018 11:31, Pierre Neidhardt wrote:
> I am not sure sure about this.  Ludovic, do we have such a thing as "source
> substitutes"?

I have searched a little bit in my store... Looks like its not the case.
After all it seems to me that this is the purpose of a source snippet.

>> I also thought about this but could not find another situation where
>> this was applicable.
> 
> Look for "emacs-build-system" in files other than emacs.scm.  It's used in quite
> a few places.

I will have a look at it.

> What emacs stuff?  You mean the build system?

Yes. With the abstraction we could only import the function and do not
need the emacs-build-system imported in modules that have nothing to do
with emacs otherwise.

>> Yes. Maybe we should add some reasoning to the commit message then?
>> Depends on whether we just want a description of the changes in a commit
>> message or also some reasoning if things might be unclear.
> 
> Well, the reasoning above is mostly a nit.  What matters most is
> - Efficiency, if it really works.
> - The abstraction function.

Then I will apply the changes to the function and send new patches when
I am done. Unless Ludo thinks differently we probably shouldn't merge

- The first two patches if they don't work like I expected them to
- The last patch in its current form until the changes are implemented
  and we can start to use the generic function in package definitions

Tim.
Pierre Neidhardt Dec. 14, 2018, 12:09 p.m. UTC | #6 
> I have searched a little bit in my store... Looks like its not the case.
> After all it seems to me that this is the purpose of a source snippet.

My understanding is that snippets are mostly meant as a Lispy way to replace
patches.

But hey, it's all Scheme code in the end, should it be executed before or after
does not make much of a difference.

> Yes. With the abstraction we could only import the function and do not
> need the emacs-build-system imported in modules that have nothing to do
> with emacs otherwise.

But you'll have to import the module where the abstraction is defined ;)
Tim Gesthuizen Dec. 14, 2018, 12:12 p.m. UTC | #7 
On 14.12.2018 13:09, Pierre Neidhardt wrote:
> But you'll have to import the module where the abstraction is defined ;)

But you could only #:select the abstraction and don't import the rest ;)

Tim.
Tim Gesthuizen Dec. 19, 2018, 5:47 p.m. UTC | #8 
Hi,
I was wondering why I sended patches that do not improve
anything. After some more investigation I found out again what the
patches improve:

The source is repacked by guix and the patches applied before the source
is stored in the store.  With the current guix:

┌────
│ ~/src/guix/build$ tar -tf `guix build -S emacs-clang-format`
│ cfe-6.0.1.src/
│ cfe-6.0.1.src/.arcconfig
│ cfe-6.0.1.src/.clang-format
│ cfe-6.0.1.src/.clang-tidy
│ cfe-6.0.1.src/.gitignore
│ cfe-6.0.1.src/CMakeLists.txt
│ cfe-6.0.1.src/CODE_OWNERS.TXT
│ cfe-6.0.1.src/INPUTS/
│ cfe-6.0.1.src/INPUTS/Cocoa_h.m
│ cfe-6.0.1.src/INPUTS/all-std-headers.cpp
│ cfe-6.0.1.src/INPUTS/c99-intconst-1.c
│ cfe-6.0.1.src/INPUTS/carbon_h.c
│ cfe-6.0.1.src/INPUTS/cfg-big-switch.c
│ cfe-6.0.1.src/INPUTS/cfg-long-chain1.c
│ cfe-6.0.1.src/INPUTS/cfg-long-chain2.c
│ cfe-6.0.1.src/INPUTS/cfg-long-chain3.c
│ cfe-6.0.1.src/INPUTS/cfg-nested-switches.c
│ cfe-6.0.1.src/INPUTS/cfg-nested-var-scopes.cpp
│ cfe-6.0.1.src/INPUTS/iostream.cc
│ cfe-6.0.1.src/INPUTS/macro_pounder_fn.c
│ cfe-6.0.1.src/INPUTS/macro_pounder_obj.c
│ cfe-6.0.1.src/INPUTS/stpcpy-test.c
│ cfe-6.0.1.src/INSTALL.txt
│ cfe-6.0.1.src/LICENSE.TXT
│ cfe-6.0.1.src/ModuleInfo.txt
│ cfe-6.0.1.src/NOTES.txt
│ ... < the rest of clangs sources >
└────

With the patches applied:

┌────
│ ~/src/guix/build$ tar -tf `guix build -S emacs-clang-format`
│ ... < repacking and building the source>
│ successfully built
/gnu/store/yds0n90idgaa65ladnpbcdjkqgs8l61d-emacs-clang-format-6.0.1.tar.xz.drv
│ cfe-6.0.1.src/
│ cfe-6.0.1.src/clang-format.el
└────

So it does save disk space. I also think that these sources can be
substituted (This may still not be the case though).  The term source
derivation is mentioned in the description of guix builds `-S` option.

I would argue that I implement the file deletion in the source snippet
in package-elisp-from-package. I hope this makes clear what the purpose
of the first two patches is.

Tim.
Pierre Neidhardt Dec. 19, 2018, 5:50 p.m. UTC | #9 
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!
diff mbox series

Patch

From e4fdd028c0e71118073aee34c5976a4948b46511 Mon Sep 17 00:00:00 2001
From: Tim Gesthuizen <tim.gesthuizen@yahoo.de>
Date: Fri, 30 Nov 2018 15:13:51 +0100
Subject: [PATCH 3/3] gnu: Add package-from-clang-elisp-file

emacs-clang-format and emacs-clang-rename both are packages that are build by
extracting a single elisp file and building it as an emacs package.
This concept is encapsulated in the package-from-clang-elisp-file
function. This reduces repeating of concepts in the two package definitions.

* gnu/packages/llvm.scm (package-from-clang-elisp-file): New function
* gnu/packages/llvm.scm (emacs-clang-format): Use new function
* gnu/packages/llvm.scm (emacs-clang-rename): Use new function
---
 gnu/packages/llvm.scm | 138 +++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 77 deletions(-)

diff --git a/gnu/packages/llvm.scm b/gnu/packages/llvm.scm
index 3eb0fb29f..ad71c967a 100644
--- a/gnu/packages/llvm.scm
+++ b/gnu/packages/llvm.scm
@@ -460,85 +460,69 @@  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
-  (let ((target-file "clang-format.el"))
-    (package
-      (inherit clang)
-      (name "emacs-clang-format")
-      (source (let ((orig (package-source clang)))
-                (origin
-                  (method (origin-method orig))
-                  (uri (origin-uri orig))
-                  (sha256 (origin-sha256 orig))
-                  (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 "tools/clang-format/" ,target-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
-                   ("clang-format-executable"
-                    (string-append clang "/bin/clang-format"))))
-               #t)))))
-      (synopsis "Format code using clang-format")
-      (description "This package allows to filter code through @code{clang-format}
+  (package-from-clang-elisp-file
+   "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}
 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}."))))
+@url{http://clang.llvm.org/docs/ClangFormatStyleOptions.html}."))
 
 (define-public emacs-clang-rename
-  (let ((target-file "clang-rename.el"))
-   (package
-     (inherit clang)
-     (name "emacs-clang-rename")
-     (source (let ((orig (package-source clang)))
-               (origin
-                 (method (origin-method orig))
-                 (uri (origin-uri orig))
-                 (sha256 (origin-sha256 orig))
-                 (file-name (string-append name
-                                           (package-version clang)))
-                 (modules '((guix build utils)
-                            (srfi srfi-1)
-                            (ice-9 ftw)))
-                 (snippet
-                  `(begin
-                     (copy-file (string-append "tools/clang-rename/" ,target-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
-                  ("clang-rename-binary"
-                   (string-append 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}."))))
+  (package-from-clang-elisp-file
+   "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
+using @code{clang-rename}."))
-- 
2.19.2