New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve make target completion #3628

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@terlar
Contributor

terlar commented Dec 7, 2016

I am not sure if the code is as good as it can be, please let me know if
there are any potential bugs that you spot straight away.

I have been using Makefile's quite extensively recently and missed the
completion of dynamic targets and also when I run tasks for subfolders
via make -C.

This pull-request solves these two issues, first by getting the make
targets by querying the make database. This solution is based on
the ideas presented here.

This also wraps the call to __fish_print_make_targets and figures out
if the commandline contains any -C or --directory, if that is the
case the directory is assigned to a variable and then passed along to
the function.

I have introduced a new argument to the function
__fish_print_make_targets which is the directory to look for the
Makefile, it will default to . if none provided.

Potential issues (or likely) issues, are that we now explicitly use
the make command and don't parse these files and thus if the command
would be something else it would not work. Personally I have only had
the use-case with make though.

TLDR;

  • Support completing dynamic make targets.
  • Support completing make targets when using -C/--directory.
Improve make target completion
- Support completing dynamic make targets.
- Support completing make targets when using -C/--directory.
Show outdated Hide outdated share/completions/make.fish
@@ -1,11 +1,19 @@
# Completions for make
function __fish_complete_make_targets
set directory (echo $argv | string replace -r '^make .*(-C|--directory) ?([^ ]*) .*$' '$2')

This comment has been minimized.

@faho

faho Dec 7, 2016

Member

Unnecessary use of echo - string replace -r '...' '...' -- $argv.

Also, this does not handle the "--directory=dir" form that at least GNU make also reads.

@faho

faho Dec 7, 2016

Member

Unnecessary use of echo - string replace -r '...' '...' -- $argv.

Also, this does not handle the "--directory=dir" form that at least GNU make also reads.

Show outdated Hide outdated share/functions/__fish_print_make_targets.fish
__fish_sgrep -h -o -E '^[^#%=$[:space:]][^#%=$]*:([^=]|$)' $file ^/dev/null | rev | cut -d ":" -f 2- | rev | sed -e 's/^ *//;s/ *$//;s/ */\\
/g' ^/dev/null
# On case insensitive filesystems, Makefile and makefile are the same; stop now so we don't double-print
make -C $directory -prRn | awk -v RS= -F: '/^# File/,/^# Finished Make data base/ {if ($1 !~ "^[#.]") {print $1}}' ^/dev/null

This comment has been minimized.

@faho

faho Dec 7, 2016

Member

This prints absolutely nothing in my fish clone, because the make -prRn output is localized - "# „Make“-Datenbank; erstellt am: Wed Dec 7 12:04:14 2016". (Yes, including those annoying quotes).

A possible workaround is to set -lx LC_ALL C in this function.

GNU make and GNU awk.

@faho

faho Dec 7, 2016

Member

This prints absolutely nothing in my fish clone, because the make -prRn output is localized - "# „Make“-Datenbank; erstellt am: Wed Dec 7 12:04:14 2016". (Yes, including those annoying quotes).

A possible workaround is to set -lx LC_ALL C in this function.

GNU make and GNU awk.

This comment has been minimized.

@terlar

terlar Dec 7, 2016

Contributor

Good find, I found that this might work instead, but not sure if it is safe to filter out all targets with %. Which one do you prefer the set -lx LC_ALL C or this one bellow:

make -C source/server -prRn | awk -v RS= -F: '{if ($1 !~ "(^[#.])|%") {print $1}}' | sort
@terlar

terlar Dec 7, 2016

Contributor

Good find, I found that this might work instead, but not sure if it is safe to filter out all targets with %. Which one do you prefer the set -lx LC_ALL C or this one bellow:

make -C source/server -prRn | awk -v RS= -F: '{if ($1 !~ "(^[#.])|%") {print $1}}' | sort

This comment has been minimized.

@faho

faho Dec 7, 2016

Member

That latter one (after removing the "-C source/server" part) prints some weird additional stuff in the fish repo. In particular: echo " CPPFLAGS = '-DLOCALEDIR=\"/usr/local/share/locale\" -DPREFIX=L\"/usr/local\" -DDATADIR=L\"/usr/local/share\" -DSYSCONFDIR=L\"/usr/local/etc\" -DBINDIR=L\"/usr/local/bin\" -DDOCDIR=L\"/usr/local/share/doc/fish\" -iquote. -iquote./src/ -DFISH_BUILD_VERSION=\"2.4.0-216-g2db0bbc\"'" ||.

@faho

faho Dec 7, 2016

Member

That latter one (after removing the "-C source/server" part) prints some weird additional stuff in the fish repo. In particular: echo " CPPFLAGS = '-DLOCALEDIR=\"/usr/local/share/locale\" -DPREFIX=L\"/usr/local\" -DDATADIR=L\"/usr/local/share\" -DSYSCONFDIR=L\"/usr/local/etc\" -DBINDIR=L\"/usr/local/bin\" -DDOCDIR=L\"/usr/local/share/doc/fish\" -iquote. -iquote./src/ -DFISH_BUILD_VERSION=\"2.4.0-216-g2db0bbc\"'" ||.

This comment has been minimized.

@terlar

terlar Dec 7, 2016

Contributor

I didn't experience this, but I went with the locale fix instead, since it worked right?

@terlar

terlar Dec 7, 2016

Contributor

I didn't experience this, but I went with the locale fix instead, since it worked right?

terlar added some commits Dec 7, 2016

Fix complete make targets string replace
- Remove the unnecessary echo.
- Support `-Cdir/path`, `-C dir/path`
- Support `--directory=dir/path`, `--directory dir/path`
Fix print make targets with non-English locale
Since we filter by text that is localized we need to ensure the correct
locale is used.
@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Dec 7, 2016

Member

This will need checking against BSD Make as well, as that has caused problems in the past.

Member

zanchey commented Dec 7, 2016

This will need checking against BSD Make as well, as that has caused problems in the past.

@faho

faho approved these changes Dec 8, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 8, 2016

Member

This works for me. Now we need someone to test it with BSD make.

Member

faho commented Dec 8, 2016

This works for me. Now we need someone to test it with BSD make.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 9, 2016

Contributor

This is not compatible with BSD make. However, if I do alias make gmake then it works great.

Contributor

krader1961 commented Dec 9, 2016

This is not compatible with BSD make. However, if I do alias make gmake then it works great.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 9, 2016

Member

@krader1961: So, what does the make -prRn output look like with BSD make?

Member

faho commented Dec 9, 2016

@krader1961: So, what does the make -prRn output look like with BSD make?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 9, 2016

Contributor

On FreeBSD 12.0:

$ make -prRn
usage: make [-BeikNnqrstWwX]
            [-C directory] [-D variable] [-d flags] [-f makefile]
            [-I directory] [-J private] [-j max_jobs] [-m directory] [-T file]
            [-V variable] [variable=value] [target ...]

Interestingly on macOS the system supplied /usr/bin/make appears to be, if not exactly identical to Gnu make, very close to it and has no problem with those options. And Homebrew has a gmake script that does exec xcrun make "$@" to invoke the system macOS Xcode make.

Contributor

krader1961 commented Dec 9, 2016

On FreeBSD 12.0:

$ make -prRn
usage: make [-BeikNnqrstWwX]
            [-C directory] [-D variable] [-d flags] [-f makefile]
            [-I directory] [-J private] [-j max_jobs] [-m directory] [-T file]
            [-V variable] [variable=value] [target ...]

Interestingly on macOS the system supplied /usr/bin/make appears to be, if not exactly identical to Gnu make, very close to it and has no problem with those options. And Homebrew has a gmake script that does exec xcrun make "$@" to invoke the system macOS Xcode make.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Dec 9, 2016

Member

Yeah, macOS (edit: Xcode, rather) just ships with an older GPL2 GNU Make rather than BSD make.

Member

floam commented Dec 9, 2016

