bug#27222: emacs-build-system install phase doesn't honor directory hierarchy

bug#27222: emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

It seems the recent changes made to the emacs-build-system broke
ert-runner; it always output the following:

Invalid reporter: dot

A quick investigation revealed that ert-runner.el is looking for
reporters under the reporters/ sudbdirectory. Here's the arborescence in
the original source:

--8<---------------cut here---------------start------------->8---
find . -name '*.el'
./reporters/ert-runner-reporter-ert.el
./reporters/ert-runner-reporter-dot.el
./features/step-definitions/ert-runner-steps.el
./features/support/env.el
./ert-runner.el
./ert-compat.el
--8<---------------cut here---------------end--------------->8---

And here's what got installed in the store:

--8<---------------cut here---------------start------------->8---
find . -name '*.el'
./ert-runner-autoloads.el
./ert-runner.el
./ert-compat.el
--8<---------------cut here---------------end--------------->8---

I'm guessing the current inclusion regexp:
(include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))

must be extended/adapted.

What should "^[^/]*\\.el$" become?

I've tried ".*\\.el" without success.

Ideas?

Maxim

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

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

Hello,

The previous regexp would have worked, but what got me was that the
default keyword arguments values were duplicated (and I was fixing the
useless version in build/emac-build-system.scm instead of the one used
in build-system/emacs.scm).

The attached patch fixes this particular problem (tested with
ert-runner).

Thanks,

Maxim


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-build-system-emacs-Install-elisp-files-from-subdirec.patch --]
[-- Type: text/x-patch, Size: 2028 bytes --]

From a035d07dfa6cbddccfa0476e2009d19bdf296941 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sat, 3 Jun 2017 23:43:02 -0700
Subject: [PATCH] build-system: emacs: Install elisp files from subdirectories

* guix/build/emacs-build-system.scm (install)[include]: Get rid of default
value.
[exclude]: Likewise.
* guix/build/emacs-build-system.scm (emacs-build)[include]: Modify default
regexp value so that elisp files get matched (and installed) for any directory
depth level.
---
 guix/build-system/emacs.scm       | 2 +-
 guix/build/emacs-build-system.scm | 5 +----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/guix/build-system/emacs.scm b/guix/build-system/emacs.scm
