GNU bug report logs - #26802
Single source file emacs packages get a ".el.el" extension

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 26802 in the body.
You can then email your comments to 26802 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: Arun Isaac <arunisaac <at> systemreboot.net>
To: guix-patches <at> gnu.org
Subject: Single source file emacs packages get a ".el.el" extension
Date: Sat, 06 May 2017 18:21:56 +0530

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

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.

[0001-build-emacs-Don-t-append-an-extra-.el-to-source-file.patch (text/x-patch, inline)]

From 4796890f507a126edb6020573547d37815b3241e Mon Sep 17 00:00:00 2001
From: Arun Isaac <arunisaac <at> 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 <at> fbengineering.ch>
 ;;; Copyright © 2016 David Thompson <davet <at> gnu.org>
 ;;; Copyright © 2016 Alex Kost <alezost <at> gmail.com>
+;;; Copyright © 2017 Arun Isaac <arunisaac <at> 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

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Tue, 09 May 2017 22:38:36 +0300

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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Fri, 12 May 2017 00:49:11 +0530

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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 3/3] gnu: emacs-key-chord: Set source file-name.
Date: Sat, 13 May 2017 10:53:37 +0530

* gnu/packages/emacs.scm (emacs-key-chord)[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 1d688f8cc..8f966d057 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -4584,6 +4584,7 @@ selected frame.")
      (origin
        (method url-fetch)
        (uri "https://www.emacswiki.org/emacs/download/key-chord.el")
+       (file-name (string-append "key-chord-" version ".el"))
        (sha256
         (base32
          "03m44pqggfrd53nh9dvpdjgm0rvca34qxmd30hr33hzprzjambxg"))))
-- 
2.12.2

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 2/3] gnu: emacs-transpose-frame: Set source file-name.
Date: Sat, 13 May 2017 10:53:36 +0530

* gnu/packages/emacs.scm (emacs-transpose-frame)[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 8608c3907..1d688f8cc 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -4564,6 +4564,7 @@ with gnuplot.")
      (origin
        (method url-fetch)
        (uri "http://www.emacswiki.org/emacs/download/transpose-frame.el")
+       (file-name (string-append "transpose-frame-" version ".el"))
        (sha256
         (base32
          "1f67yksgw9s6j0033hmqzaxx2a93jm11sd5ys7cc3li5gfh680m4"))))
-- 
2.12.2

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 1/3] gnu: emacs-goto-chg: Set source file-name.
Date: Sat, 13 May 2017 10:53:35 +0530

* 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

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Sat, 13 May 2017 11:54:27 +0300

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

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 1/3] gnu: emacs-goto-chg: Set source file-name.
Date: Sat, 13 May 2017 11:55:52 +0300

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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: Alex Kost <alezost <at> gmail.com>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Sat, 13 May 2017 22:06:05 +0530

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.

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Sun, 14 May 2017 20:15:36 +0300

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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: Alex Kost <alezost <at> gmail.com>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Mon, 15 May 2017 18:58:29 +0530

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

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Tue, 16 May 2017 20:29:17 +0300

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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 1/4] gnu: lint: Fix typo.
Date: Wed, 17 May 2017 22:22:43 +0530