Yeah, macOS (edit: Xcode, rather) just ships with an older GPL2 GNU Make rather than BSD make.

@faho faho added this to the fish 2.5.0 milestone Dec 9, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 9, 2016

Member

So, what do we do about BSD make? Try to run that command once and check the return status?

if make -prRn >/dev/null ^/dev/null
# GNU make
else
# The old stuff
end

?

Member

faho commented Dec 9, 2016

So, what do we do about BSD make? Try to run that command once and check the return status?

if make -prRn >/dev/null ^/dev/null
# GNU make
else
# The old stuff
end

?

@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Dec 9, 2016

Contributor

So I finally got FreeBSD installed in a virtual machine (was out of space). And can confirm that it is not present, the important part is the -p to print the database which doesn't seem to be available. It is a bit weird because many places claim to be BSD compatible using that flag and not sure how that could have worked, here for example.

I can update the PR with fallback to the old stuff if we can't come up with some better idea. Perhaps try to save the output to a variable directly to prevent running twice?

Contributor

terlar commented Dec 9, 2016

So I finally got FreeBSD installed in a virtual machine (was out of space). And can confirm that it is not present, the important part is the -p to print the database which doesn't seem to be available. It is a bit weird because many places claim to be BSD compatible using that flag and not sure how that could have worked, here for example.

I can update the PR with fallback to the old stuff if we can't come up with some better idea. Perhaps try to save the output to a variable directly to prevent running twice?

@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Dec 9, 2016

Contributor

Seems like something like this might work in BSD:

If anyone have some files they know please try it out:

make -n -d g1 3>&2 2>&1 1>&3 >/dev/null | awk -F, '/^#\*\*\* Input graph:/,/^$/ {if ($1 !~ "^#... ") {gsub(/# /,"",$1); print $1}}'

This was using bash as I didn't know how to switch the stderr and stdout in fish. So we want to pipe the stdout to /dev/null and then what comes to stderr we want to pipe and filter out the targets from.

The flag -d g1 prints the input graph, and then that is what I filter out with awk.

Contributor

terlar commented Dec 9, 2016

Seems like something like this might work in BSD:

If anyone have some files they know please try it out:

make -n -d g1 3>&2 2>&1 1>&3 >/dev/null | awk -F, '/^#\*\*\* Input graph:/,/^$/ {if ($1 !~ "^#... ") {gsub(/# /,"",$1); print $1}}'

This was using bash as I didn't know how to switch the stderr and stdout in fish. So we want to pipe the stdout to /dev/null and then what comes to stderr we want to pipe and filter out the targets from.

The flag -d g1 prints the input graph, and then that is what I filter out with awk.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 10, 2016

Contributor

how to switch the stderr and stdout in fish

This should work: make -n -d g1 >/dev/null ^| awk ...

Contributor

krader1961 commented Dec 10, 2016

how to switch the stderr and stdout in fish

This should work: make -n -d g1 >/dev/null ^| awk ...

Support for BSD make
This detects if the make command have the `-p` switch otherwise it
assumes it is BSD make and will run a different command to try to figure
out the available targets.
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Dec 13, 2016

Contributor

I have made the updates which according to my testing should work for the FreeBSD make. Don't entirely like these long lines, but I run it through the fish_indent function and then it joins the lines. The other option would be to have locally scoped functions that are destroyed by the end of the function. Is that possible? To isolate these commands. What do you think?

Contributor

terlar commented Dec 13, 2016

I have made the updates which according to my testing should work for the FreeBSD make. Don't entirely like these long lines, but I run it through the fish_indent function and then it joins the lines. The other option would be to have locally scoped functions that are destroyed by the end of the function. Is that possible? To isolate these commands. What do you think?

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 19, 2016

Contributor

Sorry to make your life difficult, @terlar, since I'm excited to see this working. However, on FreeBSD 12.0 (which I don't use other than to test issues with fish on BSD) both make [tab] and gmake [tab] only show path name completions. Not sure why.

On macOS make [tab] in the fish project produces output like this:

18:46 macpro coff  ~/p/3/fish-shell  (make-completions) make
                                                                                       (mv -f y.tab.c $@)
\#\ \ commands\ to\ execute\ \(built-in\):                                                       (Target)
\#\ \ File\ has\ been\ updated.                                                                  (Target)
\#\ \ File\ has\ not\ been\ updated.                                                             (Target)
…and 273 more rows

On Ubuntu 16.04 (Linux) I'm seeing the same problem as on macOS.

Contributor

krader1961 commented Dec 19, 2016

Sorry to make your life difficult, @terlar, since I'm excited to see this working. However, on FreeBSD 12.0 (which I don't use other than to test issues with fish on BSD) both make [tab] and gmake [tab] only show path name completions. Not sure why.

On macOS make [tab] in the fish project produces output like this:

18:46 macpro coff  ~/p/3/fish-shell  (make-completions) make
                                                                                       (mv -f y.tab.c $@)
\#\ \ commands\ to\ execute\ \(built-in\):                                                       (Target)
\#\ \ File\ has\ been\ updated.                                                                  (Target)
\#\ \ File\ has\ not\ been\ updated.                                                             (Target)
…and 273 more rows

On Ubuntu 16.04 (Linux) I'm seeing the same problem as on macOS.

Fix print make targets
Suppress the output both to stdout and stderr in the check for the
option. Also provide the directory (`C`) instead of utilizing an empty
Makefile.

It seems the empty Makefile (`/dev/null`) returns status code 1 which
fails the check and means that it will always be assumed to be FreeBSD.
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Dec 19, 2016

Contributor

@krader1961 No worries, this one was my bad. I had two different versions that I played around with, one that I tried inside FreeBSD and one outside (the one I use in my daily use). I didn't notice that I had broken the non-freebsd versions by not suppressing the stdout output as well, which made all those lines end up as completions. Also it seems what I read on some site that you could use /dev/null as fake Makefile to just check for the option wasn't true, then the command returned falsy and everything entered the BSD case.

However for me it is working on FreeBSD, are you sure those projects you are checking are not using GNU-specific make features? I just did a find / -name Makefile in my VM for FreeBSD and tried the various ones there to see if I got output, it it seemed to be correctly.

Contributor

terlar commented Dec 19, 2016

@krader1961 No worries, this one was my bad. I had two different versions that I played around with, one that I tried inside FreeBSD and one outside (the one I use in my daily use). I didn't notice that I had broken the non-freebsd versions by not suppressing the stdout output as well, which made all those lines end up as completions. Also it seems what I read on some site that you could use /dev/null as fake Makefile to just check for the option wasn't true, then the command returned falsy and everything entered the BSD case.

However for me it is working on FreeBSD, are you sure those projects you are checking are not using GNU-specific make features? I just did a find / -name Makefile in my VM for FreeBSD and tried the various ones there to see if I got output, it it seemed to be correctly.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 20, 2016

Contributor

Okay, it's working again on macOS. However while testing on Linux this statement

if make -C $directory -p >/dev/null ^/dev/null

failed because I had just done sudo make install and several files were not readable. But the main problem with that test is it causes a make all to be done. Which is not friendly. I recommend adding --dry-run (which is the same as the -n flag you use below). The alternative is to use a different test such as make -C $directory --version which succeeds with GNU make but fails with BSD make since it doesn't support the option.

I tested on FreeBSD with the iTerm2 project and it correctly enumerates the make targets.

Besides changing the BSD versus GNU make test the one other thing that would be nice is to have completions for the gmake command that is typically used on BSD systems.

Contributor

krader1961 commented Dec 20, 2016

Okay, it's working again on macOS. However while testing on Linux this statement

if make -C $directory -p >/dev/null ^/dev/null

failed because I had just done sudo make install and several files were not readable. But the main problem with that test is it causes a make all to be done. Which is not friendly. I recommend adding --dry-run (which is the same as the -n flag you use below). The alternative is to use a different test such as make -C $directory --version which succeeds with GNU make but fails with BSD make since it doesn't support the option.

