scop / bash-completion

Programmable completion functions for bash
GNU General Public License v2.0
2.8k stars 376 forks source link

pkgutil: don't install on platforms where it cannot be used #1210

Closed eli-schwartz closed 3 weeks ago

eli-schwartz commented 4 weeks ago

In commit 0e3a17df7eac79ff386891945a951a23134373b0, a short-circuit was added that causes the completion to return 1 if the host OS is not solaris.

On Gentoo, this triggers a warning:

 * QA Notice: Problems with installed bash completions were found:
 *
 *  pkgutil: does not define any completions (failed to source?).
 *
 * For more details on installing bash-completions, please see:
 * https://wiki.gentoo.org/wiki/Bash/Installing_completion_files

The purpose of the check is to find broken completions that need to be fixed by their respective upstreams, and in this case the completion is "intentionally broken". It seems straightforward to respect that at an earlier level, which both avoids the QA check and ensures that unneeded files don't get installed on any distro.

And... it also means that macOS users can ship their own pkgutil completion without worrying about file clashes with the bash-completion project. The entire reason for the short-circuit was because there's a different pkgutil there.

Fixes: 0e3a17df7eac79ff386891945a951a23134373b0

akinomyoga commented 3 weeks ago

Would it be possible to have a configure option to generate a host-independent version, which includes all completions and is ready to use in any operating system just by sourcing bash_completion*? It doesn't need to be done within this PR, but I think it should be useful to leave a way to generate the installation that includes everything as we've been doing until now.

*: I think /etc/profile.d/bash_completion.sh wouldn't work because it contains a hardcoded path to the install location, but still we can directly source the file bash_completion.

eli-schwartz commented 3 weeks ago

You mean it includes all completions specific to FreeBSD, in addition to all completions specific to Solaris, in addition to all completions specific to Linux?

I am not certain what scenario that would be used on, since I'm unaware of any package format that both supports all these operating systems and shares a binary cache between them all. And I doubt that it's being installed to a USB drive and run on multiple operating systems. Unlike e.g. Windows' PortableApps ecosystem, you would end up with too much software that has to be installed multiple times anyway, once per OS.

akinomyoga commented 3 weeks ago

You mean it includes all completions specific to FreeBSD, in addition to all completions specific to Solaris, in addition to all completions specific to Linux?

Yes.

I am not certain what scenario that would be used on, since I'm unaware of any package format that both supports all these operating systems and shares a binary cache between them all.

That's not related to the case I assume.

And I doubt that it's being installed to a USB drive and run on multiple operating systems. [...]

This is also irrelevant.

I'm not talking about packages for distributions or a USB drive including binary files. bash-completion is essentially a set of shell scripts and doesn't include any binary files, so it is essentially possible to just copy the files to another host and source bash_completion from e.g. ~/.bashrc.

For example, one would want to do this when one uses a shared system, where users don't have an administrative privilege or users may want to use specific versions of shell settings without affecting other users. You might say one should rebuild bash-completion for each host, but it wasn't needed so far. One could install bash-completion to somewhere once with --prefix="$HOME/tmp" and could copy the results. I don't see the necessity of requiring extra steps to generate bash-completion for each host the user logs in.

In fact, there was no need to rebuild bash-completion for each operating system. We cannot say it is really necessary to prepare a copy of bash-completion for each operating system. I agree that It is cleaner for the packages for specific operating systems to exclude unnecessary completions. However, I don't think the current way of installing everything causes a real problem. In case a distribution wants to override the completions prepared by bash-completion (such as pkgutil in macOS mentioned in the original explanation of the PR), we can prefix _ to lower the precedence (e.g. rename pkgutil to _pkgutil).

akinomyoga commented 3 weeks ago

The CI is failing. Could you modify the format of the commit message (as described in the following item I quoted from CONTRIBUTING.md)?

https://github.com/scop/bash-completion/blob/8304d3342a07e0f1a402ff0a6752e513c2a0541a/CONTRIBUTING.md?plain=1#L166-L177

eli-schwartz commented 3 weeks ago

The CI is failing. Could you modify the format of the commit message (as described in the following item I quoted from CONTRIBUTING.md)?

I'm unfamiliar with this "convention" -- as far as I'm aware it's not actually the conventional way to do anything, it seems to violate a number of best practices. That being said, a bit of digging around tells me this style was invented by the JavaScript world which... explains a whole lot about JavaScript.

The explanation provided for how it is necessary to generate changelogs makes sense to me -- I had been wondering why the changelog doesn't effectively describe user-facing changes but dives into the weeds of developer commit messages. Personally, I would suggest migrating to Towncrier

Unfortunately I lack the bandwidth at the moment to figure out how this unusual commit message style works or how to integrate it into the intended semantics of a changelog. And since if I understand correctly this is a merge blocker, that also means I don't have the bandwidth to implement the rest of your feedback. Unfortunately I can't commit to much at the moment, other than to keep this review in mind before opening further PRs to enhance bash-completion.

eli-schwartz commented 3 weeks ago

Suggestion for the future: currently CONTRIBUTING.md is very hard to read because there's a lot of disorganized information leading to cognitive overload in people that just want to submit a PR to existing code.

akinomyoga commented 3 weeks ago

Then, can I take over the PR?

scop commented 2 weeks ago

To me, following projects' conventions is a basic thing to do when contributing to them, no matter if I agree with them or not. There's no point in arguing "how" conventional or to whom the commit message style this project want is or its subjective merits in this PR: it is what this project uses, and it uses tooling that does useful things when the style is properly followed, and there's documentation about it. (I do think the name of the commit message style is somewhat unfortunate though, but :shrug:)

CONTRIBUTING.md is hard to read, agreed. Some structuring would help, also with ability to link to relevant sections in it. README.md's FAQ has the same problem (at least with linkability). PR's highly appreciated!

https://github.com/scop/bash-completion/pull/1214 resurrects what was in this PR, with the commit message formatting issues addressed. I intend to merge it after CI passes.

eli-schwartz commented 2 weeks ago

To me, following projects' conventions is a basic thing to do when contributing to them, no matter if I agree with them or not.

Not exactly. It's a basic thing to do when you intend to get your stuff merged. Sometimes you realize you lack the resources to follow said conventions, in which case you change your mind about contributing, or ideally choose to delay contributing until you can get up to speed on the conventions. In both cases you are no longer in a state of currently contributing to the project, which means that the question of following the conventions falls off the map (but may become relevant later on).

There's no point in arguing "how" conventional or to whom the commit message style this project want is or its subjective merits in this PR: it is what this project uses, and it uses tooling that does useful things when the style is properly followed, and there's documentation about it.

There is certainly a point. It's a topic that came up as a side point, as a result of the PR. Its value as a subjective critique is equally good (or equally bad!) regardless of whether it appears as a comment on another PR, or as a distinct ticket. And it came up organically. :)

eli-schwartz commented 2 weeks ago

Thanks, both, for tweaking the PR into mergeable condition. :)

@akinomyoga, apologies for neglecting to give the "go-ahead" earlier.

scop commented 2 weeks ago

I'll refrain from further arguing the semantic and subjective differences here, just speaking for myself that the likelihood of something persisting as a TODO item from a closed PR having a different topic than the one at hand for this project is much, much lower than if it was a "proper" discussion or a dedicated issue.