Supressing output from completion script

[kevinoid]

While tracking down a syntax error in a bash completion script, I noticed that stdout and stderr are redirected to /dev/null for all completion scripts. It looks like this has been present since 20c05b43b, and probably for good reason, but it seems like a big hammer which may be papering over many errors (such as the one I eventually tracked down to a syntax error, due to a missing ;). Since I hadn't seen it discussed before, I'm opening this issue to discuss what problems it may be solving and whether it's the best approach.

Thanks for considering, Kevin

[scop]

That's a fair question. I think we should not mute everything, #508 has a candidate fix. Thoughts?

[kevinoid]

Looks great! Thanks for considering it and for working up a fix. I appreciate it, and I think it'll be a positive change.

It wouldn't surprise me if some third-party bash-completion scripts inadvertently produce output. When I get a chance, I'll see if I can work up a reasonable test to see how many bash-completion scripts in the Debian archive produce output. That might give us an idea of how much pain the change may cause and whether any triage is required first.

From a quick look through the archive, I found 3 packages which put their completion script in a subdirectory of /usr/share/bash-completion/completions/:

• crystal-lang - /usr/share/bash-completion/completions/crystal-lang/completion.bash
• libgadap-dev - /usr/share/bash-completion/completions/bash_completion.d/gadap-config
• trace-cmd - /usr/share/bash-completion/completions/trace-cmd/trace-cmd.bash

I assume all of these are buggy and will investigate and file bugs shortly.

[kevinoid]

Yep, they were all packaging bugs:

• #984955 for crystal-lang.
• #984957 for libgadap-dev.
• #984959 for trace-cmd.

I included a trivial patch for each. Hopefully they will be fixed quickly.

[kevinoid]

I ran some quick tests on bash-completion scripts in the Debian archive. I'm still analyzing the results, but the most prominent is scripts which print:

bash: have: command not found

to stderr when sourced. Scripts with this issue:

Package	Script
alltray	/usr/share/bash-completion/completions/alltray
apt-xapian-index	/usr/share/bash-completion/completions/axi-cache
bumblebee	/usr/share/bash-completion/completions/bumblebee
coccinelle	/usr/share/bash-completion/completions/spatch
debfoster	/usr/share/bash-completion/completions/debfoster
dosage	/usr/share/bash-completion/completions/dosage-completion
dpatch	/usr/share/bash-completion/completions/dpatch_edit_patch
dupload	/usr/share/bash-completion/completions/dupload
fcoe-utils	/usr/share/bash-completion/completions/fcoeadm
fcoe-utils	/usr/share/bash-completion/completions/fcoemon
freecontact	/usr/share/bash-completion/completions/freecontact.bash-completion
insserv	/usr/share/bash-completion/completions/insserv
jackd1	/usr/share/bash-completion/completions/jackd1
jackd2	/usr/share/bash-completion/completions/jackd
konwert	/usr/share/bash-completion/completions/konwert
linkchecker	/usr/share/bash-completion/completions/linkchecker-completion
lldpad	/usr/share/bash-completion/completions/lldpad
lldpad	/usr/share/bash-completion/completions/lldptool
lyx-common	/usr/share/bash-completion/completions/lyx
ngraph-gtk	/usr/share/bash-completion/completions/ngraph
nmh	/usr/share/bash-completion/completions/nmh
ocaml-findlib	/usr/share/bash-completion/completions/ocamlfind
ola	/usr/share/bash-completion/completions/ola
patool	/usr/share/bash-completion/completions/patool.bash-completion
pmount	/usr/share/bash-completion/completions/pmount
porg	/usr/share/bash-completion/completions/porg_bash_completion
primus	/usr/share/bash-completion/completions/primus
python3-pastescript	/usr/share/bash-completion/completions/paster3
rasmol	/usr/share/bash-completion/completions/rasmol
tgt	/usr/share/bash-completion/completions/tgt
tuxpaint	/usr/share/bash-completion/completions/tuxpaint-completion.bash
ubuntu-dev-tools	/usr/share/bash-completion/completions/pbuilder-dist
unar	/usr/share/bash-completion/completions/unar
vdr	/usr/share/bash-completion/completions/svdrpsend
vobsub2srt	/usr/share/bash-completion/completions/vobsub2srt
wbar	/usr/share/bash-completion/completions/wbar

I'm guessing these scripts were written/tested with eager sourcing (e.g. in /etc/bash_completion.d where the issue does not occur) when have is still defined and became broken when sourced-on-use after have has been unset (e.g. in /usr)?

So, be aware that this change will likely reveal broken completion scripts (which I think is a good thing).

For this case, do you think it would be worth defining have before sourcing completion scripts from /usr (and unsetting it after to avoid leaking have) as a workaround for these scripts? Or is it better to let them break and have developers/maintainers fix them?

[scop]

Ooh, awesome work, thanks!

I'm leaning towards letting those scripts break "loudly", I think this is a good thing too. If those scripts were defining their completions conditionally based on have return value, they haven't been defining them at all in quite some time.

But even if we don't provide a workaround here in the upstream project, it would be appropriate to provide maintainers a recipe/instructions for doing that "cleanly" (for some values of it). A crude example would be an /etc/profile.d script that gets sourced after our bash-completion.sh there, and just does have() { return 0; }.

