Re: [PATCH] bash: add completions
Hi Ville, On Mon, 18 Sep 2023 22:29:58 +0300, Ville Skyttä wrote: > On Mon, 18 Sept 2023 at 15:56, Jean Delvare wrote: > > > Hi Ville, > > > > On Sat, 22 Jul 2023 22:55:31 +0300, Ville Skyttä wrote: > > > Offer only long options, but recognize short ones, too, per rationale at > > > > > https://github.com/scop/bash-completion/blob/4d0bffb791c34c96114aeb2e4f6726b80aa8698e/CONTRIBUTING.md?plain=1#L136-L153 > > > > > > > > Signed-off-by: Ville Skyttä > > > --- > > > Makefile| 14 ++-- > > > biosdecode.bash | 40 + > > > dmidecode.bash | 95 + > > > ownership.bash | 33 + > > > vpddecode.bash | 45 +++ > > > > Please create a directory for completion files, the root directory of > > the project is already cluttered enough as is. > > Done. > > I'm attaching the revised changeset as a single patch attachment here, hope > that's ok (my git/send-email-fu isn't up to dealing with followups > properly). This new one includes also scraping of --string and --type > values as discussed in another message. A new patch is fine, as I have not committed anything yet. > > +compdir = $(shell pkg-config --variable=completionsdir bash-completion > > 2>/dev/null || echo $(prefix)/etc/bash_completion.d) > > > > This is going to potentially install files outside of $(prefix) by > > default, which is questionable. > > True. Then again if the prefix or lookup dirs of bash-completion are > actually different from our $(prefix), installing in our $(prefix) will > cause the completions to not be found. I think this is the best we can do. It really depends on how bash completion scripts are handled by the system. For example, on my openSUSE system, scripts can be installed in both /usr/share/bash-completion/completions/ and /etc/bash_completion.d/. I can imagine that some distributions would also look for completions in /usr/local/share/bash-completion/completions/ or /usr/local/etc/bash_completion.d/, the same way /usr/local/bin is part of the PATH on most distributions. On my system, it is also possible to set the environment variable BASH_COMPLETION_COMPAT_DIR to an arbitrary location which contains bash completion scripts (although I'm unsure if this possibility will persist, given its name). So I could decide to set it to /usr/local/share/bash-completion/completions on my account, so that locally installed bash completion scripts are taken into account. Anyway, independently of where bash completion files must reside to be taken into account, I consider "make install" writing outside of $(DESTDIR)$(prefix) by default as not acceptable. While the user is free to overwrite $(compdir) if he/she wants to, it should be within $(prefix) by default, to avoid silently overwriting existing system files. I'm not aware of any other package doing things the way you suggest (i.e. installing files outside of $(DESTDIR)$(prefix) by default). Another thing I'm worried about is the portability of the $(shell) call. So far I tried to ensure that the Makefile is compatible with alternative implementations such as bmake. This is valuable to non-Linux users. Now to the completion scripts themselves. I gave them a try. Seems to work well, but I found one bug: the trailing "n" of string names is eaten during completion: # dmidecode --string baseboard-asset-tag chassis-asset-tagprocessor-versio baseboard-manufacturer chassis-manufacturer system-family baseboard-product-name chassis-serial-numbersystem-manufacturer baseboard-serial-number chassis-type system-product-name baseboard-versio chassis-versio system-serial-number bios-release-datefirmware-revisio system-sku-number bios-revisio processor-family system-uuid bios-vendor processor-frequency system-versio bios-versio processor-manufacturer # dmidecode --string chassis-ver completes to "chassis-versio" I suppose this is a mistake in the post-processing of the "dmidecode --string" error output. I'm willing to implement the "--string ?" option we discussed previously if it makes it easier to collect the available string names. -- Jean Delvare SUSE L3 Support
Re: [PATCH] bash: add completions
On Mon, 18 Sept 2023 at 16:06, Jean Delvare wrote: > Hi Ville, > > On Sun, 23 Jul 2023 10:01:34 +0300, Ville Skyttä wrote: > > On Sat, 22 Jul 2023 at 22:55, Ville Skyttä wrote: > > [...] > > > diff --git a/dmidecode.bash b/dmidecode.bash > > [...] > > > + -s | --string) > > > + COMPREPLY=($(compgen -W ' > > > + bios-vendor > > > + bios-version > > [...] > > > > Instead of duplicating the keywords here, these could be scraped from > > `dmidecode --string` (without providing the keyword arg) output if > > that sounds acceptable and the output is considered stable enough for > > that purpose. > > Yes, that output can be considered stable. I can't think of a reason to > change its format in a foreseeable future. > Ok, --string and --type pure bash scraping implemented in the revised patch I just sent as a reply to another message here. Ville
Re: [PATCH] bash: add completions
On Mon, 18 Sept 2023 at 15:56, Jean Delvare wrote:
> Hi Ville,
>
> On Sat, 22 Jul 2023 22:55:31 +0300, Ville Skyttä wrote:
> > Offer only long options, but recognize short ones, too, per rationale at
> >
> https://github.com/scop/bash-completion/blob/4d0bffb791c34c96114aeb2e4f6726b80aa8698e/CONTRIBUTING.md?plain=1#L136-L153
> >
> > Signed-off-by: Ville Skyttä
> > ---
> > Makefile| 14 ++--
> > biosdecode.bash | 40 +
> > dmidecode.bash | 95 +
> > ownership.bash | 33 +
> > vpddecode.bash | 45 +++
>
> Please create a directory for completion files, the root directory of
> the project is already cluttered enough as is.
>
Done.
I'm attaching the revised changeset as a single patch attachment here, hope
that's ok (my git/send-email-fu isn't up to dealing with followups
properly). This new one includes also scraping of --string and --type
values as discussed in another message.
> +compdir = $(shell pkg-config --variable=completionsdir bash-completion
> 2>/dev/null || echo $(prefix)/etc/bash_completion.d)
>
> This is going to potentially install files outside of $(prefix) by
> default, which is questionable.
>
True. Then again if the prefix or lookup dirs of bash-completion are
actually different from our $(prefix), installing in our $(prefix) will
cause the completions to not be found. I think this is the best we can do.
> > -install : install-bin install-man install-doc
> > +install : install-bin install-man install-doc install-completion
>
> I'm worried about this being enabled by default while the system may not
> even have bash installed. Can we enable this by default only if
> $(compdir) exists? This directory would typically be owned by the bash
> package.
>
I guess that'd work, done.
Ville
From 1a727665d43ae90bcff83ddc7dc1f4ed90a7d98a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ville=20Skytt=C3=A4?=
Date: Fri, 21 Jul 2023 16:33:28 +0300
Subject: [PATCH] bash: add completions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Offer only long options, but recognize short ones, too, per rationale at
https://github.com/scop/bash-completion/blob/4d0bffb791c34c96114aeb2e4f6726b80aa8698e/CONTRIBUTING.md?plain=1#L136-L153
Signed-off-by: Ville Skyttä
---
Makefile | 18 +--
completion/biosdecode.bash | 40 +++
completion/dmidecode.bash | 66 ++
completion/ownership.bash | 33 +++
completion/vpddecode.bash | 43 +
5 files changed, 198 insertions(+), 2 deletions(-)
create mode 100644 completion/biosdecode.bash
create mode 100644 completion/dmidecode.bash
create mode 100644 completion/ownership.bash
create mode 100644 completion/vpddecode.bash
diff --git a/Makefile b/Makefile
index 7aa729d..6fc946b 100644
--- a/Makefile
+++ b/Makefile
@@ -36,6 +36,7 @@ sbindir = $(prefix)/sbin
mandir = $(prefix)/share/man
man8dir = $(mandir)/man8
docdir = $(prefix)/share/doc/dmidecode
+compdir = $(shell pkg-config --variable=completionsdir bash-completion 2>/dev/null || echo $(prefix)/etc/bash_completion.d)
INSTALL := install
INSTALL_DATA:= $(INSTALL) -m 644
@@ -113,9 +114,9 @@ util.o : util.c types.h util.h config.h
strip : $(PROGRAMS)
strip $(PROGRAMS)
-install : install-bin install-man install-doc
+install : install-bin install-man install-doc install-completion
-uninstall : uninstall-bin uninstall-man uninstall-doc
+uninstall : uninstall-bin uninstall-man uninstall-doc uninstall-completion
install-bin : $(PROGRAMS)
$(INSTALL_DIR) $(DESTDIR)$(sbindir)
@@ -144,5 +145,18 @@ install-doc :
uninstall-doc :
$(RM) -r $(DESTDIR)$(docdir)
+install-completion :
+ if [ -d $(compdir) ] ; then \
+ $(INSTALL_DIR) $(DESTDIR)$(compdir) ; \
+ for program in $(PROGRAMS) ; do \
+ $(INSTALL_DATA) completion/$$program.bash $(DESTDIR)$(compdir)/$$program ; done ; \
+ fi
+
+uninstall-completion :
+ if [ -d $(DESTDIR)$(compdir) ]; then \
+ for program in $(PROGRAMS) ; do \
+ $(RM) $(DESTDIR)$(compdir)/$$program ; done ; \
+ fi
+
clean :
$(RM) *.o $(PROGRAMS) core
diff --git a/completion/biosdecode.bash b/completion/biosdecode.bash
new file mode 100644
index 000..42e0fae
--- /dev/null
+++ b/completion/biosdecode.bash
@@ -0,0 +1,40 @@
+# bash completion for biosdecode -*- shell-script -*-
+
+_comp_cmd_biosdecode() {
+ local cur prev
+ COMPREPLY=()
+ cur=${COMP_WORDS[COMP_CWORD]}
+ prev=${COMP_WORDS[COMP_CWORD - 1]}
+
+ case $prev in
+ -d | --dev-mem)
+ : "${cur:=/dev/}"
+ local IFS=$'\n'
+ compopt -o filenames
+ COMPREPLY=($(compgen -f -- "$cur"))
+ return 0
+ ;;
+ --pir)
+ COMPREPLY=($(compgen -W '
+ full
+ ' -- "$cur"))
+ return 0
+ ;;
+ -[hV] | --help | --version)
+ return 0
+ ;;
+ esac
+
+ if [[ $cur == -* ]]; then
+ COMPREPLY=($(compgen -W '
+ --dev-mem
+ --pir
+
Re: [PATCH] bash: add completions
Hi Ville, On Sun, 23 Jul 2023 10:01:34 +0300, Ville Skyttä wrote: > On Sat, 22 Jul 2023 at 22:55, Ville Skyttä wrote: > [...] > > diff --git a/dmidecode.bash b/dmidecode.bash > [...] > > + -s | --string) > > + COMPREPLY=($(compgen -W ' > > + bios-vendor > > + bios-version > [...] > > Instead of duplicating the keywords here, these could be scraped from > `dmidecode --string` (without providing the keyword arg) output if > that sounds acceptable and the output is considered stable enough for > that purpose. Yes, that output can be considered stable. I can't think of a reason to change its format in a foreseeable future. > Or let's say `--string help` could be implemented, and For name space reasons, I'd rather avoid that, although something like "--string ?" would be acceptable to me as "?" can't possibly be a valid string name. > it could print to stdout (no error), and only the keywords with no > error messages or header lines or the like if stdout is not a tty. I'd rather avoid testing stdout's type, both for portability reasons and to avoid confusion. I often test commands in a shell before including them in a shell script, and I hate when the command behaves differently once in the script. The rest is OK with me though. > Anyway, as it stands, with no modifications, could take all output > lines starting with whitespace from stderr. Ditto other keyword/type > lists across these completions. I'm fine either way. Duplicating the lists isn't necessarily that bad as they don't change often, but I understand that it's cleaner and easier to maintain to have it generated dynamically. And listing the keywords directly would obviously save some post-processing effort. Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] bash: add completions
Hi Ville, On Sat, 22 Jul 2023 22:55:31 +0300, Ville Skyttä wrote: > Offer only long options, but recognize short ones, too, per rationale at > https://github.com/scop/bash-completion/blob/4d0bffb791c34c96114aeb2e4f6726b80aa8698e/CONTRIBUTING.md?plain=1#L136-L153 > > Signed-off-by: Ville Skyttä > --- > Makefile| 14 ++-- > biosdecode.bash | 40 + > dmidecode.bash | 95 + > ownership.bash | 33 + > vpddecode.bash | 45 +++ Please create a directory for completion files, the root directory of the project is already cluttered enough as is. > 5 files changed, 225 insertions(+), 2 deletions(-) > create mode 100644 biosdecode.bash > create mode 100644 dmidecode.bash > create mode 100644 ownership.bash > create mode 100644 vpddecode.bash > > diff --git a/Makefile b/Makefile > index 7aa729d..66dc2d9 100644 > --- a/Makefile > +++ b/Makefile > @@ -36,6 +36,7 @@ sbindir = $(prefix)/sbin > mandir = $(prefix)/share/man > man8dir = $(mandir)/man8 > docdir = $(prefix)/share/doc/dmidecode > +compdir = $(shell pkg-config --variable=completionsdir bash-completion > 2>/dev/null || echo $(prefix)/etc/bash_completion.d) This is going to potentially install files outside of $(prefix) by default, which is questionable. > > INSTALL := install > INSTALL_DATA:= $(INSTALL) -m 644 > @@ -113,9 +114,9 @@ util.o : util.c types.h util.h config.h > strip : $(PROGRAMS) > strip $(PROGRAMS) > > -install : install-bin install-man install-doc > +install : install-bin install-man install-doc install-completion I'm worried about this being enabled by default while the system may not even have bash installed. Can we enable this by default only if $(compdir) exists? This directory would typically be owned by the bash package. > > -uninstall : uninstall-bin uninstall-man uninstall-doc > +uninstall : uninstall-bin uninstall-man uninstall-doc uninstall-completion > > install-bin : $(PROGRAMS) > $(INSTALL_DIR) $(DESTDIR)$(sbindir) > @@ -144,5 +145,14 @@ install-doc : > uninstall-doc : > $(RM) -r $(DESTDIR)$(docdir) > > +install-completion : > + $(INSTALL_DIR) $(DESTDIR)$(compdir) > + for program in $(PROGRAMS) ; do \ > + $(INSTALL_DATA) $$program.bash $(DESTDIR)$(compdir)/$$program ; done > + > +uninstall-completion : > + for program in $(PROGRAMS) ; do \ > + $(RM) $(DESTDIR)$(compdir)/$$program ; done > + > clean : > $(RM) *.o $(PROGRAMS) core > diff --git a/biosdecode.bash b/biosdecode.bash > new file mode 100644 > index 000..42e0fae > --- /dev/null > +++ b/biosdecode.bash > @@ -0,0 +1,40 @@ > (...) I'm not going to review the completion scripts themselves, I haven't dealt with such files for a long time so I wouldn't know what to look for. But I'll install them on my system to give them some testing. Thanks, -- Jean Delvare SUSE L3 Support
Re: [PATCH] bash: add completions
On Sat, 22 Jul 2023 at 22:55, Ville Skyttä wrote: [...] > diff --git a/dmidecode.bash b/dmidecode.bash [...] > + -s | --string) > + COMPREPLY=($(compgen -W ' > + bios-vendor > + bios-version [...] Instead of duplicating the keywords here, these could be scraped from `dmidecode --string` (without providing the keyword arg) output if that sounds acceptable and the output is considered stable enough for that purpose. Or let's say `--string help` could be implemented, and it could print to stdout (no error), and only the keywords with no error messages or header lines or the like if stdout is not a tty. Anyway, as it stands, with no modifications, could take all output lines starting with whitespace from stderr. Ditto other keyword/type lists across these completions.
