Supressing output from completion script #506
Comments
We follow the message recent bash would output when sourcing a dir without letting bash to do it; doing so avoids fd leaks on some older but still supported bash versions. Other than that, let all output from sourcing the file pass through to ease debugging. Closes #506
|
That's a fair question. I think we should not mute everything, #508 has a candidate fix. Thoughts? |
|
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
I assume all of these are buggy and will investigate and file bugs shortly. |
|
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: to
I'm guessing these scripts were written/tested with eager sourcing (e.g. in 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 |
|
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 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 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
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). |
We follow the message recent bash would output when sourcing a dir without letting bash to do it; doing so avoids fd leaks on some older but still supported bash versions. Other than that, let all output from sourcing the file pass through to ease debugging. Refs #506
We follow the message recent bash would output when sourcing a dir without letting bash to do it; doing so avoids fd leaks on some older but still supported bash versions. Other than that, let all output from sourcing the file pass through to ease debugging. Refs #506
|
(Keeping open for a bit even though #508 was merged as a reminder to work on filing reports about the packaging issues.) |
|
Another option for the documented maintainer workaround would be to do the on-the-fly |
|
@inconstante FYI the discussion above |
|
@siteshwar FYI too |
|
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.
If we are going to recommend patching the completion scripts, is there a better alternative than defining 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
Both sound like good ideas to me. I might even consider defining |
By patching I meant maintainers could patch our
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
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 consider it too, let's let it cook for a bit ;) |
|
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 |
|
(Recent example of transitioning to upstream is in 4d8e152) |
|
That all makes sense and sounds good to me. Thanks for the details!
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. 😁)
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.)
Agreed. I don't think we've found any real uses yet. All the subdirs I found in #506 (comment) were obvious packaging mistakes (typos, misunderstanding of how |
|
Maybe this issue can be closed now (as mentioned in #530 for the 2.12 release)? |
|
Agreed. |
While tracking down a syntax error in a bash completion script, I noticed that
stdoutandstderrare redirected to/dev/nullfor all completion scripts. It looks like this has been present since 20c05b4, 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
The text was updated successfully, but these errors were encountered: