GNU bug report logs - #11806
(setq load-path ..) of elisp-comp

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11806 in the body.
You can then email your comments to 11806 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Makoto Fujiwara <makoto <at> ki.nu>
To: bug-automake <at> gnu.org
Subject: (setq load-path ..) of elisp-comp
Date: Thu, 28 Jun 2012 13:47:17 +0900

We have following line in automake-1.12.1/lib/elisp-comp.

    73  mkdir $tempdir
    74  cp "$@" $tempdir
    75  
    76  (
    77    cd $tempdir
    78    echo "(setq load-path (cons nil load-path))" > script
    79    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
    80    mv *.elc ..
    81  ) || exit $?
    82  
    83  (exit 0); exit 0

 It seems to me the intention of line 78 is to set load-path
to add default directory on top of existing load-path.

 This 'script' is OK if the file to compile is only one in the
directory.

  But if some files are in the directory there and we will
compile file by file on the same directory, there may be a
possibility that some files load another file in the same
directory. In that case this 'script' fails to read such ones.

I do have problem compiling *.el files with tc-2.3.1 (svn version) 

Following patch fixes this type of problem, thanks a lot.

By the way, the same patch was once proposed as
  http://osdir.com/ml/sysutils.automake.patches/2003-01/msg00004.html
and fix seems to have made, but real line was dropped with
unknown reason.

--- lib/elisp-comp.orig	2012-06-01 22:47:10.000000000 +0900
+++ lib/elisp-comp	2012-06-28 13:28:44.000000000 +0900
@@ -75,7 +75,7 @@
 
 (
   cd $tempdir
-  echo "(setq load-path (cons nil load-path))" > script
+  echo "(setq load-path (cons \"../\" (cons nil load-path)))" > script
   $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
   mv *.elc ..
 ) || exit $?


Thanks again, 
---
Makoto Fujiwara, 
Chiba, Japan, Narita Airport and Disneyland prefecture.

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

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Makoto Fujiwara <makoto <at> ki.nu>
Cc: 11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Thu, 28 Jun 2012 12:49:45 +0200

Hi Makoto.  Thanks for the report and the patch.

On 06/28/2012 06:47 AM, Makoto Fujiwara wrote:
> We have following line in automake-1.12.1/lib/elisp-comp.
> 
>     73  mkdir $tempdir
>     74  cp "$@" $tempdir
>     75  
>     76  (
>     77    cd $tempdir
>     78    echo "(setq load-path (cons nil load-path))" > script
>     79    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
>     80    mv *.elc ..
>     81  ) || exit $?
>     82  
>     83  (exit 0); exit 0
> 
>  It seems to me the intention of line 78 is to set load-path
> to add default directory on top of existing load-path.
> 
>  This 'script' is OK if the file to compile is only one in the
> directory.
> 
>   But if some files are in the directory there and we will
> compile file by file on the same directory, there may be a
> possibility that some files load another file in the same
> directory. In that case this 'script' fails to read such ones.
> 
> I do have problem compiling *.el files with tc-2.3.1 (svn version) 
>
Could you expose such problems in a minimal test case?  This way, I
could add it to the automake testsuite, to ensure the issue doesn't
represent itself in the future.  Thanks.

> Following patch fixes this type of problem, thanks a lot.
> 
> By the way, the same patch was once proposed as
>   http://osdir.com/ml/sysutils.automake.patches/2003-01/msg00004.html
> and fix seems to have made, but real line was dropped with
> unknown reason.
>
> --- lib/elisp-comp.orig	2012-06-01 22:47:10.000000000 +0900
> +++ lib/elisp-comp	2012-06-28 13:28:44.000000000 +0900
> @@ -75,7 +75,7 @@
>  
>  (
>    cd $tempdir
> -  echo "(setq load-path (cons nil load-path))" > script
> +  echo "(setq load-path (cons \"../\" (cons nil load-path)))" > script
>    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
>    mv *.elc ..
>  ) || exit $?
> 
> 
The patch seems small and harmless, but unfortunately I know nothing at
all about Lisp or Emacs, so I can't judge whether such a change could
have unintended consequences.  Perchance someone more knowledgeable than
me in this area is reading the thread, and can chime in?  Otherwise I
will go ahead and push the patch in 72 hours.

Thanks,
  Stefano

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

From: Jack Kelly <jack <at> jackkelly.name>
To: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Cc: Makoto Fujiwara <makoto <at> ki.nu>, 11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Fri, 29 Jun 2012 08:32:18 +1000

On Thu, Jun 28, 2012 at 8:49 PM, Stefano Lattarini
<stefano.lattarini <at> gmail.com> wrote:
> On 06/28/2012 06:47 AM, Makoto Fujiwara wrote:
>> I do have problem compiling *.el files with tc-2.3.1 (svn version)

I think this is the relevant Makefile.am:
http://code.google.com/p/tcode/source/browse/trunk/tc/lisp/Makefile.am

It confuses me thoroughly.

> Could you expose such problems in a minimal test case?  This way, I
> could add it to the automake testsuite, to ensure the issue doesn't
> represent itself in the future.  Thanks.
>
>> Following patch fixes this type of problem, thanks a lot.
>>
>> By the way, the same patch was once proposed as
>>   http://osdir.com/ml/sysutils.automake.patches/2003-01/msg00004.html
>> and fix seems to have made, but real line was dropped with
>> unknown reason.
>>
>> --- lib/elisp-comp.orig       2012-06-01 22:47:10.000000000 +0900
>> +++ lib/elisp-comp    2012-06-28 13:28:44.000000000 +0900
>> @@ -75,7 +75,7 @@
>>
>>  (
>>    cd $tempdir
>> -  echo "(setq load-path (cons nil load-path))" > script
>> +  echo "(setq load-path (cons \"../\" (cons nil load-path)))" > script
>>    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
>>    mv *.elc ..
>>  ) || exit $?
>>
>>
> The patch seems small and harmless, but unfortunately I know nothing at
> all about Lisp or Emacs, so I can't judge whether such a change could
> have unintended consequences.  Perchance someone more knowledgeable than
> me in this area is reading the thread, and can chime in?  Otherwise I
> will go ahead and push the patch in 72 hours.

I'm no elisp master, but here's what I see is happening:

* Developer defines something like lisp_LISP = foo.el bar.el baz.el
* baz.el (say) depends on quux.el, in the same directory but not
listed in the primary.
* At `make' time, everything in lisp_LISP is copied to a subdirectory
and batch-byte-compile is called on all those files.
* Because quux.el wasn't copied over, the compile fails.
* Adding the parent directory to load-path might fix that, but I
wonder if it will still break on VPATH builds? Maybe the correct
answer is to add ../$(srcdir) or $(abs_srcdir) insted.
* Why (in the .el files) is the developer depending on a file that's
not being compiled? will this cause automake to silently do the wrong
thing if a new .el file has been listed and Makefile.am doesn't get
updated? Now you can go ahead and install an incomplete package.
* Should the current directory come before the source directory?

There's another (potential) problem. The script that elisp-comp
creates is called "script". Does it make sense to allow files without
a .el extension to appear in a _LISP primary? Currently, automake lets
you get away with it, but it will only compile *.el anyway (even
though it copies $@ into the temporary dir). I don't know enough about
elisp to say, but if that's fixed, then "script" should be given a
better filename (using mktemp or something) so it doesn't clobber a
user's script.

-- Jack

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

From: Makoto Fujiwara <makoto <at> ki.nu>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: Stefano Lattarini <stefano.lattarini <at> gmail.com>, 11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Sat, 30 Jun 2012 10:19:32 +0900

| To: Stefano Lattarini <stefano.lattarini <at> gmail.com>
| From: Jack Kelly <jack <at> jackkelly.name>
| Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
| Date: Fri, 29 Jun 2012 08:32:18 +1000
| Message-ID: <CAEnY7O9_MatVzqqvZ4ATjKRMFW5_w93UTzyZ8Mg3Pj+74qcjpA <at> mail.gmail.com>

> I'm no elisp master, but here's what I see is happening:

> * Developer defines something like lisp_LISP = foo.el bar.el baz.el
> * baz.el (say) depends on quux.el, in the same directory but not
> listed in the primary.

> * At `make' time, everything in lisp_LISP is copied to a subdirectory
> and batch-byte-compile is called on all those files.

> * Because quux.el wasn't copied over, the compile fails.

Thanks for reviewing Makefile.am setup. 
  The reason 'quux.el' (in above example) is not listed, is
that it should not be byte-compiled by some reason. It is
intentionally excluded from the to-be-compiled list. I didn't
get that point. Thanks a lot.  I will either adjust Makefile.am
or pursue not using automake, or insist to depend automake.

With your advice for VPATH issue, I am now using attached patch,
thanks a lot,
---
Makoto Fujiwara, 
Chiba, Japan, Narita Airport and Disneyland prefecture.

--- lib/elisp-comp.orig	2012-01-31 20:41:18.000000000 +0900
+++ lib/elisp-comp	2012-04-13 22:14:39.000000000 +0900
@@ -62,6 +62,7 @@
 fi
 
 tempdir=elc.$$
+currdir=`pwd`
 
 # Cleanup the temporary directory on exit.
 trap 'ret=$?; rm -rf "$tempdir" && exit $ret' 0
@@ -72,7 +73,7 @@
 
 (
   cd $tempdir
-  echo "(setq load-path (cons nil load-path))" > script
+  echo "(setq load-path (cons \"$currdir\" (cons nil load-path)))" > script
   $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
   mv *.elc ..
 ) || exit $?

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

From: Jack Kelly <jack <at> jackkelly.name>
To: Makoto Fujiwara <makoto <at> ki.nu>
Cc: Stefano Lattarini <stefano.lattarini <at> gmail.com>, 11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Sun, 1 Jul 2012 09:03:32 +1000

On Sat, Jun 30, 2012 at 11:19 AM, Makoto Fujiwara <makoto <at> ki.nu> wrote:
> With your advice for VPATH issue, I am now using attached patch,
> thanks a lot,
> --- lib/elisp-comp.orig 2012-01-31 20:41:18.000000000 +0900
> +++ lib/elisp-comp      2012-04-13 22:14:39.000000000 +0900
> @@ -62,6 +62,7 @@
>  fi
>
>  tempdir=elc.$$
> +currdir=`pwd`
>
>  # Cleanup the temporary directory on exit.
>  trap 'ret=$?; rm -rf "$tempdir" && exit $ret' 0
> @@ -72,7 +73,7 @@
>
>  (
>    cd $tempdir
> -  echo "(setq load-path (cons nil load-path))" > script
> +  echo "(setq load-path (cons \"$currdir\" (cons nil load-path)))" > script
>    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
>    mv *.elc ..
>  ) || exit $?

Hi,

Unfortunately your patch doesn't quite work, because in a VPATH build
the build dir is not the sourcedir, so when you set $currdir, you are
setting the wrong path.

I'm not sure of the best way to fix this. The rules for calling
elisp-comp could be updated so that $(abs_srcdir) is passed along:

        abs_srcdir="$(abs_srcdir)" EMACS="$(EMACS)" $(SHELL)
$(elisp_comp) "$$@" || exit 1; \

, which is then added to load-path. elisp-comp would still need a
patch, but that's ok.

As to providing test cases, something like this should work:

configure.ac:

AC_INIT([foo], [bar], [baz <at> quux.net])
AM_INIT_AUTOMAKE([foreign])
AM_PATH_LISPDIR
AC_CONFIG_FILES([Makefile])
AC_OUTPUT

Makefile.am:

lisp_LISP = foo.el
lisp_DATA = bar.el

foo.el:

(require 'bar)

bar.el:

(provide 'bar)

Once elisp-comp (and whatever else) is fixed, this setup should
survive a ./configure && make in both a normal and VPATH build.

(Aside: my copyright paperwork should still be in order. Email me
off-list if it is not.)

-- Jack

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

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: "automake-patches <at> gnu.org" <automake-patches <at> gnu.org>,
	Makoto Fujiwara <makoto <at> ki.nu>, 11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Thu, 05 Jul 2012 19:56:02 +0200

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

tags 11806  + patch
thanks

On 07/01/2012 01:03 AM, Jack Kelly wrote:
> On Sat, Jun 30, 2012 at 11:19 AM, Makoto Fujiwara <makoto <at> ki.nu> wrote:
>> With your advice for VPATH issue, I am now using attached patch,
>> thanks a lot,
>> --- lib/elisp-comp.orig 2012-01-31 20:41:18.000000000 +0900
>> +++ lib/elisp-comp      2012-04-13 22:14:39.000000000 +0900
>> @@ -62,6 +62,7 @@
>>  fi
>>
>>  tempdir=elc.$$
>> +currdir=`pwd`
>>
>>  # Cleanup the temporary directory on exit.
>>  trap 'ret=$?; rm -rf "$tempdir" && exit $ret' 0
>> @@ -72,7 +73,7 @@
>>
>>  (
>>    cd $tempdir
>> -  echo "(setq load-path (cons nil load-path))" > script
>> +  echo "(setq load-path (cons \"$currdir\" (cons nil load-path)))" > script
>>    $EMACS -batch -q -l script -f batch-byte-compile *.el || exit $?
>>    mv *.elc ..
>>  ) || exit $?
> 
> Hi,
> 
> Unfortunately your patch doesn't quite work, because in a VPATH build
> the build dir is not the sourcedir, so when you set $currdir, you are
> setting the wrong path.
> 
> I'm not sure of the best way to fix this. The rules for calling
> elisp-comp could be updated so that $(abs_srcdir) is passed along:
> 
>         abs_srcdir="$(abs_srcdir)" EMACS="$(EMACS)" $(SHELL)
> $(elisp_comp) "$$@" || exit 1; \
> 
> , which is then added to load-path. elisp-comp would still need a
> patch, but that's ok.
> 
> As to providing test cases, something like this should work:
> 
> configure.ac:
> 
> AC_INIT([foo], [bar], [baz <at> quux.net])
> AM_INIT_AUTOMAKE([foreign])
> AM_PATH_LISPDIR
> AC_CONFIG_FILES([Makefile])
> AC_OUTPUT
> 
> Makefile.am:
> 
> lisp_LISP = foo.el
> lisp_DATA = bar.el
> 
> foo.el:
> 
> (require 'bar)
> 
> bar.el:
> 
> (provide 'bar)
> 
> Once elisp-comp (and whatever else) is fixed, this setup should
> survive a ./configure && make in both a normal and VPATH build.
>
Thanks.  I've condensed a patch from you explanation (see attachment).
I will push it shortly if there are no objections.

> (Aside: my copyright paperwork should still be in order. Email me
> off-list if it is not.)
>
I don't have an account on fencepost, so I can't check this, sorry.
Since your part of the changes are short enough to be classified as
"tiny change", I've just added a 'Copyright-paperwork-exempt' field
to the patch to get the easiest way out.

> -- Jack

Thanks,
  Stefano

[0001-lisp-better-support-of-VPATH-builds.patch (text/x-diff, attachment)]

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

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: Makoto Fujiwara <makoto <at> ki.nu>,
	"automake-patches <at> gnu.org" <automake-patches <at> gnu.org>,
	11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Thu, 05 Jul 2012 23:57:11 +0200

Hi Jack, thanks for the feedback.

On 07/05/2012 11:33 PM, Jack Kelly wrote:
> On Fri, Jul 6, 2012 at 3:56 AM, Stefano Lattarini
> <stefano.lattarini <at> gmail.com> wrote:
>> On 07/01/2012 01:03 AM, Jack Kelly wrote:
>> Thanks.  I've condensed a patch from you explanation (see attachment).
>> I will push it shortly if there are no objections.
> 
> Though I doubt it makes any practical difference, the only thing I
> would suggest is to cons the load-path so the current directory is
> before $abs_srcdir: echo "(setq load-path (cons nil (cons
> \"$abs_srcdir\" load-path)))" > script
> 
> That way, the build directory (where all the .el files being compiled
> actually are), then the src directory, then everywhere else will be
> searched.
>
Usually I'd agree, and in fact I had done as you're suggesting in a
previous attempt; but that caused the test 'lisp3.sh' to fail :-/
With the patch I've posted, the testsuite remains clean at least.

So I say we commit my patch: albeit suboptimal, and somewhat reeking
of cargo-cult programming, it solves the OP issue.

If someone then wants to venture in refactoring, fixing and cleaning
up the elisp support in Automake -- be him my guest!  As for more,
I'm not knowledgeable nor motivated enough to venture in something
like that at the moment.  Sorry.

> Another thing I noticed from `man emacs': There's a --eval command
> like argument, so generating that subscript is technically
> unnecessary. Try something like:
> 
> $EMACS -batch -q --eval "(setq load-path (cons nil (cons
> \"$abs_srcdir\" load-path)))" -f batch-byte-compile *.el || exit $?
> 
> Since elisp-comp doesn't handle stamps but only a temporary directory,
> I think you might even be able to call it directly from make using
> something like:
> 
> $(EMACS) -batch -q --eval "(setq load-path (cons \"$(srcdir)\"
> load-path))" -f batch-byte-compile $(LISP) || exit 1
>
I'm not sure how this would interact with $(LISP) containing '.el' files
in a subdirectory ...  But then, I guess all the current implementation
of byte-compilation for elisp sources is probably brittle and buggy in
such a setup anyway, I guess, so my misgivings might be unjustified:

  <http://lists.gnu.org/archive/html/automake/2009-10/msg00013.html>

Anyway, I'd rather not touch the existing code, for fear of introducing
regressions.  But patches would be mostly welcome (if coming with proper
testsuite enhancement).

Thanks,
  Stefano

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

From: Jack Kelly <jack <at> jackkelly.name>
To: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Cc: Makoto Fujiwara <makoto <at> ki.nu>,
	"automake-patches <at> gnu.org" <automake-patches <at> gnu.org>,
	11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Fri, 6 Jul 2012 10:42:05 +1000

On Fri, Jul 6, 2012 at 7:57 AM, Stefano Lattarini
<stefano.lattarini <at> gmail.com> wrote:
> Usually I'd agree, and in fact I had done as you're suggesting in a
> previous attempt; but that caused the test 'lisp3.sh' to fail :-/
> With the patch I've posted, the testsuite remains clean at least.
>
> So I say we commit my patch: albeit suboptimal, and somewhat reeking
> of cargo-cult programming, it solves the OP issue.

I would like to have a crack at rewriting some of the elisp stuff, but
I can make no promises as to when that will actually happen. Therefore
I agree with you: commit the patch and we'll deal with the refactoring
if/when it arrives.

-- Jack

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

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: Makoto Fujiwara <makoto <at> ki.nu>,
	"automake-patches <at> gnu.org" <automake-patches <at> gnu.org>,
	11806 <at> debbugs.gnu.org
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Fri, 06 Jul 2012 10:34:38 +0200

On 07/06/2012 02:42 AM, Jack Kelly wrote:
> On Fri, Jul 6, 2012 at 7:57 AM, Stefano Lattarini
> <stefano.lattarini <at> gmail.com> wrote:
>> Usually I'd agree, and in fact I had done as you're suggesting in a
>> previous attempt; but that caused the test 'lisp3.sh' to fail :-/
>> With the patch I've posted, the testsuite remains clean at least.
>>
>> So I say we commit my patch: albeit suboptimal, and somewhat reeking
>> of cargo-cult programming, it solves the OP issue.
> 
> I would like to have a crack at rewriting some of the elisp stuff, but
> I can make no promises as to when that will actually happen.
>
No need to make any such promise.  Just know that if you find time for
such a rewrite, I'd be happy to help out and try my best to have it
integrated.

> Therefore I agree with you: commit the patch and we'll deal with the
> refactoring if/when it arrives.
>
Thanks.  I'll push the patch by this evening or afternooon if nobody
else speaks out.

Best regards,
  Stefano

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

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jack Kelly <jack <at> jackkelly.name>
Cc: 11806-done <at> debbugs.gnu.org, Makoto Fujiwara <makoto <at> ki.nu>,
	"automake-patches <at> gnu.org" <automake-patches <at> gnu.org>
Subject: Re: bug#11806: (setq load-path ..) of elisp-comp
Date: Fri, 06 Jul 2012 21:39:15 +0200

On 07/06/2012 10:34 AM, Stefano Lattarini wrote:
>
> Thanks.  I'll push the patch by this evening or afternoon if nobody
> else speaks out.
> 
Pushed now.  I'm thus closing this bug report.

Regards,
  Stefano

Previous Next

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