bug#26802: Single source file emacs packages get a ".el.el" extension

bug#26802: Single source file emacs packages get a ".el.el" extension

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]


Single source file emacs packages (such as emacs-goto-chg,
emacs-transpose-frame, emacs-key-chord, etc.) get installed with a
double extension (".el.el"). This patch fixes that.


[-- Attachment #2: 0001-build-emacs-Don-t-append-an-extra-.el-to-source-file.patch --]
[-- Type: text/x-patch, Size: 1398 bytes --]

From 4796890f507a126edb6020573547d37815b3241e Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac@systemreboot.net>
Date: Sat, 6 May 2017 18:16:36 +0530
Subject: [PATCH] build: emacs: Don't append an extra ".el" to source file
 name.

* guix/build/emacs-build-system.scm (store-file->elisp-source-file): The
  source file name already has a ".el" suffix. Don't append an extra ".el".
---
 guix/build/emacs-build-system.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 44e8b0d31..3669b7d59 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2015 Federico Beffa <beffa@fbengineering.ch>
 ;;; Copyright © 2016 David Thompson <davet@gnu.org>
 ;;; Copyright © 2016 Alex Kost <alezost@gmail.com>
+;;; Copyright © 2017 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -50,7 +51,7 @@ name that has been stripped of the hash and version number."
   (let-values (((name version)
                 (package-name->name+version
                  (strip-store-file-name file))))
-    (string-append name ".el")))
+    name))
 
 (define* (unpack #:key source #:allow-other-keys)
   "Unpack SOURCE into the build directory.  SOURCE may be a compressed
-- 
2.12.2

bug#26802: Single source file emacs packages get a ".el.el" extension

[Alex Kost]

Arun Isaac (2017-05-06 18:21 +0530) wrote:

> Single source file emacs packages (such as emacs-goto-chg,
> emacs-transpose-frame, emacs-key-chord, etc.) get installed with a
> double extension (".el.el"). This patch fixes that.

Actually, this patch will break most of the single-file packages.  Try,
for example, the following command with your patch:

  guix build emacs-adaptive-wrap

As you can see, the elisp file is called "adaptive-wrap" instead of
"adaptive-wrap.el".  I would rather suggest to fix the packages you
mentioned (emacs-key-chord, etc.) to add 'file-name' field as it is done
for the other single-file packages (emacs-web-mode, emacs-yaml-mode,
etc.)

-- 
Alex

bug#26802: Single source file emacs packages get a ".el.el" extension

[Arun Isaac]

> I would rather suggest to fix the packages you mentioned
> (emacs-key-chord, etc.) to add 'file-name' field as it is done for the
> other single-file packages (emacs-web-mode, emacs-yaml-mode, etc.)

You're right. I'll do that. But, I think we should also have a guix lint
check that makes sure the version is present in the store file
name. What do you think?

bug#26802: Single source file emacs packages get a ".el.el" extension

[Alex Kost]

Arun Isaac (2017-05-12 00:49 +0530) wrote:

>> I would rather suggest to fix the packages you mentioned
>> (emacs-key-chord, etc.) to add 'file-name' field as it is done for the
>> other single-file packages (emacs-web-mode, emacs-yaml-mode, etc.)
>
> You're right. I'll do that. But, I think we should also have a guix lint
> check that makes sure the version is present in the store file
> name. What do you think?

It would be a linter only for *.el files, is that what you mean?  AFAIK
we do not have such specific linters, but yeah, why not, I think it's a
good idea.

P.S.  Could you please keep my email in Cc?  It would be easier to
follow the discussion for me, thanks.

-- 
Alex

bug#26802: Single source file emacs packages get a ".el.el" extension

[Arun Isaac]

I've pushed the other uncontroversial patches.

> It would be a linter only for *.el files, is that what you mean?  AFAIK
> we do not have such specific linters, but yeah, why not, I think it's a
> good idea.

No, it will be a linter for all packages, not just emacs packages.

The following is the current linter for checking source file names. I
have a couple of issues with it, in addition to the new linter feature I
am suggesting. Let me explain.

> (define (check-source-file-name package)
>   "Emit a warning if PACKAGE's origin has no meaningful file name."
>   (define (origin-file-name-valid? origin)
>     ;; Return #t if the source file name contains only a version or is #f;
>     ;; indicates that the origin needs a 'file-name' field.

Isn't this logic somehow backward? Should'nt a predicate called
`file-name-valid?' return #t if the file name is valid, and #f
otherwise?  This seems to be doing the opposite of that, returning #f if
the file name is valid, and #t otherwise.

>     (let ((file-name (origin-actual-file-name origin))
>           (version (package-version package)))
>       (and file-name
>            (not (or (string-prefix? version file-name)
>                     ;; Common in many projects is for the filename to start
>                     ;; with a "v" followed by the version,
>                     ;; e.g. "v3.2.0.tar.gz".
>                     (string-prefix? (string-append "v" version) file-name))))))

I think this check can be done by matching against a single regexp like
so:

(string-match (string-append "^v?" version) file-name)

In addition to this logic, we add an extra condition that makes sure the
version is some substring of the source file name, like so:

(string-match version file-name)

With this new check, single source file packages like
emacs-transpose-frame, etc. which did not have file-name fields would
have raised a lint warning.

>   (let ((origin (package-source package)))
>     (unless (or (not origin) (origin-file-name-valid? origin))
>       (emit-warning package
>                     (G_ "the source file name should contain the package name")
>                     'source))))

> P.S.  Could you please keep my email in Cc?  It would be easier to
> follow the discussion for me, thanks.

Sorry, I thought debbugs does some magic to send out mails to everybody
involved in a specific bug. Didn't realize it was only using Cc. Will
keep you in Cc for this and future mails.

bug#26802: Single source file emacs packages get a ".el.el" extension

[Alex Kost]

Arun Isaac (2017-05-13 22:06 +0530) wrote:

> I've pushed the other uncontroversial patches.
>
>> It would be a linter only for *.el files, is that what you mean?  AFAIK
>> we do not have such specific linters, but yeah, why not, I think it's a
>> good idea.
>
> No, it will be a linter for all packages, not just emacs packages.

Hm, but the ".el" source files are special: they must have
"name-version.el" name, while the other sources may be named pretty
arbitrary.

> The following is the current linter for checking source file names. I
> have a couple of issues with it, in addition to the new linter feature I
> am suggesting. Let me explain.
>
>> (define (check-source-file-name package)
>>   "Emit a warning if PACKAGE's origin has no meaningful file name."
>>   (define (origin-file-name-valid? origin)
>>     ;; Return #t if the source file name contains only a version or is #f;
                 ^^    (it's a typo I think, should be ‘#f’ there)
>>     ;; indicates that the origin needs a 'file-name' field.
>
> Isn't this logic somehow backward? Should'nt a predicate called
> `file-name-valid?' return #t if the file name is valid, and #f
> otherwise?  This seems to be doing the opposite of that, returning #f if
> the file name is valid, and #t otherwise.

If I understand correctly, this predicate does the right thing; it's
just a typo in the commentary.

>>     (let ((file-name (origin-actual-file-name origin))
>>           (version (package-version package)))
>>       (and file-name
>>            (not (or (string-prefix? version file-name)
>>                     ;; Common in many projects is for the filename to start
>>                     ;; with a "v" followed by the version,
>>                     ;; e.g. "v3.2.0.tar.gz".
>>                     (string-prefix? (string-append "v" version) file-name))))))
>
> I think this check can be done by matching against a single regexp like
> so:
>
> (string-match (string-append "^v?" version) file-name)

I agree, a single regexp looks better!

> In addition to this logic, we add an extra condition that makes sure the
> version is some substring of the source file name, like so:
>
> (string-match version file-name)
>
> With this new check, single source file packages like
> emacs-transpose-frame, etc. which did not have file-name fields would
> have raised a lint warning.

I'm not sure, I think:

- it's too much for all the sources, as the upstream source may not
contain a version in the file name at all.  Do we really want to raise a
warning in this case?

- and it's not enough for ".el" sources, I mean "something-version.el"
is not enough, as the file name must exactly be "name-version.el" (as it
is with ELPA single-filed sources), so the emacs-build-system will
output "name.el" file which will correspond to 'name' feature provided
by this file.

[...]
>> P.S.  Could you please keep my email in Cc?  It would be easier to
>> follow the discussion for me, thanks.
>
> Sorry, I thought debbugs does some magic to send out mails to everybody
> involved in a specific bug. Didn't realize it was only using Cc. Will
> keep you in Cc for this and future mails.

Thanks!

-- 
Alex

bug#26802: Single source file emacs packages get a ".el.el" extension

[Arun Isaac]

> I'm not sure, I think:
>
> - it's too much for all the sources, as the upstream source may not
> contain a version in the file name at all.  Do we really want to raise a
> warning in this case?

> - and it's not enough for ".el" sources, I mean "something-version.el"
> is not enough, as the file name must exactly be "name-version.el" (as it
> is with ELPA single-filed sources), so the emacs-build-system will
> output "name.el" file which will correspond to 'name' feature provided
> by this file.

You have a point, but...

If all packages cannot be expected to have "name-version", then it is
unreasonable and arbitrary to only expect single source file emacs
packages to have a filename of this format. Instead, the emacs build
system should be made more robust so that it can tolerate a souce file
name like "web-mode.el" and still produce the correct installation path.

bug#26802: Single source file emacs packages get a ".el.el" extension

[Alex Kost]

Arun Isaac (2017-05-15 18:58 +0530) wrote:

>> I'm not sure, I think:
>>
>> - it's too much for all the sources, as the upstream source may not
>> contain a version in the file name at all.  Do we really want to raise a
>> warning in this case?
>
>> - and it's not enough for ".el" sources, I mean "something-version.el"
>> is not enough, as the file name must exactly be "name-version.el" (as it
>> is with ELPA single-filed sources), so the emacs-build-system will
>> output "name.el" file which will correspond to 'name' feature provided
>> by this file.
>
> You have a point, but...
>
> If all packages cannot be expected to have "name-version", then it is
> unreasonable and arbitrary to only expect single source file emacs
> packages to have a filename of this format. Instead, the emacs build
> system should be made more robust so that it can tolerate a souce file
> name like "web-mode.el" and still produce the correct installation path.

Yeah, it would definitely be good to make emacs-build-system more robust.

After thinking more, I came to the conclusion that expanding the linter
to check any source for "name-version" is a good idea (if this is what
you suggest, then I agree with you!)  So if a source name has some other
form, it would be linted, and can be fixed with 'file-name' field.  I
think such consistency in source file names would be really great.

Apparently, this was your original propose (right?), now I support this
idea! :-)

-- 
Alex

bug#26802: Single source file emacs packages get a ".el.el" extension

[Arun Isaac]

> After thinking more, I came to the conclusion that expanding the linter
> to check any source for "name-version" is a good idea (if this is what
> you suggest, then I agree with you!)  So if a source name has some other
> form, it would be linted, and can be fixed with 'file-name' field.  I
> think such consistency in source file names would be really great.
>
> Apparently, this was your original propose (right?), now I support this
> idea! :-)

Well, I'm totally confused now! :-P I don't know which proposal each of
us is for/against. So, to make things clearer, I have sent a few patches
implementing the various proposals. Patches 1 and 2 are pretty
uncontroversial and have little to do with the double extension
bug. These patches fix the typo in the docstring, and simplify
`check-source-file-name' using a regexp, as discussed earlier.

Patches 3 and 4 are two different ways to solve the double extension
".el.el" problem, only one of which we should push.

Patch 3 makes the linter check for the existence of the version number
somewhere in the source file name. Therefore, if there is no version in
the file name, the packager will put in a file-name field, thus avoiding
the double extension problem. However, modifying the linter like this
will have far-reaching consequences possibly affecting other packages
which build fine without lint warnings. I am currently NOT IN FAVOR of
this approach.

Patch 4 fixes the problem by just making the emacs-build-system (in
particular, the `store-file->elisp-source-file' function) more robust,
and capable of handling file names without a version number. This, I
think, is the better solution. I am currently IN FAVOR of this approach.

Hopefully, this settles the confusion and ambiguity. :-) WDYT -- Patch
3 or 4?

