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

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.

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!

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.

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?

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.

Hello!

Pierre Neidhardt <mail@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’.

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?

Hi,

Pierre Neidhardt <mail@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’.

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.

Pierre Neidhardt <mail@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’.

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

Tim, what do you think?

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.