I tested on FreeBSD with the iTerm2 project and it correctly enumerates the make targets.

Besides changing the BSD versus GNU make test the one other thing that would be nice is to have completions for the gmake command that is typically used on BSD systems.

Use dry run in BSD make check
We don't want to run the default make target when testing for the command.
@terlar

This comment has been minimized.

Show comment
Hide comment
@terlar

terlar Dec 21, 2016

Contributor

I wonder how we should do with gmake should the function take another argument telling it which command should be used for make? Should we detect if there is anything in PATH called gmake and in that case always use that one? Should we try to run with make first and otherwise try with gmake?

The tricky thing would be on BSD, let's say you have gmake and make, then there are two different scenarios, you try to do completions with a Makefile in the BSD format, then gmake would fail but make wouldn't. If the Makefile is in the GNU format then make would fail but gmake wouldn't. I just wonder if there is a good way to know which one should be used.

Contributor

terlar commented Dec 21, 2016

I wonder how we should do with gmake should the function take another argument telling it which command should be used for make? Should we detect if there is anything in PATH called gmake and in that case always use that one? Should we try to run with make first and otherwise try with gmake?

The tricky thing would be on BSD, let's say you have gmake and make, then there are two different scenarios, you try to do completions with a Makefile in the BSD format, then gmake would fail but make wouldn't. If the Makefile is in the GNU format then make would fail but gmake wouldn't. I just wonder if there is a good way to know which one should be used.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Dec 21, 2016

Member

you try to do completions with a Makefile in the BSD format, then gmake would fail but make wouldn't.

I was under the impression that BSD make's syntax is a subset of the GNU one. Is that not the case?

Anyway, what might work is to call whatever make is used on the commandline (via a wrapper function, not eval), and silencing stderr. If it doesn't result in completions, then what the user entered is wrong anyway.

E.g. if I enter make <TAB> in a GNU-make project, any completions might lead me to believe that I can actually use that, when really I'd need to use gmake.

(Theoretically it would even be possible to rewrite the commandline to gmake in that case, but that sounds weird)

Member

faho commented Dec 21, 2016

you try to do completions with a Makefile in the BSD format, then gmake would fail but make wouldn't.

I was under the impression that BSD make's syntax is a subset of the GNU one. Is that not the case?

Anyway, what might work is to call whatever make is used on the commandline (via a wrapper function, not eval), and silencing stderr. If it doesn't result in completions, then what the user entered is wrong anyway.

E.g. if I enter make <TAB> in a GNU-make project, any completions might lead me to believe that I can actually use that, when really I'd need to use gmake.

(Theoretically it would even be possible to rewrite the commandline to gmake in that case, but that sounds weird)

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 22, 2016

Contributor

The solution for gmake is to simply add a share/completions/gmake.fish script that blindly assumes GNU semantics. Obviously we probably want to refactor the code to reuse common bits. However, I think this PR is now in good enough shape to merge. Adding support for gmake can be done in a separate change. I don't think there is any point in trying to decide if the user is using the wrong make command since that's not really the responsibility of command completions. It's sufficient if they don't spew huge numbers of errors in that situation and otherwise behave reasonably.

Contributor

krader1961 commented Dec 22, 2016

The solution for gmake is to simply add a share/completions/gmake.fish script that blindly assumes GNU semantics. Obviously we probably want to refactor the code to reuse common bits. However, I think this PR is now in good enough shape to merge. Adding support for gmake can be done in a separate change. I don't think there is any point in trying to decide if the user is using the wrong make command since that's not really the responsibility of command completions. It's sufficient if they don't spew huge numbers of errors in that situation and otherwise behave reasonably.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Dec 22, 2016

Contributor

Squash merged as commit 2740cc8. Many thanks, @terlar.

Contributor

krader1961 commented Dec 22, 2016

Squash merged as commit 2740cc8. Many thanks, @terlar.

@krader1961 krader1961 closed this Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment