Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Jackson Hamilton]

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

The contents of these test fixtures are significant.  Changing them caused
the project's tests to fail.  Can byte compilation be disabled for these
files without modifying their source? (For instance, by disabling
byte-compilation for the whole fixtures/ directory?  Maybe through
.dir-locals.el?  Or maybe by respecting .elpaignore?)

On Thu, Aug 4, 2016 at 6:37 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> branch: master
> commit b7d8d3ca7f1bc279a3e8b21646ccea481c93ed46
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>
>     * fixtures/test/: Don't byte-compile
> ---
>  packages/context-coloring/fixtures/test/cond.el                   |    1
> +
>  packages/context-coloring/fixtures/test/condition-case.el         |    1
> +
>  packages/context-coloring/fixtures/test/defun.el                  |    1
> +
>  packages/context-coloring/fixtures/test/ignored.el                |    1
> +
>  packages/context-coloring/fixtures/test/let.el                    |    1
> +
>  packages/context-coloring/fixtures/test/macroexp-let2.el          |    1
> +
>  packages/context-coloring/fixtures/test/unbalanced-parenthesis.el |    1
> +
>  packages/context-coloring/fixtures/test/varlist-spacing.el        |    1
> +
>  8 files changed, 8 insertions(+)
>
> diff --git a/packages/context-coloring/fixtures/test/cond.el
> b/packages/context-coloring/fixtures/test/cond.el
> index d5aae5b..5ed1ec2 100644
> --- a/packages/context-coloring/fixtures/test/cond.el
> +++ b/packages/context-coloring/fixtures/test/cond.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (let (a)
>    (cond
>     (a t)
> diff --git a/packages/context-coloring/fixtures/test/condition-case.el
> b/packages/context-coloring/fixtures/test/condition-case.el
> index 151f591..56a5f44 100644
> --- a/packages/context-coloring/fixtures/test/condition-case.el
> +++ b/packages/context-coloring/fixtures/test/condition-case.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (condition-case err
>      (progn err free)
>    (error err free)
> diff --git a/packages/context-coloring/fixtures/test/defun.el
> b/packages/context-coloring/fixtures/test/defun.el
> index 10a52f6..173ba0b 100644
> --- a/packages/context-coloring/fixtures/test/defun.el
> +++ b/packages/context-coloring/fixtures/test/defun.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (defun abc (def ghi &optional jkl)
>    (+ def ghi jkl free))
>
> diff --git a/packages/context-coloring/fixtures/test/ignored.el
> b/packages/context-coloring/fixtures/test/ignored.el
> index 1f5fd42..0c540ff 100644
> --- a/packages/context-coloring/fixtures/test/ignored.el
> +++ b/packages/context-coloring/fixtures/test/ignored.el
> @@ -1,2 +1,3 @@
> +;; -*- no-byte-compile:t' -*-
>  (defun a ()
>    (+ a 1 +1 -1 1.0 #x0 ,a   :a t nil (0 . 0)))
> diff --git a/packages/context-coloring/fixtures/test/let.el
> b/packages/context-coloring/fixtures/test/let.el
> index 49edb50..761a265 100644
> --- a/packages/context-coloring/fixtures/test/let.el
> +++ b/packages/context-coloring/fixtures/test/let.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (let (a
>        (b a)
>        (c free)
> diff --git a/packages/context-coloring/fixtures/test/macroexp-let2.el
> b/packages/context-coloring/fixtures/test/macroexp-let2.el
> index 1b61df2..97fbb23 100644
> --- a/packages/context-coloring/fixtures/test/macroexp-let2.el
> +++ b/packages/context-coloring/fixtures/test/macroexp-let2.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (let (exp)
>    (macroexp-let2 macroexp-copyable-p v exp
>      v exp))
> diff --git a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
> b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
> index caaf7e2..e028aef 100644
> --- a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
> +++ b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
> @@ -1,2 +1,3 @@
> +;; -*- no-byte-compile:t' -*-
>  (let ())
>  (let ()
> diff --git a/packages/context-coloring/fixtures/test/varlist-spacing.el
> b/packages/context-coloring/fixtures/test/varlist-spacing.el
> index 97ec208..399cd18 100644
> --- a/packages/context-coloring/fixtures/test/varlist-spacing.el
> +++ b/packages/context-coloring/fixtures/test/varlist-spacing.el
> @@ -1,3 +1,4 @@
> +;; -*- no-byte-compile:t' -*-
>  (let (
>        (a (lambda ()))))
>
>

[-- Attachment #2: Type: text/html, Size: 5506 bytes --]

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Stefan Monnier]

> The contents of these test fixtures are significant.  Changing them caused
> the project's tests to fail.

Hmm... indeed I see that now.

> Can byte compilation be disabled for these
> files without modifying their source? (For instance, by disabling
> byte-compilation for the whole fixtures/ directory?  Maybe through
> .dir-locals.el?  Or maybe by respecting .elpaignore?)

Respecting .elpaignore would be perfect, indeed.  But that file is using
a format defined by tar, whereas I'd need to tweak the GNUmakefile rules
to understand it, which is rather tricky.

I'll see what I can come up with.


        Stefan


> On Thu, Aug 4, 2016 at 6:37 PM, Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:

>> branch: master
>> commit b7d8d3ca7f1bc279a3e8b21646ccea481c93ed46
>> Author: Stefan Monnier <monnier@iro.umontreal.ca>
>> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>> 
>> * fixtures/test/: Don't byte-compile
>> ---
>> packages/context-coloring/fixtures/test/cond.el                   |    1
>> +
>> packages/context-coloring/fixtures/test/condition-case.el         |    1
>> +
>> packages/context-coloring/fixtures/test/defun.el                  |    1
>> +
>> packages/context-coloring/fixtures/test/ignored.el                |    1
>> +
>> packages/context-coloring/fixtures/test/let.el                    |    1
>> +
>> packages/context-coloring/fixtures/test/macroexp-let2.el          |    1
>> +
>> packages/context-coloring/fixtures/test/unbalanced-parenthesis.el |    1
>> +
>> packages/context-coloring/fixtures/test/varlist-spacing.el        |    1
>> +
>> 8 files changed, 8 insertions(+)
>> 
>> diff --git a/packages/context-coloring/fixtures/test/cond.el
>> b/packages/context-coloring/fixtures/test/cond.el
>> index d5aae5b..5ed1ec2 100644
>> --- a/packages/context-coloring/fixtures/test/cond.el
>> +++ b/packages/context-coloring/fixtures/test/cond.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (let (a)
>> (cond
>> (a t)
>> diff --git a/packages/context-coloring/fixtures/test/condition-case.el
>> b/packages/context-coloring/fixtures/test/condition-case.el
>> index 151f591..56a5f44 100644
>> --- a/packages/context-coloring/fixtures/test/condition-case.el
>> +++ b/packages/context-coloring/fixtures/test/condition-case.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (condition-case err
>> (progn err free)
>> (error err free)
>> diff --git a/packages/context-coloring/fixtures/test/defun.el
>> b/packages/context-coloring/fixtures/test/defun.el
>> index 10a52f6..173ba0b 100644
>> --- a/packages/context-coloring/fixtures/test/defun.el
>> +++ b/packages/context-coloring/fixtures/test/defun.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (defun abc (def ghi &optional jkl)
>> (+ def ghi jkl free))
>> 
>> diff --git a/packages/context-coloring/fixtures/test/ignored.el
>> b/packages/context-coloring/fixtures/test/ignored.el
>> index 1f5fd42..0c540ff 100644
>> --- a/packages/context-coloring/fixtures/test/ignored.el
>> +++ b/packages/context-coloring/fixtures/test/ignored.el
>> @@ -1,2 +1,3 @@
>> +;; -*- no-byte-compile:t' -*-
>> (defun a ()
>> (+ a 1 +1 -1 1.0 #x0 ,a   :a t nil (0 . 0)))
>> diff --git a/packages/context-coloring/fixtures/test/let.el
>> b/packages/context-coloring/fixtures/test/let.el
>> index 49edb50..761a265 100644
>> --- a/packages/context-coloring/fixtures/test/let.el
>> +++ b/packages/context-coloring/fixtures/test/let.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (let (a
>> (b a)
>> (c free)
>> diff --git a/packages/context-coloring/fixtures/test/macroexp-let2.el
>> b/packages/context-coloring/fixtures/test/macroexp-let2.el
>> index 1b61df2..97fbb23 100644
>> --- a/packages/context-coloring/fixtures/test/macroexp-let2.el
>> +++ b/packages/context-coloring/fixtures/test/macroexp-let2.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (let (exp)
>> (macroexp-let2 macroexp-copyable-p v exp
>> v exp))
>> diff --git a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>> b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>> index caaf7e2..e028aef 100644
>> --- a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>> +++ b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>> @@ -1,2 +1,3 @@
>> +;; -*- no-byte-compile:t' -*-
>> (let ())
>> (let ()
>> diff --git a/packages/context-coloring/fixtures/test/varlist-spacing.el
>> b/packages/context-coloring/fixtures/test/varlist-spacing.el
>> index 97ec208..399cd18 100644
>> --- a/packages/context-coloring/fixtures/test/varlist-spacing.el
>> +++ b/packages/context-coloring/fixtures/test/varlist-spacing.el
>> @@ -1,3 +1,4 @@
>> +;; -*- no-byte-compile:t' -*-
>> (let (
>> (a (lambda ()))))
>> 
>> 

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Jackson Ray Hamilton]

Hi Stefan,

I was hoping we could revisit this issue, and use .elpaignore to exclude
files from byte compilation.

Apparently, tar uses gnulib's excluded_file_name function, which appears
to delegate to fnmatch, which looks pretty complex to me.  I'm not aware
of a "standalone" command providing an interface for this pattern
matcher, but if one exists, maybe we could utilize that.

Another option might be to generate an archive with tar and then use the
list of included files to determine what to compile.  This command:

$ tar -ch packages/context-coloring/ --exclude-vcs -X \
  packages/context-coloring/.elpaignore | tar --list

Produces this output:

packages/context-coloring/
packages/context-coloring/LICENSE
packages/context-coloring/context-coloring-emacs-lisp.el
packages/context-coloring/README.md
packages/context-coloring/context-coloring-javascript.el
packages/context-coloring/context-coloring.el

We could filter down the ".el" files from there.

Jackson

On 08/06/2016 11:29 AM, Stefan Monnier wrote:
>> The contents of these test fixtures are significant.  Changing them caused
>> the project's tests to fail.
> 
> Hmm... indeed I see that now.
> 
>> Can byte compilation be disabled for these
>> files without modifying their source? (For instance, by disabling
>> byte-compilation for the whole fixtures/ directory?  Maybe through
>> .dir-locals.el?  Or maybe by respecting .elpaignore?)
> 
> Respecting .elpaignore would be perfect, indeed.  But that file is using
> a format defined by tar, whereas I'd need to tweak the GNUmakefile rules
> to understand it, which is rather tricky.
> 
> I'll see what I can come up with.
> 
> 
>         Stefan
> 
> 
>> On Thu, Aug 4, 2016 at 6:37 PM, Stefan Monnier <monnier@iro.umontreal.ca>
>> wrote:
> 
>>> branch: master
>>> commit b7d8d3ca7f1bc279a3e8b21646ccea481c93ed46
>>> Author: Stefan Monnier <monnier@iro.umontreal.ca>
>>> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>>>
>>> * fixtures/test/: Don't byte-compile
>>> ---
>>> packages/context-coloring/fixtures/test/cond.el                   |    1
>>> +
>>> packages/context-coloring/fixtures/test/condition-case.el         |    1
>>> +
>>> packages/context-coloring/fixtures/test/defun.el                  |    1
>>> +
>>> packages/context-coloring/fixtures/test/ignored.el                |    1
>>> +
>>> packages/context-coloring/fixtures/test/let.el                    |    1
>>> +
>>> packages/context-coloring/fixtures/test/macroexp-let2.el          |    1
>>> +
>>> packages/context-coloring/fixtures/test/unbalanced-parenthesis.el |    1
>>> +
>>> packages/context-coloring/fixtures/test/varlist-spacing.el        |    1
>>> +
>>> 8 files changed, 8 insertions(+)
>>>
>>> diff --git a/packages/context-coloring/fixtures/test/cond.el
>>> b/packages/context-coloring/fixtures/test/cond.el
>>> index d5aae5b..5ed1ec2 100644
>>> --- a/packages/context-coloring/fixtures/test/cond.el
>>> +++ b/packages/context-coloring/fixtures/test/cond.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (let (a)
>>> (cond
>>> (a t)
>>> diff --git a/packages/context-coloring/fixtures/test/condition-case.el
>>> b/packages/context-coloring/fixtures/test/condition-case.el
>>> index 151f591..56a5f44 100644
>>> --- a/packages/context-coloring/fixtures/test/condition-case.el
>>> +++ b/packages/context-coloring/fixtures/test/condition-case.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (condition-case err
>>> (progn err free)
>>> (error err free)
>>> diff --git a/packages/context-coloring/fixtures/test/defun.el
>>> b/packages/context-coloring/fixtures/test/defun.el
>>> index 10a52f6..173ba0b 100644
>>> --- a/packages/context-coloring/fixtures/test/defun.el
>>> +++ b/packages/context-coloring/fixtures/test/defun.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (defun abc (def ghi &optional jkl)
>>> (+ def ghi jkl free))
>>>
>>> diff --git a/packages/context-coloring/fixtures/test/ignored.el
>>> b/packages/context-coloring/fixtures/test/ignored.el
>>> index 1f5fd42..0c540ff 100644
>>> --- a/packages/context-coloring/fixtures/test/ignored.el
>>> +++ b/packages/context-coloring/fixtures/test/ignored.el
>>> @@ -1,2 +1,3 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (defun a ()
>>> (+ a 1 +1 -1 1.0 #x0 ,a   :a t nil (0 . 0)))
>>> diff --git a/packages/context-coloring/fixtures/test/let.el
>>> b/packages/context-coloring/fixtures/test/let.el
>>> index 49edb50..761a265 100644
>>> --- a/packages/context-coloring/fixtures/test/let.el
>>> +++ b/packages/context-coloring/fixtures/test/let.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (let (a
>>> (b a)
>>> (c free)
>>> diff --git a/packages/context-coloring/fixtures/test/macroexp-let2.el
>>> b/packages/context-coloring/fixtures/test/macroexp-let2.el
>>> index 1b61df2..97fbb23 100644
>>> --- a/packages/context-coloring/fixtures/test/macroexp-let2.el
>>> +++ b/packages/context-coloring/fixtures/test/macroexp-let2.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (let (exp)
>>> (macroexp-let2 macroexp-copyable-p v exp
>>> v exp))
>>> diff --git a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>>> b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>>> index caaf7e2..e028aef 100644
>>> --- a/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>>> +++ b/packages/context-coloring/fixtures/test/unbalanced-parenthesis.el
>>> @@ -1,2 +1,3 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (let ())
>>> (let ()
>>> diff --git a/packages/context-coloring/fixtures/test/varlist-spacing.el
>>> b/packages/context-coloring/fixtures/test/varlist-spacing.el
>>> index 97ec208..399cd18 100644
>>> --- a/packages/context-coloring/fixtures/test/varlist-spacing.el
>>> +++ b/packages/context-coloring/fixtures/test/varlist-spacing.el
>>> @@ -1,3 +1,4 @@
>>> +;; -*- no-byte-compile:t' -*-
>>> (let (
>>> (a (lambda ()))))
>>>
>>>

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Stefan Monnier]

> Apparently, tar uses gnulib's excluded_file_name function, which appears
> to delegate to fnmatch, which looks pretty complex to me.  I'm not aware
> of a "standalone" command providing an interface for this pattern
> matcher, but if one exists, maybe we could utilize that.

I think we could use sh's "case" patterns for that.

> Another option might be to generate an archive with tar and then use the
> list of included files to determine what to compile.  This command:
>
> $ tar -ch packages/context-coloring/ --exclude-vcs -X \
>   packages/context-coloring/.elpaignore | tar --list
>
> Produces this output:

> packages/context-coloring/
> packages/context-coloring/LICENSE
> packages/context-coloring/context-coloring-emacs-lisp.el
> packages/context-coloring/README.md
> packages/context-coloring/context-coloring-javascript.el
> packages/context-coloring/context-coloring.el
>
> We could filter down the ".el" files from there.

That might work as well, indeed.

BTW, given the current GNUmakefile, it's easy/natural to only test
.elpaignore matches for those files which end in .el and which don't yet
have a .elc file.  I expect this would be an important optimization to
make sure that "make" is fast when there's not much to (re)compile.


        Stefan

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Jackson Ray Hamilton]

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

Would you be amenable to me pushing the attached patch?

Jackson

On 12/17/2016 07:18 PM, Stefan Monnier wrote:
>> Apparently, tar uses gnulib's excluded_file_name function, which appears
>> to delegate to fnmatch, which looks pretty complex to me.  I'm not aware
>> of a "standalone" command providing an interface for this pattern
>> matcher, but if one exists, maybe we could utilize that.
> 
> I think we could use sh's "case" patterns for that.
> 
>> Another option might be to generate an archive with tar and then use the
>> list of included files to determine what to compile.  This command:
>>
>> $ tar -ch packages/context-coloring/ --exclude-vcs -X \
>>   packages/context-coloring/.elpaignore | tar --list
>>
>> Produces this output:
> 
>> packages/context-coloring/
>> packages/context-coloring/LICENSE
>> packages/context-coloring/context-coloring-emacs-lisp.el
>> packages/context-coloring/README.md
>> packages/context-coloring/context-coloring-javascript.el
>> packages/context-coloring/context-coloring.el
>>
>> We could filter down the ".el" files from there.
> 
> That might work as well, indeed.
> 
> BTW, given the current GNUmakefile, it's easy/natural to only test
> .elpaignore matches for those files which end in .el and which don't yet
> have a .elc file.  I expect this would be an important optimization to
> make sure that "make" is fast when there's not much to (re)compile.
> 
> 
>         Stefan
> 

[-- Attachment #2: 0001-Respect-.elpaignore-when-determining-files-to-byte-c.patch --]
[-- Type: text/x-patch, Size: 1634 bytes --]

From 7670085e197cbf0b966232889acb760e5555f25c Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Sun, 18 Dec 2016 11:53:54 -0800
Subject: [PATCH 1/1] Respect .elpaignore when determining files to
 byte-compile.

---
 GNUmakefile | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/GNUmakefile b/GNUmakefile
index 7d6d100..ab81163 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -144,9 +144,23 @@ $(foreach al, $(autoloads), $(eval $(call RULE-srcdeps, $(al))))
 	                                          \"$$(pwd)\")"
 
 # Put into elcs the set of elc files we need to keep up-to-date.
-# I.e. one for each .el file except for the -pkg.el, the -autoloads.el, and
-# the .el files that are marked "no-byte-compile".
-els := $(call FILTER-nonsrc, $(wildcard packages/*/*.el))
+# I.e. one for each .el file in each package root, except for the -pkg.el,
+# the -autoloads.el, the .el files that are marked "no-byte-compile", and
+# files matching patterns in packages' .elpaignore files.
+included_els := $(shell \
+  for pt in packages/*; do				\
+      if [ -f "$${pt}/.elpaignore" ]; then		\
+          ignore="$${pt}/.elpaignore";			\
+      else						\
+          ignore="/dev/null";				\
+      fi;						\
+      if [ -d $$pt ]; then				\
+          tar -ch $$pt --exclude-vcs -X "$$ignore"	\
+            | tar --list				\
+            | grep '^[^/]*/[^/]*/[^/]*\.el$$';		\
+      fi;						\
+  done)
+els := $(call FILTER-nonsrc, $(included_els))
 naive_elcs := $(patsubst %.el, %.elc, $(els))
 current_elcs := $(wildcard packages/*/*.elc)
 
-- 
2.1.4

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Stefan Monnier]

> Would you be amenable to me pushing the attached patch?

Hmm...

It works well, but it slows down "make" in the case where there's
nothing to do by more than a factor 2 (on the slowest machine on which
I maintain a fully-built `elpa` checkout, it brings the time for "make"
from 7s to 18s).  The current time is already on the high side for my
taste, so I think we should try a bit harder to keep it in check.


        Stefan

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Jackson Ray Hamilton]

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

Sorry, I didn't even notice the slowdown; I see now that it went from
0.9s to 1.9s on my machine.

The first attached patch is simple and speeds it up to 1.65s.

The second attached patch sort of re-implements what tar would do, using
sh case, but it's more complex.  It clocks in at 1.35s after that.

Jackson

On 12/19/2016 06:49 AM, Stefan Monnier wrote:
>> Would you be amenable to me pushing the attached patch?
> 
> Hmm...
> 
> It works well, but it slows down "make" in the case where there's
> nothing to do by more than a factor 2 (on the slowest machine on which
> I maintain a fully-built `elpa` checkout, it brings the time for "make"
> from 7s to 18s).  The current time is already on the high side for my
> taste, so I think we should try a bit harder to keep it in check.
> 
> 
>         Stefan
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Slightly-optimize-excluding.patch --]
[-- Type: text/x-patch; name="0001-Slightly-optimize-excluding.patch", Size: 911 bytes --]

From adc8237cd08a14a62ac3a14a3f45ebc8530bd091 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Mon, 19 Dec 2016 17:26:42 -0800
Subject: [PATCH 1/2] Slightly optimize excluding.

---
 GNUmakefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/GNUmakefile b/GNUmakefile
index ab81163..9690af4 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -155,9 +155,9 @@ included_els := $(shell \
           ignore="/dev/null";				\
       fi;						\
       if [ -d $$pt ]; then				\
-          tar -ch $$pt --exclude-vcs -X "$$ignore"	\
-            | tar --list				\
-            | grep '^[^/]*/[^/]*/[^/]*\.el$$';		\
+          tar -ch $$pt/*.el --no-recursion		\
+              --exclude-vcs -X "$$ignore"		\
+            | tar --list;				\
       fi;						\
   done)
 els := $(call FILTER-nonsrc, $(included_els))
-- 
2.1.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Optimize-exclusion-further-with-case.patch --]
[-- Type: text/x-patch; name="0002-Optimize-exclusion-further-with-case.patch", Size: 2000 bytes --]

From b62ee2e2fbaccbbba7501115819e7d10eefe38d5 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Mon, 19 Dec 2016 21:44:18 -0800
Subject: [PATCH 2/2] Optimize exclusion further with case.

TBH, I'm not nearly as confident in this code as the code it replaces, but it is
slightly faster.
---
 GNUmakefile | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/GNUmakefile b/GNUmakefile
index 9690af4..442dd9c 100644
--- a/GNUmakefile
+++ b/GNUmakefile
@@ -148,17 +148,22 @@ $(foreach al, $(autoloads), $(eval $(call RULE-srcdeps, $(al))))
 # the -autoloads.el, the .el files that are marked "no-byte-compile", and
 # files matching patterns in packages' .elpaignore files.
 included_els := $(shell \
-  for pt in packages/*; do				\
-      if [ -f "$${pt}/.elpaignore" ]; then		\
-          ignore="$${pt}/.elpaignore";			\
-      else						\
-          ignore="/dev/null";				\
-      fi;						\
-      if [ -d $$pt ]; then				\
-          tar -ch $$pt/*.el --no-recursion		\
-              --exclude-vcs -X "$$ignore"		\
-            | tar --list;				\
-      fi;						\
+  for pt in packages/*; do						\
+      if [ -d $$pt ]; then						\
+          if [ -f "$${pt}/.elpaignore" ]; then				\
+              cd $$pt;							\
+              ls -1 *.el | while IFS= read -r filename; do		\
+                  hasmatch=0;						\
+                  while IFS= read -r pattern; do			\
+                      case $$filename in ($$pattern) hasmatch=1; break ;; esac;\
+                  done < .elpaignore;					\
+                  test $$hasmatch = 1 || echo "$${pt}/$${filename}";	\
+              done;							\
+              cd ../..;							\
+          else								\
+              ls -1 $$pt/*.el;						\
+          fi;								\
+      fi;								\
   done)
 els := $(call FILTER-nonsrc, $(included_els))
 naive_elcs := $(patsubst %.el, %.elc, $(els))
-- 
2.1.4

Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile

[Stefan Monnier]

> Sorry, I didn't even notice the slowdown; I see now that it went from
> 0.9s to 1.9s on my machine.
>
> The first attached patch is simple and speeds it up to 1.65s.
>
> The second attached patch sort of re-implements what tar would do, using
> sh case, but it's more complex.  It clocks in at 1.35s after that.

[ If we can use `tar` it seems better, indeed.  BTW we need to be careful
  to try and make it difficult for a committer to elpa.git to get access
  to elpa.gnu.org via this script (I'm thinking of a commit that adds
  files with names that contain funny chars like space, ;, etc...).  ]

But they both restrict the .el files to be in the top-level directory,
and the source of the problem that lead to this discussion is that
I want to byte-compile also those files that are inside sub-directories
(even though we don't currently do that).

I think we could optimize the code in other ways:
- currently `make` is invoked twice (once recursively to build `elcs`),
  so this computation of included_els is performed twice.  I'd be great to get
  rid of this redundancy.
- only use `tar` if there is a .elpaignore, and just use
  something like $(wildcard *.el */*.el */*/*.el) in the other case.
- only check .elpaignore for *changes*.  I.e. cache the previous computation
  somehow.  One way is to only check .elpaignore for those files which don't
  have a corresponding .elc.


        Stefan