* 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

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 2/4] guix: lint: Slightly simplify `check-source-file-name'.
Date: Wed, 17 May 2017 22:22:44 +0530

* guix/scripts/lint.scm (check-source-file-name): Implement file name matching
  with regular expression.
---
 guix/scripts/lint.scm | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 2b0071475..1d930d8c0 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -672,11 +672,10 @@ descriptions maintained upstream."
     (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))))))
+           ;; Common in many projects is for the filename to start
+           ;; with a "v" followed by the version,
+           ;; e.g. "v3.2.0.tar.gz".
+           (not (string-match (string-append "^v?" version) file-name)))))
 
   (let ((origin (package-source package)))
     (unless (or (not origin) (origin-file-name-valid? origin))
-- 
2.12.2

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 3/4] guix: lint: Check for version in source file name.
Date: Wed, 17 May 2017 22:22:45 +0530

* guix/scripts/lint.scm (check-source-file-name): Check for version in source
  file name.
---
 guix/scripts/lint.scm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 1d930d8c0..b6f73d0e6 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -667,20 +667,22 @@ 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 #f if the source file name contains only a version or is #f;
-    ;; indicates that the origin needs a 'file-name' field.
+    ;; Return #f if the source file name is #f, contains only a version, or
+    ;; does not contain a version; indicates that the origin needs a
+    ;; 'file-name' field.
     (let ((file-name (origin-actual-file-name origin))
           (version (package-version package)))
       (and 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".
-           (not (string-match (string-append "^v?" version) file-name)))))
+           (not (string-match (string-append "^v?" version) file-name))
+           (string-match version file-name))))
 
   (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")
+                    (G_ "the source file name should contain the package name and version")
                     'source))))
 
 (define (check-mirror-url package)
-- 
2.12.2

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802 <at> debbugs.gnu.org
Cc: Arun Isaac <arunisaac <at> systemreboot.net>
Subject: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.
Date: Wed, 17 May 2017 22:22:46 +0530

This prevents a ".el.el" extension for source files with no version number in
their file name.

* guix/build/emacs-build-system.scm (store-file->elisp-source-file): Remove
  ".el" extension from file name before splitting to name and version.
---
 guix/build/emacs-build-system.scm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 44e8b0d31..25a3beaa2 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -47,10 +47,13 @@
 (define (store-file->elisp-source-file file)
   "Convert FILE, a store file name for an Emacs Lisp source file, into a file
 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")))
+  (let ((extension ".el"))
+    (let-values (((name version)
+                  (package-name->name+version
+                   (strip-store-file-name
+                    (string-drop-right
+                     file (string-length extension))))))
+      (string-append name extension))))
 
 (define* (unpack #:key source #:allow-other-keys)
   "Unpack SOURCE into the build directory.  SOURCE may be a compressed
-- 
2.12.2

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: Alex Kost <alezost <at> gmail.com>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Wed, 17 May 2017 22:34:19 +0530

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

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 1/4] gnu: lint: Fix typo.
Date: Thu, 18 May 2017 13:28:49 +0200

Arun Isaac <arunisaac <at> systemreboot.net> skribis:

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

LGTM!

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 2/4] guix: lint: Slightly simplify
 `check-source-file-name'.
Date: Thu, 18 May 2017 13:30:12 +0200

Arun Isaac <arunisaac <at> systemreboot.net> skribis:

> * guix/scripts/lint.scm (check-source-file-name): Implement file name matching
>   with regular expression.

LGTM!

BTW, for all this patch series, please make sure

  make check TESTS=tests/lint.scm

still passes.

Thanks,
Ludo’.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 3/4] guix: lint: Check for version in source
 file name.
Date: Thu, 18 May 2017 13:32:08 +0200

Arun Isaac <arunisaac <at> systemreboot.net> skribis:

> * guix/scripts/lint.scm (check-source-file-name): Check for version in source
>   file name.

[...]

>    (define (origin-file-name-valid? origin)
> -    ;; Return #f if the source file name contains only a version or is #f;
> -    ;; indicates that the origin needs a 'file-name' field.
> +    ;; Return #f if the source file name is #f, contains only a version, or
> +    ;; does not contain a version; indicates that the origin needs a
> +    ;; 'file-name' field.
>      (let ((file-name (origin-actual-file-name origin))
>            (version (package-version package)))
>        (and 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".
> -           (not (string-match (string-append "^v?" version) file-name)))))
> +           (not (string-match (string-append "^v?" version) file-name))
> +           (string-match version file-name))))

What about simply:

  (string-prefix? (string-append (package-name package) "-"
                                 (package-version package))
                  file-name)

?

That’s a bit stricter but I think that’s what we expect.

Thanks,
Ludo’.

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Alex Kost <alezost <at> gmail.com>, 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 3/4] guix: lint: Check for version in source
 file name.
Date: Thu, 18 May 2017 23:29:53 +0530

>>    (define (origin-file-name-valid? origin)
>> -    ;; Return #f if the source file name contains only a version or is #f;
>> -    ;; indicates that the origin needs a 'file-name' field.
>> +    ;; Return #f if the source file name is #f, contains only a version, or
>> +    ;; does not contain a version; indicates that the origin needs a
>> +    ;; 'file-name' field.
>>      (let ((file-name (origin-actual-file-name origin))
>>            (version (package-version package)))
>>        (and 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".
>> -           (not (string-match (string-append "^v?" version) file-name)))))
>> +           (not (string-match (string-append "^v?" version) file-name))
>> +           (string-match version file-name))))
>
> What about simply:
>
>   (string-prefix? (string-append (package-name package) "-"
>                                  (package-version package))
>                   file-name)

This will break all those emacs, python, etc. packages that have
"emacs-", "python-" prefixes in the package-name, but not in their
source file names. We'll have to add the file-name field to practically
every Guix package. I'm not sure this is a good idea.