index 9a46ecfd2..a97fcedc3 100644
--- a/guix/build-system/emacs.scm
+++ b/guix/build-system/emacs.scm
@@ -83,7 +83,7 @@
                       (phases '(@ (guix build emacs-build-system)
                                   %standard-phases))
                       (outputs '("out"))
-                      (include ''("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
+                      (include ''("\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
                       (exclude ''("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
                       (search-paths '())
                       (system (%current-system))
diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 50af4be36..1373cb6f7 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -95,10 +95,7 @@ store in '.el' files."
           (substitute-cmd))))
     #t))
 
-(define* (install #:key outputs
-                  (include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
-                  (exclude '("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
-                  #:allow-other-keys)
+(define* (install #:key outputs include exclude #:allow-other-keys)
   "Install the package contents."
 
   (define source (getcwd))
-- 
2.13.0

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Alex Kost]

I Cc-ed Arun, the author of the mentioned change (commit
d879685176d23c111f4fc665698251b25cdf9124).

[...]
> From a035d07dfa6cbddccfa0476e2009d19bdf296941 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
> Date: Sat, 3 Jun 2017 23:43:02 -0700
> Subject: [PATCH] build-system: emacs: Install elisp files from subdirectories
>
> * guix/build/emacs-build-system.scm (install)[include]: Get rid of default
> value.
> [exclude]: Likewise.
> * guix/build/emacs-build-system.scm (emacs-build)[include]: Modify default
> regexp value so that elisp files get matched (and installed) for any directory
> depth level.
> ---
>  guix/build-system/emacs.scm       | 2 +-
>  guix/build/emacs-build-system.scm | 5 +----
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/guix/build-system/emacs.scm b/guix/build-system/emacs.scm
> index 9a46ecfd2..a97fcedc3 100644
> --- a/guix/build-system/emacs.scm
> +++ b/guix/build-system/emacs.scm
> @@ -83,7 +83,7 @@
>                        (phases '(@ (guix build emacs-build-system)
>                                    %standard-phases))
>                        (outputs '("out"))
> -                      (include ''("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
> +                      (include ''("\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))

As far as I understand it, it was done for purpose: some packages
include "uninteresting" (for tests, maintenance, etc.) *.el files in
subdirs, that's why they are excluded by default.  So probably a better
solution would be to fix 'ert-runner' package (as it is done in commit
b1d32ec0e23bfec1dab4c56909228a494b2b0d60, for example).  WDYT?

>                        (exclude ''("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
>                        (search-paths '())
>                        (system (%current-system))
> diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
> index 50af4be36..1373cb6f7 100644
> --- a/guix/build/emacs-build-system.scm
> +++ b/guix/build/emacs-build-system.scm
> @@ -95,10 +95,7 @@ store in '.el' files."
>            (substitute-cmd))))
>      #t))
>  
> -(define* (install #:key outputs
> -                  (include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
> -                  (exclude '("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
> -                  #:allow-other-keys)
> +(define* (install #:key outputs include exclude #:allow-other-keys)
>    "Install the package contents."

I also think these arguments are redundant!  I suggested to remove this
duplication at:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#41

-- 
Alex

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

Hello,

Alex Kost <alezost@gmail.com> writes:

> I Cc-ed Arun, the author of the mentioned change (commit
> d879685176d23c111f4fc665698251b25cdf9124).
>
> [...]
>> From a035d07dfa6cbddccfa0476e2009d19bdf296941 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Sat, 3 Jun 2017 23:43:02 -0700
>> Subject: [PATCH] build-system: emacs: Install elisp files from subdirectories
>>
>> * guix/build/emacs-build-system.scm (install)[include]: Get rid of default
>> value.
>> [exclude]: Likewise.
>> * guix/build/emacs-build-system.scm (emacs-build)[include]: Modify default
>> regexp value so that elisp files get matched (and installed) for any directory
>> depth level.
>> ---
>>  guix/build-system/emacs.scm       | 2 +-
>>  guix/build/emacs-build-system.scm | 5 +----
>>  2 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/guix/build-system/emacs.scm b/guix/build-system/emacs.scm
>> index 9a46ecfd2..a97fcedc3 100644
>> --- a/guix/build-system/emacs.scm
>> +++ b/guix/build-system/emacs.scm
>> @@ -83,7 +83,7 @@
>>                        (phases '(@ (guix build emacs-build-system)
>>                                    %standard-phases))
>>                        (outputs '("out"))
>> -                      (include ''("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
>> +                      (include ''("\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
>
> As far as I understand it, it was done for purpose: some packages
> include "uninteresting" (for tests, maintenance, etc.) *.el files in
> subdirs, that's why they are excluded by default.  So probably a better
> solution would be to fix 'ert-runner' package (as it is done in commit
> b1d32ec0e23bfec1dab4c56909228a494b2b0d60, for example).  WDYT?

I acknowledge the intent, but I think the default set of rexgeps
should be more lenient; filtering (inoffensive) files is desirable, but
not at the cost of breaking perfectly valid packages. That's one extra
hurdle the packagers shouldn't have to bear in my opinion.

This change also doesn't prevent excluding subfolders if they are truly
unnecessary (such as tests subfolder), but this should happen due to
explicit regexp in the exclude option, not because *all* subfolders are
excluded.

What do you think?

Maxim

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Alex Kost]

Maxim Cournoyer (2017-06-04 09:44 -0700) wrote:

[...]
>>>                        (phases '(@ (guix build emacs-build-system)
>>>                                    %standard-phases))
>>>                        (outputs '("out"))
>>> -                      (include ''("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
>>> +                      (include ''("\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
>>
>> As far as I understand it, it was done for purpose: some packages
>> include "uninteresting" (for tests, maintenance, etc.) *.el files in
>> subdirs, that's why they are excluded by default.  So probably a better
>> solution would be to fix 'ert-runner' package (as it is done in commit
>> b1d32ec0e23bfec1dab4c56909228a494b2b0d60, for example).  WDYT?
>
> I acknowledge the intent, but I think the default set of rexgeps
> should be more lenient; filtering (inoffensive) files is desirable, but
> not at the cost of breaking perfectly valid packages. That's one extra
> hurdle the packagers shouldn't have to bear in my opinion.
>
> This change also doesn't prevent excluding subfolders if they are truly
> unnecessary (such as tests subfolder), but this should happen due to
> explicit regexp in the exclude option, not because *all* subfolders are
> excluded.
>
> What do you think?

I think my view is not what most people would like: I am for excluding
as much as possible (as it is now), and for manual adjusting packages
when it is needed.

-- 
Alex

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Arun Isaac]

> As far as I understand it, it was done for purpose: some packages
> include "uninteresting" (for tests, maintenance, etc.) *.el files in
> subdirs, that's why they are excluded by default.  So probably a better
> solution would be to fix 'ert-runner' package (as it is done in commit
> b1d32ec0e23bfec1dab4c56909228a494b2b0d60, for example).  WDYT?

I agree. The solution is to fix the ert-runner package, not the
emacs-build-system.

> This change also doesn't prevent excluding subfolders if they are truly
> unnecessary (such as tests subfolder), but this should happen due to
> explicit regexp in the exclude option, not because *all* subfolders are
> excluded.

We adopted the policy of excluding *all* subfolders from MELPA. From
their "Recipe Format" section at https://github.com/melpa/melpa

"Note that elisp in subdirectories is never included by default, so you
might find it convenient to separate auxiliiary files such as tests into
subdirectories to keep packaging simple."

I think this is a good policy. If we include subfolders by default,
we'll have to modify many packages with #:exclude arguments to get rid
of unnecessary subfolders. However, if we exclude subfolders by default,
we'll only have to modify fewer packages with #:include arguments.

> I also think these arguments are redundant!  I suggested to remove this
> duplication at:
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#41

And, I did respond at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#53

> ... but I think the include/exclude arguments need to be duplicated in
> two places. For example, look at arguments #:strip-flags and
> #:strip-directories in the `strip' phase of the gnu-build-system. Even
> there, the default values of the arguments are repeated in two places.

Do you know of some way in which we can avoid duplication of the
arguments? Even the gnu-build-system duplicates default values of
arguments.

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

[-- Attachment #1.1: Type: text/plain, Size: 2961 bytes --]

Hello,

Arun Isaac <arunisaac@systemreboot.net> writes:

>> As far as I understand it, it was done for purpose: some packages
>> include "uninteresting" (for tests, maintenance, etc.) *.el files in
>> subdirs, that's why they are excluded by default.  So probably a better
>> solution would be to fix 'ert-runner' package (as it is done in commit
>> b1d32ec0e23bfec1dab4c56909228a494b2b0d60, for example).  WDYT?
>
> I agree. The solution is to fix the ert-runner package, not the
> emacs-build-system.
>
>> This change also doesn't prevent excluding subfolders if they are truly
>> unnecessary (such as tests subfolder), but this should happen due to
>> explicit regexp in the exclude option, not because *all* subfolders are
>> excluded.
>
> We adopted the policy of excluding *all* subfolders from MELPA. From
> their "Recipe Format" section at https://github.com/melpa/melpa
>
> "Note that elisp in subdirectories is never included by default, so you
> might find it convenient to separate auxiliiary files such as tests into
> subdirectories to keep packaging simple."

Oh. I didn't know MELPA had such a policy. This is a good point. It's
nice to try to stick to whatever MELPA does, as they've pretty much
become the authority in Elisp packaging IIUC.

> I think this is a good policy. If we include subfolders by default,
> we'll have to modify many packages with #:exclude arguments to get rid
> of unnecessary subfolders. However, if we exclude subfolders by default,
> we'll only have to modify fewer packages with #:include arguments.

Although, for the sake of cleanliness, they enforce a project layout
that discourage the organization of a project in subdirectories, which I
find a bit strange. But if the lack of complaints about it in the MELPA
issues tracker tells anything, it doesn't seem to be much of a bother
for most projects (this might have to do with the fact that until
recently most Elisp projects were organized as a single file of
thousands of lines of code ;).

>> I also think these arguments are redundant!  I suggested to remove this
>> duplication at:
>>
>>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#41
>
> And, I did respond at
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#53
>
>> ... but I think the include/exclude arguments need to be duplicated in
>> two places. For example, look at arguments #:strip-flags and
>> #:strip-directories in the `strip' phase of the gnu-build-system. Even
>> there, the default values of the arguments are repeated in two places.

> Do you know of some way in which we can avoid duplication of the
> arguments? Even the gnu-build-system duplicates default values of
> arguments.

I've decided to go with the flow and modify ert-runner so that it
includes the elisp files under the 'reporters' subdirectory. I've also
factorized out the default args of the include and exclude option of the
emacs-build-system install phase. Please see the two patches attached.

Maxim


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-build-system-emacs-Factorize-include-exclude-default.patch --]
[-- Type: text/x-patch, Size: 3709 bytes --]

From 626eb2b0551aee16836b7ec796a8ad1759be5a52 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sat, 3 Jun 2017 23:43:02 -0700
Subject: [PATCH 1/2] build-system: emacs: Factorize include/exclude default
 args

The `install' phase of the emac-build-system builder side contained arguments
duplicated from the higer level `emacs-build' procedure. This change
factorizes them so that:

1. They are not duplicated;
2. They can be reused and extended easily when defining Emacs packages.

* guix/build/emacs-build-system.scm (%default-include): New symbol.
(%default-exclude): Likewise.
(install)[include]: Use %default-include variable.
[exclude]: Use %default exclude variable.
---
 guix/build-system/emacs.scm       | 11 ++++++++---
 guix/build/emacs-build-system.scm | 11 +++++++++--
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/guix/build-system/emacs.scm b/guix/build-system/emacs.scm
index 9a46ecfd2..02296829c 100644
--- a/guix/build-system/emacs.scm
+++ b/guix/build-system/emacs.scm
@@ -17,6 +17,8 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix build-system emacs)
+  #:use-module ((guix build emacs-build-system)
+                #:select (%default-include %default-exclude))
   #:use-module (guix store)
   #:use-module (guix utils)
   #:use-module (guix packages)
@@ -28,7 +30,10 @@
   #:use-module (srfi srfi-26)
   #:export (%emacs-build-system-modules
             emacs-build
-            emacs-build-system))
+            emacs-build-system)
+  #:re-export (%default-include         ;for convenience
+               %default-exclude))
+
 
 ;; Commentary:
 ;;
@@ -83,8 +88,8 @@
                       (phases '(@ (guix build emacs-build-system)
                                   %standard-phases))
                       (outputs '("out"))
-                      (include ''("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
-                      (exclude ''("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
+                      (include (quote %default-include))
+                      (exclude (quote %default-exclude))
                       (search-paths '())
                       (system (%current-system))
                       (guile #f)
diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 50af4be36..bda699ddf 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -29,6 +29,8 @@
   #:use-module (ice-9 regex)
   #:use-module (ice-9 match)
   #:export (%standard-phases
+            %default-include
+            %default-exclude
             emacs-build))
 
 ;; Commentary:
@@ -42,6 +44,11 @@
 ;; archive signature.
 (define %install-suffix "/share/emacs/site-lisp/guix.d")
 
+;; These are the default inclusion/exclusion regexps for the install phase.
+(define %default-include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
+(define %default-exclude '("^\\.dir-locals\\.el$" "-pkg\\.el$"
+                           "^[^/]*tests?\\.el$"))
+
 (define gnu:unpack (assoc-ref gnu:%standard-phases 'unpack))
 
 (define (store-file->elisp-source-file file)
@@ -96,8 +103,8 @@ store in '.el' files."
     #t))
 
 (define* (install #:key outputs
-                  (include '("^[^/]*\\.el$" "^[^/]*\\.info$" "^doc/.*\\.info$"))
-                  (exclude '("^\\.dir-locals\\.el$" "-pkg\\.el$" "^[^/]*tests?\\.el$"))
+                  (include %default-include)
+                  (exclude %default-exclude)
                   #:allow-other-keys)
   "Install the package contents."
 
-- 
2.13.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: 0002-gnu-emacs-Fix-ert-runner-by-adding-reporters-subdir.patch --]
[-- Type: text/x-patch, Size: 1210 bytes --]

From ffb86810fe945a6cded4b5363ef0b8ce4ea58d02 Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Sun, 4 Jun 2017 20:57:03 -0700
Subject: [PATCH 2/2] gnu: emacs: Fix ert-runner by adding 'reporters' subdir

Previous this change, ert-runner would fail with error: "Invalid reporter: dot".

* gnu/packages/emacs.scm (ert-runner)[include]: Add regexp to match elisp
files under the 'reporters' subdirectory.
---
 gnu/packages/emacs.scm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/emacs.scm b/gnu/packages/emacs.scm
index 81a74d1fb..52118099e 100644
--- a/gnu/packages/emacs.scm
+++ b/gnu/packages/emacs.scm
@@ -4814,7 +4814,8 @@ Emacs.")
                           ;; determined by emacs' standard initialization
                           ;; procedure
                           (list ""))))
-                 #t))))))
+                 #t))))
+         #:include (cons* "^reporters/.*\\.el$" %default-include)))
       (home-page "https://github.com/rejeep/ert-runner.el")
       (synopsis "Opinionated Ert testing workflow")
       (description "@code{ert-runner} is a tool for Emacs projects tested
-- 
2.13.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Arun Isaac]

> this might have to do with the fact that until recently most Elisp
> projects were organized as a single file of thousands of lines of code
> ;).

Exactly! :-)

> I've also factorized out the default args of the include and exclude
> option of the emacs-build-system install phase. Please see the two
> patches attached.

Thanks! Both patches LGTM. I just have a few doubts and minor comments.

>  (define-module (guix build-system emacs)
> +  #:use-module ((guix build emacs-build-system)
> +                #:select (%default-include %default-exclude))

Interesting solution to the problem. I didn't think of this
earlier. Importing host side to build side would be a problem, but you
are importing build side to host side, and I think that's ok.

>    #:use-module (guix store)
>    #:use-module (guix utils)
>    #:use-module (guix packages)
> @@ -28,7 +30,10 @@
>    #:use-module (srfi srfi-26)
>    #:export (%emacs-build-system-modules
>              emacs-build
> -            emacs-build-system))
> +            emacs-build-system)
> +  #:re-export (%default-include         ;for convenience
> +               %default-exclude))

What is this re-export for? ert-runner seems to build fine without
this. I don't understand what you mean by "convenience" here.

Also, perhaps we should deduplicate default values of arguments in the
gnu-build-system as well. But, I don't know if it is worth deduplicating
simple default values like #t. WDYT?

@Alex Kost:

Please make any other changes you think are necessary, and push. Thanks!

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

Hello Arun,

Arun Isaac <arunisaac@systemreboot.net> writes:

>> this might have to do with the fact that until recently most Elisp
>> projects were organized as a single file of thousands of lines of code
>> ;).
>
> Exactly! :-)
>
>> I've also factorized out the default args of the include and exclude
>> option of the emacs-build-system install phase. Please see the two
>> patches attached.
>
> Thanks! Both patches LGTM. I just have a few doubts and minor comments.
>

Thanks for the prompt review!

>>  (define-module (guix build-system emacs)
>> +  #:use-module ((guix build emacs-build-system)
>> +                #:select (%default-include %default-exclude))
>
> Interesting solution to the problem. I didn't think of this
> earlier. Importing host side to build side would be a problem, but you
> are importing build side to host side, and I think that's ok.
>
>>    #:use-module (guix store)
>>    #:use-module (guix utils)
>>    #:use-module (guix packages)
>> @@ -28,7 +30,10 @@
>>    #:use-module (srfi srfi-26)
>>    #:export (%emacs-build-system-modules
>>              emacs-build
>> -            emacs-build-system))
>> +            emacs-build-system)
>> +  #:re-export (%default-include         ;for convenience
>> +               %default-exclude))
>
> What is this re-export for? ert-runner seems to build fine without
> this. I don't understand what you mean by "convenience" here.

If you take another look at the second patch I sent, you'll see I build
the include argument value of the emacs-build-sysltem by consing the new
regexp in front of %default-include instead of building it from
scratch. That's what these re-exports are for: they enable their use in
gnu/packages/emacs.scm (and anywhere we've imported (guix build-system emacs).

> Also, perhaps we should deduplicate default values of arguments in the
> gnu-build-system as well. But, I don't know if it is worth deduplicating
> simple default values like #t. WDYT?

There's not much value in deduplicating simple things such as a boolean
flag, but for more complicated ones it could be useful; although it
would be very nice if instead of having to do this Scheme/Guile offered
something for "querying" the defaults values of function. One could then
go and extend the defaults rather than override them them in a similar
way to what we can do with records.

Maxim

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Alex Kost]

Arun Isaac (2017-06-05 15:33 +0530) wrote:

> Please make any other changes you think are necessary, and push. Thanks!

Thanks, but I'm afraid I'm not competent to judge about the first
patch.  I don't really know what is used on build side and on host side,
and whether these things can be mixed like that.

-- 
Alex

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Arun Isaac]

Pushed! :-)

Made a few minor modifications to the commit message...

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Maxim Cournoyer]

Arun Isaac <arunisaac@systemreboot.net> writes:

> Pushed! :-)
>
> Made a few minor modifications to the commit message...

Thank you, Arun!

Maxim

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Alex Kost]

Arun Isaac (2017-06-05 00:55 +0530) wrote:

[...]
>> I also think these arguments are redundant!  I suggested to remove this
>> duplication at:
>>
>>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#41
>
> And, I did respond at
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26559#53

Sure, I didn't mean I was ignored, I just wanted to say that I got the
same thought about those arguments as Maxim.

>> ... but I think the include/exclude arguments need to be duplicated in
>> two places. For example, look at arguments #:strip-flags and
>> #:strip-directories in the `strip' phase of the gnu-build-system. Even
>> there, the default values of the arguments are repeated in two places.
>
> Do you know of some way in which we can avoid duplication of the
> arguments?

As I said, I think this duplication can be avoided simply by removing
the values of 'include'/'exclude' arguments from 'install' procedures of
(guix build emacs-build-system) module.

> Even the gnu-build-system duplicates default values of arguments.

I don't know about gnu-build-system, but I don't see a reason to have
these values in 'install' procedure of emacs-build-system.

-- 
Alex

bug#27222: [PATCH] emacs-build-system install phase doesn't honor directory hierarchy

[Arun Isaac]

Alex Kost writes:

> Sure, I didn't mean I was ignored, I just wanted to say that I got the
> same thought about those arguments as Maxim.

No hard feelings! :-) debbugs can be a pain to read. I thought you
missed that message from me.

> As I said, I think this duplication can be avoided simply by removing
> the values of 'include'/'exclude' arguments from 'install' procedures of
> (guix build emacs-build-system) module.

I tried something similar when I wrote the original patch, but I
couldn't get it working. Probably something to do with all the quotes
and build side/host side stuff. I didn't pursue it further when I
noticed that even the gnu-build-system duplicates default arguments.

bug#27222: [PATCH] Fix ert-runner regression (was: emacs-build-system install phase doesn't honor directory hierarchy)

[Maxim Cournoyer]

Hello Ludovic,

Alex Kost (2017-06-05) wrote:

> Arun Isaac (2017-06-05) wrote:

> > Please make any other changes you think are necessary, and push. Thanks!

> Thanks, but I'm afraid I'm not competent to judge about the first
> patch.  I don't really know what is used on build side and on host side,
> and whether these things can be mixed like that.

> -- 
> Alex

Could you please have a quick look at the small change I did in the
first patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27222#23?
If it is sane, feel free to merge those (as suggested by Arun) and close
this bug :)

Thanks,

Maxim

bug#27222: [PATCH] Fix ert-runner regression

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> Alex Kost (2017-06-05) wrote:
>
>> Arun Isaac (2017-06-05) wrote:
>
>> > Please make any other changes you think are necessary, and push. Thanks!
>
>> Thanks, but I'm afraid I'm not competent to judge about the first
>> patch.  I don't really know what is used on build side and on host side,
>> and whether these things can be mixed like that.
>
>> -- 
>> Alex
>
> Could you please have a quick look at the small change I did in the
> first patch at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27222#23?
> If it is sane, feel free to merge those (as suggested by Arun) and close
> this bug :)

Sorry I haven’t closely followed this discussion.  Arun: Could you merge
it if that looks good to you?  I trust your judgment, comrades.  :-)

TIA!

Ludo’.

PS: Apologies, I’ll be less responsive over the next few days.

bug#27222: [PATCH] Fix ert-runner regression

[Arun Isaac]

> Sorry I haven’t closely followed this discussion.  Arun: Could you merge
> it if that looks good to you?  I trust your judgment, comrades.  :-)

The patches are fine by me. Alex Kost is reviewing this patch. So, I
think he should push. Thanks!

bug#27222: [PATCH] Fix ert-runner regression

[Alex Kost]

Arun Isaac (2017-06-07 15:58 +0530) wrote:

>> Sorry I haven’t closely followed this discussion.  Arun: Could you merge
>> it if that looks good to you?  I trust your judgment, comrades.  :-)
>
> The patches are fine by me. Alex Kost is reviewing this patch. So, I
> think he should push. Thanks!

Wow, so if someone replies to some message, (s)he is considered to be a
reviewer?  I didn't realize this is so strict :-)

I just replied to the original Maxim's proposal to include elisp files
from subdirs.  After that, the patch changed completely, and I'm not
confident in the potential consequences, so I let other people judge on
that patch.

Arun, please commit it, if it looks good to you.

-- 
Alex