GNU bug report logs - #33598
Optimizations for emacs-clang-format and emacs-clang-rename

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33598 in the body.
You can then email your comments to 33598 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: guix-patches <at> gnu.org
Subject: Optimizations for emacs-clang-format and emacs-clang-rename
Date: Mon, 3 Dec 2018 14:47:48 +0100

[Message part 1 (text/plain, inline)]

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.

[0001-gnu-Shrink-source-for-emacs-clang-format.patch (text/x-patch, attachment)]

[0002-gnu-Shrink-source-for-emacs-clang-rename.patch (text/x-patch, attachment)]

[0003-gnu-Add-package-from-clang-elisp-file.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, attachment)]

Message #8 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>,
 Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Thu, 13 Dec 2018 23:49:27 +0100

Hello Tim,

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

Tim Gesthuizen <tim.gesthuizen <at> 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’.

Message #11 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 10:23:41 +0100

[Message part 1 (text/plain, inline)]

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?

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>, Ludovic Courtès
 <ludo <at> gnu.org>
Cc: 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 11:06:21 +0100

[Message part 1 (text/plain, inline)]

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.

[signature.asc (application/pgp-signature, attachment)]

Message #17 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 11:31:44 +0100

[Message part 1 (text/plain, inline)]

> 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! :)

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #20 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 12:00:34 +0100

[Message part 1 (text/plain, inline)]

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.

[signature.asc (application/pgp-signature, attachment)]

Message #23 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 13:09:44 +0100

[Message part 1 (text/plain, inline)]

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

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 14 Dec 2018 13:12:49 +0100

[Message part 1 (text/plain, inline)]

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.

[signature.asc (application/pgp-signature, attachment)]

Message #29 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Wed, 19 Dec 2018 18:47:00 +0100

[Message part 1 (text/plain, inline)]

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.

[signature.asc (application/pgp-signature, attachment)]

Message #32 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Wed, 19 Dec 2018 18:50:28 +0100

[Message part 1 (text/plain, inline)]

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!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Fri, 4 Jan 2019 23:00:52 +0100

[Message part 1 (text/plain, inline)]

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.

[0001-gnu-Shrink-source-for-emacs-clang-format.patch (text/x-patch, attachment)]

[0002-gnu-Shrink-source-for-emacs-clang-rename.patch (text/x-patch, attachment)]

[0003-gnu-Add-package-from-clang-elisp-file.patch (text/x-patch, attachment)]

[0004-gnu-Add-package-elisp-from-package.patch (text/x-patch, attachment)]

[0005-gnu-Use-package-elisp-from-package-for-clangs-emacs-.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, attachment)]

Message #38 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Sun, 06 Jan 2019 20:00:19 +0100

[Message part 1 (text/plain, inline)]

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!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #41 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Sun, 6 Jan 2019 22:29:26 +0100

[Message part 1 (text/plain, inline)]

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.

[0001-gnu-Add-package-elisp-from-package.patch (text/x-patch, attachment)]

[0002-gnu-Use-package-elisp-from-package-for-clangs-emacs-.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, attachment)]

Message #44 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 07 Jan 2019 14:47:55 +0100

[Message part 1 (text/plain, inline)]

Merged!

> : > +;;; Returns a package definition that packages an emacs-lisp file from the
> : 
> : "Return", not "Returns".

You forgot this!  Anyways, I was confused by the docstring, so I took the
liberty to simplify it a little bit to the following:

  "Return a package definition named PACKAGE-NAME that packages the Emacs Lisp
SOURCE-FILES found in SOURCE-PACKAGE."

If you think this is missing something, let me know and I'll fix it.

Thanks for your great contribution!

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #47 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 07 Jan 2019 15:00:34 +0100

[Message part 1 (text/plain, inline)]

Oops, got too fast: there is a circular dependency problem because emacs.scm
depends on llvm.scm.
The function must be moved to some other place.  I'll place it in emacs utils or
something.

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #50 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 07 Jan 2019 15:08:12 +0100

[Message part 1 (text/plain, inline)]

Hmmm... Not too sure where to put package-elisp-from-package.
I see the following options:

- build-system/emacs.scm, but is it our policy?
- Or a separate file, but which one?
  
@Ludo?