bug#26802: Single source file emacs packages get a ".el.el" extension

[Alex Kost]

Arun Isaac (2017-05-17 22:34 +0530) wrote:

[...]
> Patches 3 and 4 are two different ways to solve the double extension
> ".el.el" problem, only one of which we should push.

Actually, I am for both (but for a modified version of the patch 3) :-)

> Patch 3 makes the linter check for the existence of the version number
> somewhere in the source file name. Therefore, if there is no version in
> the file name, the packager will put in a file-name field, thus avoiding
> the double extension problem. However, modifying the linter like this
> will have far-reaching consequences possibly affecting other packages
> which build fine without lint warnings.

Lint warnings are just warnings after all.  Having more warnings will
not be a big problem I think.

> I am currently NOT IN FAVOR of this approach.

And I like this approach :-)

As I've just written in another message, I'd like to have a linter that
will check for "name" and "version" to make the store file names
unambiguous.  But this is a more general discussion for another topic.

> Patch 4 fixes the problem by just making the emacs-build-system (in
> particular, the `store-file->elisp-source-file' function) more robust,
> and capable of handling file names without a version number. This, I
> think, is the better solution. I am currently IN FAVOR of this approach.

Right, I agree: it's a good fix for the problem, thanks!

> Hopefully, this settles the confusion and ambiguity. :-) WDYT -- Patch
> 3 or 4?

I think patch 4 can be committed now, and patch 3 is for another
discussion.

-- 
Alex

bug#26802: [PATCH 1/3] gnu: emacs-goto-chg: Set source file-name.

[Arun Isaac]

* gnu/packages/emacs.scm (emacs-goto-chg)[source]: Add file-name field.
---
 gnu/packages/emacs.scm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index 21120b331..8608c3907 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -3488,6 +3488,7 @@ extensions.")
        (method url-fetch)
        ;; There is no versioned source.
        (uri "https://www.emacswiki.org/emacs/download/goto-chg.el")
+       (file-name (string-append "goto-chg-" version ".el"))
        (sha256
         (base32
          "078d6p4br5vips7b9x4v6cy0wxf6m5ij9gpqd4g33bryn22gnpij"))))
-- 
2.12.2

bug#26802: [PATCH 1/3] gnu: emacs-goto-chg: Set source file-name.

[Alex Kost]

Arun Isaac (2017-05-13 10:53 +0530) wrote:

> * gnu/packages/emacs.scm (emacs-goto-chg)[source]: Add file-name field.
> ---
>  gnu/packages/emacs.scm | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
> index 21120b331..8608c3907 100644
> --- a/gnu/packages/emacs.scm
> +++ b/gnu/packages/emacs.scm
> @@ -3488,6 +3488,7 @@ extensions.")
>         (method url-fetch)
>         ;; There is no versioned source.
>         (uri "https://www.emacswiki.org/emacs/download/goto-chg.el")
> +       (file-name (string-append "goto-chg-" version ".el"))
>         (sha256
>          (base32
>           "078d6p4br5vips7b9x4v6cy0wxf6m5ij9gpqd4g33bryn22gnpij"))))

This and the other patches are uncontroversial, I think you can push
them, thanks!

-- 
Alex

bug#26802: [PATCH 1/4] gnu: lint: Fix typo.

[Arun Isaac]

* guix/scripts/lint.scm (check-source-file-name): Fix wrong return value in
  docstring.
---
 guix/scripts/lint.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index f2720f669..2b0071475 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -667,7 +667,7 @@ descriptions maintained upstream."
 (define (check-source-file-name package)
   "Emit a warning if PACKAGE's origin has no meaningful file name."
   (define (origin-file-name-valid? origin)
-    ;; Return #t if the source file name contains only a version or is #f;
+    ;; Return #f if the source file name contains only a version or is #f;
     ;; indicates that the origin needs a 'file-name' field.
     (let ((file-name (origin-actual-file-name origin))
           (version (package-version package)))
-- 
2.12.2

bug#26802: [PATCH 1/4] gnu: lint: Fix typo.

[Ludovic Courtès]

Arun Isaac <arunisaac@systemreboot.net> skribis:

> * guix/scripts/lint.scm (check-source-file-name): Fix wrong return value in
>   docstring.

LGTM!