zsh make(1) completion on FreeBSD

zsh make(1) completion on FreeBSD

[Guilherme Salazar]

Hey,

The _make completion script has the following snippet:

case "$OSTYPE" in
  freebsd*)
    _make-parseMakefile $PWD < <(_call_program targets "$words[1]"
-nsp -f "$file" 4| .PHONY 2> /dev/null)
  ;;

That is, _call_program invokes the make command ($words[1]) with the
given options (-nsp...) to get the make database. The issue is the
default make utility in FreeBSD is not GNU Make and it does not
support these options; to get something in FreeBSD make, we could use
the -d option (for debugging), along with A (for all) -- see [1] for
details.

If one has gmake installed, one can replace "$words[1]" with gmake or
g"$words[1]" to have gmake dump the database info, but I'm not sure
it's the ideal solution.

[1] https://www.freebsd.org/cgi/man.cgi?query=make&apropos=0&sektion=1&manpath=FreeBSD+11-current&arch=default&format=html

Cheers,
G. Salazar

Re: zsh make(1) completion on FreeBSD

[Daniel Shahaf]

Guilherme Salazar wrote on Wed, Oct 05, 2016 at 23:56:52 -0300:
> Hey,
> 
> The _make completion script has the following snippet:
> 
> case "$OSTYPE" in
>   freebsd*)
>     _make-parseMakefile $PWD < <(_call_program targets "$words[1]"
> -nsp -f "$file" 4| .PHONY 2> /dev/null)
>   ;;
> 
> That is, _call_program invokes the make command ($words[1]) with the
> given options (-nsp...) to get the make database. The issue is the
> default make utility in FreeBSD is not GNU Make and it does not
> support these options; to get something in FreeBSD make, we could use
> the -d option (for debugging), along with A (for all) -- see [1] for
> details.
> 

I'd like to see this fixed but I'm not on FreeBSD right now.  Any chance
you could write a patch?  I think there are two approaches, either (a)
figure out what incantation of BSD make is equivalent to «gmake -nspf
$file», or (b) use the -d switch to ask make what are the targets,
variables, and other things that _make-parseMakefile extracts.

> If one has gmake installed, one can replace "$words[1]" with gmake or
> g"$words[1]" to have gmake dump the database info, but I'm not sure
> it's the ideal solution.
> 

g$words[1] won't work when $words[1] is "/usr/bin/make", and in any case
I'd be concerned about syntaxes that are valid to BSD make but not to
GNU make; there are bound to be numerous instances of that in the ports
tree.

Thanks,

Daniel

> [1] https://www.freebsd.org/cgi/man.cgi?query=make&apropos=0&sektion=1&manpath=FreeBSD+11-current&arch=default&format=html
> 
> Cheers,
> G. Salazar
> 

Re: zsh make(1) completion on FreeBSD

[Guilherme Salazar]

Hi Daniel,

> I'd like to see this fixed but I'm not on FreeBSD right now.  Any chance
> you could write a patch?  I think there are two approaches, either (a)
> figure out what incantation of BSD make is equivalent to «gmake -nspf
> $file», or (b) use the -d switch to ask make what are the targets,
> variables, and other things that _make-parseMakefile extracts.

Baptiste (cc'ed) kindly sent me a patch he uses to get targets
completion [1]. Using it and adding variable completion, I got to the
following:

````
--- /usr/ports/shells/zsh/work/zsh-5.2/Completion/Unix/Command/_make
 2015-08-08 14:51:33.000000000 -0300
+++ /usr/local/share/zsh/5.2/functions/Completion/Unix/_make
2016-10-11 20:14:43.403084000 -0300
@@ -268,7 +268,14 @@
       else
         case "$OSTYPE" in
           freebsd*)
-          _make-parseMakefile $PWD < <(_call_program targets
"$words[1]" -nsp -f "$file" .PHONY 2> /dev/null)
+          if [[ $words[1] == *'gmake'* ]]
+          then
+            args="-nsp"
+          else
+            args="-nsdg1Fstdout"
+            TARGETS+=(${=${(f)"$(_call_program targets \"$words[1]\"
-s -f "$file" -V.ALLTARGETS 2> /dev/null)"}})
+          fi
+          _make-parseMakefile $PWD < <(_call_program targets
"$words[1]" $args -f "$file" .PHONY 2> /dev/null)
     ;;
     *)
           _make-parseMakefile $PWD < $file
````

It works well for bmake without breaking gmake.

[1] https://people.freebsd.org/~bapt/_make.diff

Re: zsh make(1) completion on FreeBSD

[Daniel Shahaf]

Guilherme Salazar wrote on Tue, Oct 11, 2016 at 20:27:14 -0300:
> Hi Daniel,
> 
> > I'd like to see this fixed but I'm not on FreeBSD right now.  Any chance
> > you could write a patch?  I think there are two approaches, either (a)
> > figure out what incantation of BSD make is equivalent to «gmake -nspf
> > $file», or (b) use the -d switch to ask make what are the targets,
> > variables, and other things that _make-parseMakefile extracts.
> 
> Baptiste (cc'ed) kindly sent me a patch he uses to get targets
> completion [1]. Using it and adding variable completion, I got to the
> following:
> 

Thanks, both of you.

> ````
> --- /usr/ports/shells/zsh/work/zsh-5.2/Completion/Unix/Command/_make
>  2015-08-08 14:51:33.000000000 -0300
> +++ /usr/local/share/zsh/5.2/functions/Completion/Unix/_make
> 2016-10-11 20:14:43.403084000 -0300
> @@ -268,7 +268,14 @@
>        else
>          case "$OSTYPE" in
>            freebsd*)
> -          _make-parseMakefile $PWD < <(_call_program targets
> "$words[1]" -nsp -f "$file" .PHONY 2> /dev/null)
> +          if [[ $words[1] == *'gmake'* ]]
> +          then
> +            args="-nsp"
> +          else
> +            args="-nsdg1Fstdout"
> +            TARGETS+=(${=${(f)"$(_call_program targets \"$words[1]\"
> -s -f "$file" -V.ALLTARGETS 2> /dev/null)"}})

The correct way to quote $words[1] would have been «${(q)words[1]}», but
elements of $words are already command-line quoted so they don't need to
be quoted again.  That is: just using $words[1] directly would be correct.
(modulo noglob, but don't worry about that)

Since the patch was broken by your mailer, I'm reincluding it here so
it's easier to review/apply.  (You can usually send patches as
attachments named *.txt to prevent munging and ensure a sane MIME type.)

workers@: I've added this to my list to review, but my list is quite full
nowadays, feel free to beat me to applying this.

Thanks again for the patch.

Cheers,

Daniel

(patch by Guilherme and bapt)
diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index d10c8ee..de828bb 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -269,7 +269,14 @@ _make() {
       else
         case "$OSTYPE" in
           freebsd*)
-          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsp -f "$file" .PHONY 2> /dev/null)
+          if [[ $words[1] == *'gmake'* ]]
+          then
+            args="-nsp"
+          else
+            args="-nsdg1Fstdout"
+            TARGETS+=(${=${(f)"$(_call_program targets \"$words[1]\" -s -f "$file" -V.ALLTARGETS 2> /dev/null)"}})
+          fi
+          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" $args -f "$file" .PHONY 2> /dev/null)
     ;;
     *)
           _make-parseMakefile $PWD < $file

> +          fi
> +          _make-parseMakefile $PWD < <(_call_program targets
> "$words[1]" $args -f "$file" .PHONY 2> /dev/null)
>      ;;
>      *)
>            _make-parseMakefile $PWD < $file
> ````
> 
> It works well for bmake without breaking gmake.
> 
> [1] https://people.freebsd.org/~bapt/_make.diff

Re: zsh make(1) completion on FreeBSD

[Guilherme Salazar]

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

> The correct way to quote $words[1] would have been «${(q)words[1]}», but
> elements of $words are already command-line quoted so they don't need to
> be quoted again.  That is: just using $words[1] directly would be correct.
> (modulo noglob, but don't worry about that)

Thank you for the explanation.

If one's make points to gmake instead of bmake, $words[1] itself will
not be gmake. New patch attached fixes that, by looking for GNU in
`$words[1] -v`. Sorry about that, didn't think about it before ;p

[-- Attachment #2: _make.diff --]
[-- Type: text/plain, Size: 847 bytes --]

--- /usr/ports/shells/zsh/work/zsh-5.2/Completion/Unix/Command/_make	2015-08-08 14:51:33.000000000 -0300
+++ /usr/local/share/zsh/5.2/functions/Completion/Unix/_make	2016-10-11 21:15:56.295311000 -0300
@@ -268,7 +268,14 @@
       else
         case "$OSTYPE" in
           freebsd*)
-          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsp -f "$file" .PHONY 2> /dev/null)
+          if [[ `$words[1] -v 2> /dev/null` == *'GNU'* ]] 
+          then
+            args="-nsp"
+          else
+            args="-nsdg1Fstdout"
+            TARGETS+=(${=${(f)"$(_call_program targets \"$words[1]\" -s -f "$file" -V.ALLTARGETS 2> /dev/null)"}})
+          fi
+          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" $args -f "$file" .PHONY 2> /dev/null)
     ;;
     *)
           _make-parseMakefile $PWD < $file

Re: zsh make(1) completion on FreeBSD

[Daniel Shahaf]

Guilherme Salazar wrote on Tue, Oct 11, 2016 at 21:24:16 -0300:
> If one's make points to gmake instead of bmake, $words[1] itself will
> not be gmake. New patch attached fixes that, by looking for GNU in
> `$words[1] -v`. Sorry about that, didn't think about it before ;p

> --- /usr/ports/shells/zsh/work/zsh-5.2/Completion/Unix/Command/_make	2015-08-08 14:51:33.000000000 -0300
> +++ /usr/local/share/zsh/5.2/functions/Completion/Unix/_make	2016-10-11 21:15:56.295311000 -0300
> @@ -268,7 +268,14 @@
> +          if [[ `$words[1] -v 2> /dev/null` == *'GNU'* ]] 

That's precisely what the _pick_variant call at the top of the function
does, so you can just test $is_gnu instead.  Note that the enclosing if
already inspects that variable.

In current master (before your patch), the 'call-command' style is
consulted only for GNU make but not for FreeBSD.  Do you know if that's
intentional, perhaps (going by the style's docs) because the GNU make
invocation has side-effects while the BSD make invocation has none?

Re: zsh make(1) completion on FreeBSD

[Guilherme Salazar]

> That's precisely what the _pick_variant call at the top of the function
> does, so you can just test $is_gnu instead.  Note that the enclosing if
> already inspects that variable.

$is_gnu will still give unix (on FreeBSD) in case `which make` is just
a symlink to /usr/local/bin/gmake.

> In current master (before your patch), the 'call-command' style is
> consulted only for GNU make but not for FreeBSD.  Do you know if that's
> intentional, perhaps (going by the style's docs) because the GNU make
> invocation has side-effects while the BSD make invocation has none?

I'd expect the -n option to avoid side effects. Perhaps the reason is
that the BSD make infrastructure is a lot different than GNU's and a
single Makefile may not carry enough information by itself to generate
good completion?

Re: zsh make(1) completion on FreeBSD

[Guilherme Salazar]

By the way, completely unrelated question: I noticed some
inconsistencies in the use of tabs/spaces for indentation; are there
conventions for that in the project? If so, are patches fixing it
welcome?

On Tue, Oct 11, 2016 at 10:14 PM, Guilherme Salazar
<gmesalazar@gmail.com> wrote:
>> That's precisely what the _pick_variant call at the top of the function
>> does, so you can just test $is_gnu instead.  Note that the enclosing if
>> already inspects that variable.
>
> $is_gnu will still give unix (on FreeBSD) in case `which make` is just
> a symlink to /usr/local/bin/gmake.
>
>> In current master (before your patch), the 'call-command' style is
>> consulted only for GNU make but not for FreeBSD.  Do you know if that's
>> intentional, perhaps (going by the style's docs) because the GNU make
>> invocation has side-effects while the BSD make invocation has none?
>
> I'd expect the -n option to avoid side effects. Perhaps the reason is
> that the BSD make infrastructure is a lot different than GNU's and a
> single Makefile may not carry enough information by itself to generate
> good completion?

Re: zsh make(1) completion on FreeBSD

[Oliver Kiddle]

Guilherme Salazar wrote:
> By the way, completely unrelated question: I noticed some
> inconsistencies in the use of tabs/spaces for indentation; are there
> conventions for that in the project? If so, are patches fixing it
> welcome?

Coding conventions are mentioned in the following two files
  Etc/zsh-development-guide
  Etc/completion-style-guide
Theres also a .editorconfig file.

The short version is that for C, it should be 4 spaces for indentation
using tab characters for blocks of 8 spaces. For completion functions,
it should be 2 (but 4 for continuation lines). Some other functions such
as vcs_info use a different convention and I wouldn't change them.
Completions tend to need long lines so short indentation helps avoid
wrapping. .editorconfig also implies tab characters in the completion functions
which I'm not sure is particularly clever.

As for fixing inconsistencies, it depends:
The trouble with patches that only fix indentation is that they can make
it harder to follow git history. git diff and blame do have a -w option
but still.

In the case of completions, I correct the indentation when making
changes that affect many lines. For example, _zfs has capitalised
descriptions so it would be reasonable to reindent it if also
correcting the descriptions to follow the convention of all lowercase:
descriptions appear on most lines.

You could also argue that fixing indentation is harmless if there is
virtually no history of changes to the file anyway - perhaps common
in the case of function that doesn't follow the conventions anyway,
e.g. _gradle has 2 changes, _gnutls has 4, _zfs has 11 which is a
bit more.

Ultimately, updating the functions for new options is more useful.

Oliver

Re: zsh make(1) completion on FreeBSD

[Daniel Shahaf]

Oliver Kiddle wrote on Thu, Oct 13, 2016 at 12:08:32 +0200:
> As for fixing inconsistencies, it depends:
> The trouble with patches that only fix indentation is that they can make
> it harder to follow git history. git diff and blame do have a -w option
> but still.
> 
> In the case of completions, I correct the indentation when making
> changes that affect many lines. For example, _zfs has capitalised
> descriptions so it would be reasonable to reindent it if also
> correcting the descriptions to follow the convention of all lowercase:
> descriptions appear on most lines.

Agreed that a patch that touches a line can fix indentation on that
line; doing so doesn't make 'blame' any harder to read.

However, some other kinds of style changes should be done in separate
commits.  For example, the _tmux part of 39067 deleted the «function»
keyword and the $args arrays, whilst also making other changes;
consequently it was very hard to review.¹  I think those two file-wide
changes are examples of style changes that shouldn't be done in the same
commit as functional changes.

(Incidentally, I prefer having $args arrays to multiline _arguments
calls because the former (a) is immune to "Forgot to append a backslash"
bugs, (b) allows writing comments between option specifications.)

Cheers,

Daniel

¹ That reminds me of the infamous "goto fail" bug.

Re: zsh make(1) completion on FreeBSD

[Daniel Shahaf]

Guilherme Salazar wrote on Tue, Oct 11, 2016 at 22:14:04 -0300:
> > That's precisely what the _pick_variant call at the top of the function
> > does, so you can just test $is_gnu instead.  Note that the enclosing if
> > already inspects that variable.
> 
> $is_gnu will still give unix (on FreeBSD) in case `which make` is just
> a symlink to /usr/local/bin/gmake.

I went over this with Baptiste on IRC.  _pick_variant behaves as
expected; however, the freebsd* case was taken for both bmake and gmake
on that OS (when call-command is unset, which is the default).

This patch refactors the code to avoid that, adds the TARGETS+= and
-nsdg1Fstdout magic from the original patch, and also adds dragonfly and
netbsd support at Baptiste's suggestion.

I left call-comand out of the non-GNU path since that's how it was
originally.

Thanks,

Daniel

[[[
diff --git a/Completion/Unix/Command/_make b/Completion/Unix/Command/_make
index d10c8ee..35a892c 100644
--- a/Completion/Unix/Command/_make
+++ b/Completion/Unix/Command/_make
@@ -263,17 +263,20 @@ _make() {
 
     if [[ -n "$file" ]]
     then
-      if [[ $is_gnu == gnu ]] && zstyle -t ":completion:${curcontext}:targets" call-command
+      if [[ $is_gnu == gnu ]] 
       then
-        _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .PHONY 2> /dev/null)
+        if zstyle -t ":completion:${curcontext}:targets" call-command; then
+          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsp --no-print-directory -f "$file" .PHONY 2> /dev/null)
+        else
+          _make-parseMakefile $PWD < $file
+        fi
       else
-        case "$OSTYPE" in
-          freebsd*)
-          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsp -f "$file" .PHONY 2> /dev/null)
-    ;;
-    *)
+        if [[ $OSTYPE == (freebsd|dragonfly|netbsd)* || /$words[1] == */bmake* ]]; then
+          TARGETS+=(${=${(f)"$(_call_program targets "$words[1]" -s -f "$file" -V.ALLTARGETS 2> /dev/null)"}})
+          _make-parseMakefile $PWD < <(_call_program targets "$words[1]" -nsdg1Fstdout -f "$file" .PHONY 2> /dev/null)
+        else
           _make-parseMakefile $PWD < $file
-        esac
+        fi
       fi
     fi
 
]]]

> > In current master (before your patch), the 'call-command' style is
> > consulted only for GNU make but not for FreeBSD.  Do you know if that's
> > intentional, perhaps (going by the style's docs) because the GNU make
> > invocation has side-effects while the BSD make invocation has none?
> 
> I'd expect the -n option to avoid side effects. Perhaps the reason is
> that the BSD make infrastructure is a lot different than GNU's and a
> single Makefile may not carry enough information by itself to generate
> good completion?
>