* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile [not found] ` <20160805013758.9B5BB2201A4@vcs.savannah.gnu.org> @ 2016-08-06 17:42 ` Jackson Hamilton 2016-08-06 18:29 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: Jackson Hamilton @ 2016-08-06 17:42 UTC (permalink / raw) To: emacs-devel, Stefan Monnier [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-08-06 17:42 ` [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile Jackson Hamilton @ 2016-08-06 18:29 ` Stefan Monnier 2016-12-18 0:06 ` Jackson Ray Hamilton 0 siblings, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2016-08-06 18:29 UTC (permalink / raw) To: Jackson Hamilton; +Cc: emacs-devel > 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 ())))) >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-08-06 18:29 ` Stefan Monnier @ 2016-12-18 0:06 ` Jackson Ray Hamilton 2016-12-18 3:18 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: Jackson Ray Hamilton @ 2016-12-18 0:06 UTC (permalink / raw) To: Stefan Monnier, Jackson Hamilton; +Cc: emacs-devel 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 ())))) >>> >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-12-18 0:06 ` Jackson Ray Hamilton @ 2016-12-18 3:18 ` Stefan Monnier 2016-12-19 2:43 ` Jackson Ray Hamilton 0 siblings, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2016-12-18 3:18 UTC (permalink / raw) To: Jackson Ray Hamilton; +Cc: emacs-devel, Jackson Hamilton > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-12-18 3:18 ` Stefan Monnier @ 2016-12-19 2:43 ` Jackson Ray Hamilton 2016-12-19 14:49 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: Jackson Ray Hamilton @ 2016-12-19 2:43 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-12-19 2:43 ` Jackson Ray Hamilton @ 2016-12-19 14:49 ` Stefan Monnier 2016-12-20 6:04 ` Jackson Ray Hamilton 0 siblings, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2016-12-19 14:49 UTC (permalink / raw) To: Jackson Ray Hamilton; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-12-19 14:49 ` Stefan Monnier @ 2016-12-20 6:04 ` Jackson Ray Hamilton 2016-12-20 14:53 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: Jackson Ray Hamilton @ 2016-12-20 6:04 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel [-- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile 2016-12-20 6:04 ` Jackson Ray Hamilton @ 2016-12-20 14:53 ` Stefan Monnier 0 siblings, 0 replies; 8+ messages in thread From: Stefan Monnier @ 2016-12-20 14:53 UTC (permalink / raw) To: Jackson Ray Hamilton; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-20 14:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20160805013757.31623.20092@vcs.savannah.gnu.org>
[not found] ` <20160805013758.9B5BB2201A4@vcs.savannah.gnu.org>
2016-08-06 17:42 ` [elpa] master b7d8d3c 9/9: * fixtures/test/: Don't byte-compile Jackson Hamilton
2016-08-06 18:29 ` Stefan Monnier
2016-12-18 0:06 ` Jackson Ray Hamilton
2016-12-18 3:18 ` Stefan Monnier
2016-12-19 2:43 ` Jackson Ray Hamilton
2016-12-19 14:49 ` Stefan Monnier
2016-12-20 6:04 ` Jackson Ray Hamilton
2016-12-20 14:53 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).