-- 
Pierre Neidhardt
https://ambrevar.xyz/

[signature.asc (application/pgp-signature, inline)]

Message #53 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 7 Jan 2019 16:37:17 +0100

[Message part 1 (text/plain, inline)]

On 07.01.19 14:47, Pierre Neidhardt wrote:
> Merged!
> 
>> : > +;;; Returns a package definition that packages an emacs-lisp file from the
>> : 
>> : "Return", not "Returns".
> 
> You forgot this!  Anyways, I was confused by the docstring, so I took the
> liberty to simplify it a little bit to the following:

Whoops... Maybe I did not reformat the patches.

>   "Return a package definition named PACKAGE-NAME that packages the Emacs Lisp
> SOURCE-FILES found in SOURCE-PACKAGE."
> 
> If you think this is missing something, let me know and I'll fix it.

Much better. That was probably just to straight forward for me.

> Oops, got too fast: there is a circular dependency problem because emacs.scm
> depends on llvm.scm.
> The function must be moved to some other place.  I'll place it in emacs utils or
> something.

So that is my "weird issue"!
I first packaged the function in emacs-utils.scm and experienced much
worse things. I suspected it to be a circular dependency as any package
using emacs-build-system includes emacs-utils under the hood and moved
it to packages/emacs.scm.
I thought it was something different because the same error appeared
there too.

Maybe we should have a file with "shortcuts" for package definitions of
special kind and place the function there?
I would claim that we would have the same problems in emacs-utils.scm.

Tim.

[signature.asc (application/pgp-signature, attachment)]

Message #56 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 07 Jan 2019 23:10:23 +0100

Hello!

Pierre Neidhardt <mail <at> ambrevar.xyz> skribis:

> Hmmm... Not too sure where to put package-elisp-from-package.
> I see the following options:
>
> - build-system/emacs.scm, but is it our policy?
> - Or a separate file, but which one?
>   
> @Ludo?

I actually fixed it today right after you reverted the commit (I rebased
and didn’t notice it had been reverted in the meantime):

  https://git.savannah.gnu.org/cgit/guix.git/commit/?id=67eebb19b70acfe997b40d8c7978f9dc0673a4af

With this you should be able to reinstate the rest of the commit.

I tried to explain the reason for the issue in the commit log, let me
know if anything’s unclear.

Thanks,
Ludo’.

Message #59 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Mon, 07 Jan 2019 23:14:42 +0100

It works, but it's semantically dubious.  For packages like emacs-cmake-mode now
have to use the llvm module to use package-elisp-from-package.
Thoughts?

Message #62 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Tue, 08 Jan 2019 09:39:09 +0100

Hi,

Pierre Neidhardt <mail <at> ambrevar.xyz> skribis:

> It works, but it's semantically dubious.  For packages like emacs-cmake-mode now
> have to use the llvm module to use package-elisp-from-package.
> Thoughts?

Sure, it probably belongs elsewhere.  I moved it there to quickly fix
the problem, and I purposefully made it private, but I agree, it could
go to some other places if there’s a need for it outside of llvm.scm.
I’m not sure exactly where.

I wonder how often the approach of ‘package-elisp-from-package’ is
applicable or desirable.  There are packages (e.g., recutils, GLOBAL)
that come with elisp files, which automatically get installed upon “make
install.”  I’m not sure we’d want to make them separate.

Thoughts?

Ludo’.

Message #65 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Tue, 08 Jan 2019 09:48:04 +0100

The benefit of separate Emacs packages is if the Emacs library can be used
without installing the parent package to the user profile.

For instance, emacs-clang-rename can be installed and it will work while the
user does not have to install clang.  (Clang remains an input, obviously.)

For this reason, "package-elisp-from-package" gives maximal flexibility in my
opinion.

Currently, there are a few more packages.  We can look up "emacs-build-system"
outside emacs.scm to find them (e.g. agda2).

Off the top of my head, there is also asymptote.

Now to the ideal place for package-elisp-from-package: it seems that no existing
file would be a fit.  So what about guix/utils/emacs.scm?  Having a separate
file would make sure we run into other meta-circular dependency issues.

Message #68 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Tue, 08 Jan 2019 10:53:50 +0100