Couldn't we drop patch 3, and just use patch 4 to fix this bug?

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

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 2/4] guix: lint: Slightly simplify
 `check-source-file-name'.
Date: Thu, 18 May 2017 23:30:59 +0530

> BTW, for all this patch series, please make sure
>
>   make check TESTS=tests/lint.scm
>
> still passes.

Noted, thanks!

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: Alex Kost <alezost <at> gmail.com>, 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 3/4] guix: lint: Check for version in source
 file name.
Date: Thu, 18 May 2017 23:01:55 +0200

Arun Isaac <arunisaac <at> systemreboot.net> skribis:

>>>    (define (origin-file-name-valid? origin)
>>> -    ;; Return #f if the source file name contains only a version or is #f;
>>> -    ;; indicates that the origin needs a 'file-name' field.
>>> +    ;; Return #f if the source file name is #f, contains only a version, or
>>> +    ;; does not contain a version; indicates that the origin needs a
>>> +    ;; 'file-name' field.
>>>      (let ((file-name (origin-actual-file-name origin))
>>>            (version (package-version package)))
>>>        (and 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".
>>> -           (not (string-match (string-append "^v?" version) file-name)))))
>>> +           (not (string-match (string-append "^v?" version) file-name))
>>> +           (string-match version file-name))))
>>
>> What about simply:
>>
>>   (string-prefix? (string-append (package-name package) "-"
>>                                  (package-version package))
>>                   file-name)
>
> This will break all those emacs, python, etc. packages that have
> "emacs-", "python-" prefixes in the package-name, but not in their
> source file names. We'll have to add the file-name field to practically
> every Guix package. I'm not sure this is a good idea.

Oh right, my bad.

> Couldn't we drop patch 3, and just use patch 4 to fix this bug?

Well patch 3 is OK after all, if you want to push it.

As for the patch 4, I prefer to let Alex reply!

Thanks,
Ludo’.

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 4/4] build: emacs: Fix
 `store-file->elisp-source-file'.
Date: Sun, 21 May 2017 11:33:45 +0300

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

> This prevents a ".el.el" extension for source files with no version number in
> their file name.
>
> * guix/build/emacs-build-system.scm (store-file->elisp-source-file): Remove
>   ".el" extension from file name before splitting to name and version.
> ---
>  guix/build/emacs-build-system.scm | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
> index 44e8b0d31..25a3beaa2 100644
> --- a/guix/build/emacs-build-system.scm
> +++ b/guix/build/emacs-build-system.scm
> @@ -47,10 +47,13 @@
>  (define (store-file->elisp-source-file file)
>    "Convert FILE, a store file name for an Emacs Lisp source file, into a file
>  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")))
> +  (let ((extension ".el"))
> +    (let-values (((name version)
> +                  (package-name->name+version
> +                   (strip-store-file-name
> +                    (string-drop-right
> +                     file (string-length extension))))))
> +      (string-append name extension))))
>  
>  (define* (unpack #:key source #:allow-other-keys)
>    "Unpack SOURCE into the build directory.  SOURCE may be a compressed

Wow, I don't know if removing ".el" from a file name can be written in a
more simple way, but the point of the patch is great!  I didn't realize
this problem could be fixed so easily.  It looks good to me, thanks!

-- 
Alex

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

From: Alex Kost <alezost <at> gmail.com>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: Arun Isaac <arunisaac <at> systemreboot.net>, 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 3/4] guix: lint: Check for version in source
 file name.
Date: Sun, 21 May 2017 11:44:13 +0300

Ludovic Courtès (2017-05-18 23:01 +0200) wrote:

> Arun Isaac <arunisaac <at> systemreboot.net> skribis:
[...]
>> Couldn't we drop patch 3, and just use patch 4 to fix this bug?
>
> Well patch 3 is OK after all, if you want to push it.
>
> As for the patch 4, I prefer to let Alex reply!

It sounds like I'm considered to be an expert in that part of code.
Thanks, but note that it's Federico who wrote emacs-build-system, and
it's David who wrote the code that Arun is modifying with this patch.

Well, I mean I know something about emacs-build-system and what the
topic problem is, but it's too much credits for me :-)

-- 
Alex

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 3/4] guix: lint: Check for version in source
 file name.
Date: Sun, 21 May 2017 12:00:47 +0300

Arun Isaac (2017-05-18 23:29 +0530) wrote:

>>>    (define (origin-file-name-valid? origin)
>>> -    ;; Return #f if the source file name contains only a version or is #f;
>>> -    ;; indicates that the origin needs a 'file-name' field.
>>> +    ;; Return #f if the source file name is #f, contains only a version, or
>>> +    ;; does not contain a version; indicates that the origin needs a
>>> +    ;; 'file-name' field.
>>>      (let ((file-name (origin-actual-file-name origin))
>>>            (version (package-version package)))
>>>        (and 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".
>>> -           (not (string-match (string-append "^v?" version) file-name)))))
>>> +           (not (string-match (string-append "^v?" version) file-name))
>>> +           (string-match version file-name))))
>>
>> What about simply:
>>
>>   (string-prefix? (string-append (package-name package) "-"
>>                                  (package-version package))
>>                   file-name)
>
> This will break all those emacs, python, etc. packages that have
> "emacs-", "python-" prefixes in the package-name, but not in their
> source file names. We'll have to add the file-name field to practically
> every Guix package. I'm not sure this is a good idea.

Well, it will not "break" the packages, it will just add many new lint
warnings.  But I agree that the Ludovic's version is too strict.

What I would prefer is to make this linter check for "name-version" (or
for "name" and "version" separately) inside a source file name (not with
'string-prefix?', but with 'string-match'), so that the store file names
will look like this:

  foo-0.1.tar
  emacs-bar-0.2.el

and not like this:

  v0.1.tar
  emacs-bar.el

> Couldn't we drop patch 3, and just use patch 4 to fix this bug?

I think so, patch 4 is definitely a fix for the original problem, while
a general file-name linting is probably for another thread.

-- 
Alex

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

From: Alex Kost <alezost <at> gmail.com>
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: Single source file emacs packages get a ".el.el"
 extension
Date: Sun, 21 May 2017 12:03:02 +0300

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

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Alex Kost <alezost <at> gmail.com>
Cc: Arun Isaac <arunisaac <at> systemreboot.net>, 26802 <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 4/4] build: emacs: Fix
 `store-file->elisp-source-file'.
Date: Mon, 22 May 2017 00:24:08 +0200

Alex Kost <alezost <at> gmail.com> skribis:

>> +  (let ((extension ".el"))
>> +    (let-values (((name version)
>> +                  (package-name->name+version
>> +                   (strip-store-file-name
>> +                    (string-drop-right
>> +                     file (string-length extension))))))
>> +      (string-append name extension))))

Pro tip: (string-drop-right …) can be replaced by:

  (basename file ".el")

:-)

Ludo’.

Message #91 received at 26802-done <at> debbugs.gnu.org (full text, mbox):

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802-done <at> debbugs.gnu.org
Cc: Alex Kost <alezost <at> gmail.com>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: bug#26802: [PATCH 4/4] build: emacs: Fix
 `store-file->elisp-source-file'.
Date: Tue, 23 May 2017 04:40:44 +0530

> Pro tip: (string-drop-right …) can be replaced by:
>
>   (basename file ".el")

Thanks! Made this modification...

Pushed patches 1, 2 and 4.

Message #94 received at 26802-done <at> debbugs.gnu.org (full text, mbox):

From: Arun Isaac <arunisaac <at> systemreboot.net>
To: 26802-done <at> debbugs.gnu.org
Cc: Alex Kost <alezost <at> gmail.com>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: bug#26802: [PATCH 4/4] build: emacs: Fix
 `store-file->elisp-source-file'.
Date: Tue, 23 May 2017 06:22:01 +0530

> Pushed patches 1, 2 and 4.

Looks like I made a mistake with patch 4's commit message. I should have
used the prefix "build-system: emacs:", instead of "build:
emacs:". Sorry! :-(

Message #97 received at 26802-done <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Arun Isaac <arunisaac <at> systemreboot.net>
Cc: Alex Kost <alezost <at> gmail.com>, 26802-done <at> debbugs.gnu.org
Subject: Re: bug#26802: [PATCH 4/4] build: emacs: Fix
 `store-file->elisp-source-file'.
Date: Tue, 23 May 2017 10:15:45 +0200

Hey Arun,

Arun Isaac <arunisaac <at> systemreboot.net> skribis:

>> Pushed patches 1, 2 and 4.

Thank you!

> Looks like I made a mistake with patch 4's commit message. I should have
> used the prefix "build-system: emacs:", instead of "build:
> emacs:". Sorry! :-(

No big deal, that’s OK.

Cheers,
Ludo’.

Previous Next

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