Some of those completion file names will also not satisfy the rules for what gets dynamically loaded. Need to change them to use the command's basename or basename.bash, see __load_completion for details (underscore filename prefix is reserved for our internal deprecation use). Suspects:

• dosage-completion
• freecontact.bash-completion
• linkchecker-completion
• patool.bash-completion
• porg_bash_completion

I wonder how would I remember to add a prominent note to our release notes for maintainers about this. Maybe we should just not sit on this for too long but just push a release with it out Soon(tm).

[scop]

(Keeping open for a bit even though #508 was merged as a reminder to work on filing reports about the packaging issues.)

[scop]

Another option for the documented maintainer workaround would be to do the on-the-fly have definition/undefinition, but even though cleaner in a way, that'd require them to patch it in instead of just dropping a separate file out there.

[scop]

@inconstante FYI the discussion above

[scop]

@siteshwar FYI too

[kevinoid]

Sounds good, thanks @scop! Good catch on the file names. I'll start cataloging those next. It looks like there are many others not included in the above list as well.

Another option for the documented maintainer workaround would be to do the on-the-fly have definition/undefinition

If we are going to recommend patching the completion scripts, is there a better alternative than defining have in each script? Could they use _have? If _have is private, could a new permanent/public alias be provided? We could recommend calling PATH=... type as _have does, but it seems likely to be a common source of error, and giving distributions an easy patch-point for all scripts is nice, in case they want to use a distro-specific search path.

After more thought: in many (most?) cases it may make sense to recommend scripts stop checking if the command exists. If the bash-completion script is installed in the same package as the executable, calling have is probably not very useful.

I wonder how would I remember to add a prominent note to our release notes for maintainers about this. Maybe we should just not sit on this for too long but just push a release with it out Soon(tm).

Both sound like good ideas to me. I might even consider defining have() { echo "${BASH_SOURCE[1] uses obsolete have(), use X instead. See https://..." >&2; return 1; } before . "$compfile" (and undef -f after) to make the brokenness easier for users to locate and understand, since nobody reads release notes. :laughing:

[scop]

If we are going to recommend patching the completion scripts, is there a better alternative than defining have in each script?

By patching I meant maintainers could patch our __load_completion to define/undefine have on the fly, not patch all individual scripts to define it -- if they go about patching individual scripts, they could just patch away the use of have instead.

If _have is private

The underscore prefix in our function names is not a marker for privateness, it's there just so that our functions won't clutter command name completions (well anywhere else besides when one wants to complete command names starting with an underscore). There's no public/private "API" to speak of in bash-completion. But yeah, they could use _have if they wanted.

After more thought: in many (most?) cases it may make sense to recommend scripts stop checking if the command exists. If the bash-completion script is installed in the same package as the executable, calling have is probably not very useful.

Exactly. And we don't need it in bash-completion either. I think the primary reason for why the world has been so slow to adopt bash-completion 2.x practices (released almost 9 years ago) and still a lot of things out there dump their files to the eagerly-loaded dir, is twofold: 1) bunch of packages that haven't been updated in a long time, and 2) macs having had bash 3.x as default, and upstreams wanting to support them out of the box (no "be sure to install bash 4.x, set it properly as your shell, and bash-completion@2 too"). I'm afraid the latter reason pretty much kills my interest in trying to get this recommendation done. Maybe in another decade when no macs with bash 3.x are supported any more...

I might even consider defining have() {...}

I might consider it too, let's let it cook for a bit ;)

[scop]

BTW I guess lintian could be extended to check some issues we've uncovered here. It already checks for shebangs in them and the use if /etc/bash_completion.d dir, but could do more. At least the completion file naming and no subdirs part should be easily checkable.

On the other hand, I suppose we could actually look inside subdirs of completions/ for scripts. But not inclined to add that complexity unless it solves a real world problem. Transitioning files included here to ones in packages could be one; we do that currently by using the underscore prefix whenever we become aware of something upstreamed. But that way we lag chronically behind, there are file conflicts to solve in the meantime -- would be better to give packages a way to load something before our files, for example by placing them in a subdir...

[scop]

(Recent example of transitioning to upstream is in 4d8e152039311e2bebf3d3ecb875e570b3171865)

[kevinoid]

That all makes sense and sounds good to me. Thanks for the details!

I think the primary reason for why the world has been so slow to adopt bash-completion 2.x practices [...]

I agree with both the reasons you listed and would add one more: 3) An outside contributor submitted a bash-completion script for a program that the developer or maintainer accepted, but doesn't deeply understand or own. (I may be a guilty of this. :grin:)

BTW I guess lintian could be extended to check some issues we've uncovered here.

I had the same thought! I've been keeping a list of issues I think Lintian could warn about, which is starting to look a bit daunting. (Especially because I've never written a Lintian rule.)

On the other hand, I suppose we could actually look inside subdirs of completions/ for scripts. But not inclined to add that complexity unless it solves a real world problem.

Agreed. I don't think we've found any real uses yet. All the subdirs I found in https://github.com/scop/bash-completion/issues/506#issuecomment-796302817 were obvious packaging mistakes (typos, misunderstanding of how dh_bash-completion works, etc.). But I'll keep an eye out for interesting cases that may be worth supporting.

[akinomyoga]

Maybe this issue can be closed now (as mentioned in #530 for the 2.12 release)?

[scop]

Agreed.