Pierre Neidhardt <mail <at> ambrevar.xyz> skribis:

> The benefit of separate Emacs packages is if the Emacs library can be used
> without installing the parent package to the user profile.
>
> For instance, emacs-clang-rename can be installed and it will work while the
> user does not have to install clang.  (Clang remains an input, obviously.)
>
> For this reason, "package-elisp-from-package" gives maximal flexibility in my
> opinion.

Yes, I agree that it makes a lot of sense for ‘emacs-clang-rename’ for
instance.  I’m just unsure whether the approach generalize to other
packages.

> Currently, there are a few more packages.  We can look up "emacs-build-system"
> outside emacs.scm to find them (e.g. agda2).
>
> Off the top of my head, there is also asymptote.

I’m not convinced sure ‘emacs-agda2-mode’ and ‘asymptote’ need to be
changed; it doesn’t look like a clear win, dunno.

For example, ‘package-elisp-from-package’ preserves the name, synopsis,
and description, so you end up having to do:

  (define foo
    (package
      (inherit (package-elisp-from-package x))
      (name "emacs-foo")
      (license …)
      (synopsis …)
      (description …)))

… which I think it a marginal improvement compared to
‘emacs-agda2-mode’.  Also, the “find *.el” approach may not work out of
the box for all cases, so the procedure may need to be tweaked further,
etc.

> Now to the ideal place for package-elisp-from-package: it seems that no existing
> file would be a fit.  So what about guix/utils/emacs.scm?  Having a separate
> file would make sure we run into other meta-circular dependency issues.

Circular, not meta-circular.  ;-)

So yeah, (guix utils emacs) is one option; another one would be to trim
the list of modules emacs.scm depends on, such that we don’t have this
issue in the first place (that requires care, though.)

However, my suggestion would be to use ‘package-elisp-from-package’ as
Tim intended in the original patch, keeping the procedure private to
llvm.scm, and generalize if and when we see other use cases.

How does that sound?

Ludo’.

Message #71 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Tue, 08 Jan 2019 11:05:29 +0100

Agreed, the win is not always obvious.
If we are all OK with this, let's close the issue.

Tim, what do you think?

Message #74 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>, Ludovic Courtès
 <ludo <at> gnu.org>
Cc: 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Tue, 8 Jan 2019 16:35:11 +0100

[Message part 1 (text/plain, inline)]

On 08.01.19 11:05, Pierre Neidhardt wrote:
> Agreed, the win is not always obvious.
> If we are all OK with this, let's close the issue.
> 
> Tim, what do you think?
> 

That's no problem for me.
After all my intent was to make sure I don't have clangs source tree 3
times in the store :)
I think we should still put the generic function into llvm.scm and not
the clang specific one.

[signature.asc (application/pgp-signature, attachment)]

Message #77 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>, Ludovic Courtès
 <ludo <at> gnu.org>
Cc: 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Thu, 10 Jan 2019 19:28:16 +0100

[Message part 1 (text/plain, inline)]

Hi,

I implemented the changes we discussed so we can finally close this ticket.
Patches are attached.

Tim.

[0001-gnu-Add-package-elisp-from-package.patch (text/x-patch, attachment)]

[0002-gnu-Use-package-elisp-from-package-for-clangs-emacs-.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, attachment)]

Message #80 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Thu, 10 Jan 2019 19:40:24 +0100

Err... The patch is already merged, what did you change?

Message #83 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
To: Pierre Neidhardt <mail <at> ambrevar.xyz>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Thu, 10 Jan 2019 19:47:38 +0100

On 10.01.19 19:40, Pierre Neidhardt wrote:
> Err... The patch is already merged, what did you change?
> 

Sorry... Forgot to pull.
Is there anything left keeping us from closing the ticket?

Message #86 received at 33598 <at> debbugs.gnu.org (full text, mbox):

From: Pierre Neidhardt <mail <at> ambrevar.xyz>
To: Tim Gesthuizen <tim.gesthuizen <at> yahoo.de>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 33598 <at> debbugs.gnu.org
Subject: Re: [bug#33598] Optimizations for emacs-clang-format and
 emacs-clang-rename
Date: Thu, 10 Jan 2019 19:50:16 +0100

No, I think you can close it.  Thank you again for your contribution!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.