[joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Clint Adams]

I can reproduce this, but only if use-perl is off.

----- Forwarded message from Joshua Kwan <joshk@triplehelix.org> -----

I have this in a debian/rules file:

ifneq (,$(findstring noopt,$(DEB_BUILD_OPTIONS)))
CFLAGS := -g -O0 -Wall
else
CFLAGS := -g -O2 -Wall
endif

Completion shows:

% make -f debian/rules
CFLAGS             build-arch         build-stamp-indep
binary             build-indep        clean
build              build-stamp-arch   install

CFLAGS shouldn't get flagged as a target.

The complete debian/rules is attached.

-- System Information:
Debian Release: 3.1
  APT prefers unstable
  APT policy: (500, 'unstable'), (499, 'testing'), (1, 'experimental')
Architecture: i386 (i686)
Kernel: Linux 2.6.9-rc1
Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (ignored: LC_ALL set to en_US.UTF-8)

Versions of packages zsh depends on:
ii  debconf                     1.4.34       Debian configuration management sy
ii  libc6                       2.3.2.ds1-16 GNU C Library: Shared libraries an
ii  libcap1                     1:1.10-14    support for getting/setting POSIX.
ii  libncurses5                 5.4-4        Shared libraries for terminal hand
ii  passwd                      1:4.0.3-30.1 Change and administer password and

-- debconf information excluded

#!/usr/bin/make -f
# debian/rules for devscripts, based on the example file rules.indep.

# Uncomment this to turn on verbose mode.
#export DH_VERBOSE=1

ifneq (,$(findstring noopt,$(DEB_BUILD_OPTIONS)))
CFLAGS := -g -O0 -Wall
else
CFLAGS := -g -O2 -Wall
endif

# Here we perform some checks to ensure that we haven't goofed or left
# debugging code around or ... before we build the package
test: test-stamp
test-stamp:
	# debugging info in bts?
	if grep -q '\$$debug *= *1' bts.pl; then exit 1; else exit 0; fi
	touch test-stamp

build-indep: build-stamp-indep
build-stamp-indep: test-stamp
	dh_testdir
	touch build-stamp-indep

build-arch: build-stamp-arch
build-stamp-arch: test-stamp
	dh_testdir
	$(MAKE) CFLAGS='$(CFLAGS)'
	touch build-stamp-arch

build: build-arch build-indep

clean:
	dh_testdir
	dh_testroot
	-$(MAKE) clean
	rm -f build-stamp* test-stamp
	dh_clean

install: build
	dh_testdir
	dh_testroot
	dh_clean -k
	dh_installdirs
	$(MAKE) DESTDIR=$(CURDIR)/debian/devscripts install

# Build architecture-independent files here.
binary-indep: build-indep
# We have nothing to do

# Build architecture-dependent files here.
binary-arch: build-arch install
	dh_testdir
	dh_testroot
	dh_installdocs README
	dh_installexamples
#	This is now done by the make install command above
#	dh_installman
	dh_link /usr/share/devscripts/conf.default \
		/usr/share/doc/devscripts/devscripts.conf.ex
	dh_installchangelogs
	dh_strip
	dh_compress
	dh_fixperms
	dh_installdeb
	dh_perl
	dh_shlibdeps
	dh_gencontrol
	dh_md5sums
	dh_builddeb

binary: binary-indep binary-arch

.PHONY: build-arch build-indep build clean binary-indep binary-arch binary \
	install

# Local variables:
# mode: makefile
# End:


----- End forwarded message -----

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Clint Adams]

> I can reproduce this, but only if use-perl is off.

And only if awk is gawk; it works fine with mawk.

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Clint Adams]

> And only if awk is gawk; it works fine with mawk.

And only under certain locales.

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Clint Adams]

> And only under certain locales.

This avoids what might be a gawk bug by not using awk.  Should we move
toward 20277 instead?

Index: Completion/Unix/Command/_make
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_make,v
retrieving revision 1.8
diff -u -r1.8 _make
--- Completion/Unix/Command/_make	21 Aug 2004 03:07:01 -0000	1.8
+++ Completion/Unix/Command/_make	12 Sep 2004 03:17:25 -0000
@@ -1,6 +1,6 @@
 #compdef make gmake pmake dmake
 
-local prev="$words[CURRENT-1]" file expl tmp is_gnu cmdargs useperl
+local prev="$words[CURRENT-1]" file expl tmp is_gnu useperl
 
 zstyle -t ":completion:${curcontext}:" use-perl && useperl=1
 _pick_variant -r is_gnu gnu=GNU unix -v -f
@@ -27,11 +27,10 @@
     if [[ $is_gnu = gnu ]] &&
        zstyle -t ":completion:${curcontext}:targets" call-command; then
        if [[ -n $useperl ]]; then
-        cmdargs=(perl -ne '/^([a-zA-Z0-9][^\/\t=]+):([^=]|$)/ && print "$1\n"')
+        tmp=( $(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .PHONY 2> /dev/null | perl -ne '/^([a-zA-Z0-9][^\/\t=]+):([^=]|$)/ && print "$1\n"') )
        else
-        cmdargs=(awk '/^[a-zA-Z0-9][^\/\t=]+:([^=]|$)/ {print $1}' FS=:)
+        tmp=(${${(M)${(f)"$(_call_program targets "$words[1]" -nsp --no-print-directory -f /tmp/rules .PHONY 2>/dev/null)"}:#[a-zA-Z0-9][^/\t=]##:([^=]*|(#e))}%:*})
        fi
-       tmp=( $(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .PHONY 2> /dev/null | $cmdargs) )
     elif [[ -n $useperl ]]; then
       tmp=(
       $(perl -ne '/^([a-zA-Z0-9][^\/\t=]+):([^=]|$)/ and print "$1\n";
@@ -42,11 +41,9 @@
 ' $file)
 	)
     else
-      tmp=(
-            $(awk '/^[a-zA-Z0-9][^\/\t=]+:([^=]|$)/ {print $1}
- 	      /^\.include  *<bsd\.port\.(subdir\.|pre\.)?mk>/ || /^\.include  *".*mk\/bsd\.pkg\.(subdir\.)?mk"/ {
- 	        print "fetch fetch-list extract patch configure build install reinstall deinstall package describe checkpatch checksum makesum" }' \
- 	     FS=: $file)
+      tmp=( ${${(M)${(f)"$(<$file)"}:#[a-zA-Z0-9][^/\t=]##:([^=]*|(#e))}%:*}
+            $(grep -E '^\.include  *(<bsd\.port\.(subdir\.|pre\.)?mk>|".*mk\/bsd\.pkg\.(subdir\.)?mk")' $file >/dev/null &&
+ 	        print "fetch fetch-list extract patch configure build install reinstall deinstall package describe checkpatch checksum makesum" )
            )
     fi
     _wanted targets expl 'make target' compadd -a tmp && return 0

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Wayne Davison]

On Sat, Sep 11, 2004 at 11:24:58PM -0400, Clint Adams wrote:
> Should we move toward 20277 instead?

I'm curious what people think about that new version of _make that
avoids using perl and awk.  Here's a link to that message:

http://www.zsh.org/mla/workers/2004/msg00926.html

Should we go this route?  Maybe someone has some really large Makefiles
that could be used for some speed comparisons?

Also, we'd need to figure out if it is handling non-gnu include files
properly (there is probably still a little more work to do there).

If we don't want to go this route, I'd like to see the existing perl/awk
code enhanced to do variable expansion and include-file following.

..wayne..

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Bart Schaefer]

On Wed, 29 Sep 2004, Wayne Davison wrote:

> On Sat, Sep 11, 2004 at 11:24:58PM -0400, Clint Adams wrote:
> > Should we move toward 20277 instead?
> 
> I'm curious what people think about that new version of _make that
> avoids using perl and awk.

I'd be happy with it.

> Also, we'd need to figure out if it is handling non-gnu include files
> properly (there is probably still a little more work to do there).

Unfortunately I'm no help there, any longer.

Re: Bug#271196: makefile completion catches

[Geoff Wing]

Bart Schaefer <schaefer@brasslantern.com> typed:
: On Wed, 29 Sep 2004, Wayne Davison wrote:
:> On Sat, Sep 11, 2004 at 11:24:58PM -0400, Clint Adams wrote:
:> > Should we move toward 20277 instead?
:
:> Also, we'd need to figure out if it is handling non-gnu include files
:> properly (there is probably still a little more work to do there).

The make used by the *BSD's has extensions for assignment operators,
viz. ``?='' and ``!='', which aren't handled properly.

     ?=      Assign the value to the variable if it is not already defined.

     !=      Expand the value and pass it to the shell for execution and
             assign the result to the variable.  Any newlines in the result
             are replaced with spaces.

% echo 'FOO!=bar' >Makefile
% make <TAB>
make (eval):1: command not found: FOO!=bar

...

Regards,
Geoff

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Peter Stephenson]

Wayne Davison wrote:
> On Sat, Sep 11, 2004 at 11:24:58PM -0400, Clint Adams wrote:
> > Should we move toward 20277 instead?
> 
> I'm curious what people think about that new version of _make that
> avoids using perl and awk.

A quick test suggests the performance is perfectly acceptable with a
typical makefile.  I haven't gone into much detail.

I did notice that this target:

  builds/$(CONFIG)/stamp-builddir

gets shortened to

  builds//stamp-builddir

I realise there it's not likely to be able to do the substitution
properly, but I'm not sure why it's doing it at all.  (The basic
substitution problem is the same for include files.)

However, none of this is any worse than in the old system; in fact, it
appears to find a lot more targets and be better about stripping
colons.  So I can't see any reason not to use it.

pws


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Wayne Davison]

On Thu, Sep 30, 2004 at 10:44:59AM +0100, Peter Stephenson wrote:
> I did notice that this target:
>   builds/$(CONFIG)/stamp-builddir
> gets shortened to
>   builds//stamp-builddir
> 
> I realise there it's not likely to be able to do the substitution
> properly, but I'm not sure why it's doing it at all.

I wanted substitutions for targets such as this:

rsync$(EXEEXT): ...

so that completion returns either "rsync" or "rsync.exe", as
appropriate.  I'm surprised that the makefile you cited didn't set
CONFIG somewhere inside it.  Perhaps it was in an include that didn't
get processed?

One option that could make this better is to not expand variables that
are not set.  Thus, in the case of the $(EXEEXT) var (since it is
usually set to an empty string) it would get expanded.  In your cited
case (if CONFIG was not set) the target would be returned unchanged.
The only drawback I see to this is that some folks might be depending on
the fact that an unset variable is just as good as an empty variable in
a makefile.  However, since there would be a work-around for such
situations (by setting the variable to an empty value), I could live
with that behavior.  Thoughts?

..wayne..

Re: [joshk@triplehelix.org: Bug#271196: makefile completion catches some delayed expansions as targets]

[Peter Stephenson]

Wayne Davison wrote:
> On Thu, Sep 30, 2004 at 10:44:59AM +0100, Peter Stephenson wrote:
> > I did notice that this target:
> >   builds/$(CONFIG)/stamp-builddir
> > gets shortened to
> >   builds//stamp-builddir
> > 
> > I realise there it's not likely to be able to do the substitution
> > properly, but I'm not sure why it's doing it at all.
> 
> I wanted substitutions for targets such as this:
> 
> rsync$(EXEEXT): ...
> 
> so that completion returns either "rsync" or "rsync.exe", as
> appropriate.  I'm surprised that the makefile you cited didn't set
> CONFIG somewhere inside it.  Perhaps it was in an include that didn't
> get processed?

I can see your behaviour's more use, I didn't realise it was expanding
rather than removing the variables.  The makefile in question calls
itself recursively, so it's hard to predict how CONFIG will be set.  So
I don't think this needs any fundamental change.

It would be quite nice if it got set on

make CONFIG=foo <TAB>

however.  Maybe that's not too hard to add.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR Ltd., Science Park, Milton Road,
Cambridge, CB4 0WH, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************