bug#26802: [PATCH 2/4] guix: lint: Slightly simplify `check-source-file-name'.

bug#26802: [PATCH 2/4] guix: lint: Slightly simplify `check-source-file-name'.

[Arun Isaac]

* 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

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Arun Isaac]

* 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

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Arun Isaac]

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

bug#26802: [PATCH 2/4] guix: lint: Slightly simplify `check-source-file-name'.

[Ludovic Courtès]

Arun Isaac <arunisaac@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’.

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Ludovic Courtès]

Arun Isaac <arunisaac@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’.

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Arun Isaac]

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

bug#26802: [PATCH 2/4] guix: lint: Slightly simplify `check-source-file-name'.

[Arun Isaac]

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

Noted, thanks!

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Ludovic Courtès]

Arun Isaac <arunisaac@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’.

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Alex Kost]

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

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Alex Kost]

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

> Arun Isaac <arunisaac@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

bug#26802: [PATCH 3/4] guix: lint: Check for version in source file name.

[Alex Kost]

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

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Ludovic Courtès]

Alex Kost <alezost@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’.

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Arun Isaac]

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

Thanks! Made this modification...

Pushed patches 1, 2 and 4.

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Arun Isaac]

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

bug#26802: [PATCH 4/4] build: emacs: Fix `store-file->elisp-source-file'.

[Ludovic Courtès]

Hey Arun,

Arun Isaac <arunisaac@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’.