[BUG] debian packages: bash completion script is installed to wrong place

[dseomn]

Description salt-common from https://repo.saltproject.io/salt/py3/debian/12/amd64/latest includes /usr/share/bash-completions/completions/salt-common.bash/salt.bash, which doesn't seem to enable bash completion. From a glance at other bash completion scripts on my system, I think it should probably go in /usr/share/bash-completion/completions (no s in bash-completion) instead, and it looks like the filenames are based on the command names. So maybe it should be installed as /usr/share/bash-completion/completions/salt with symlinks salt-key, salt-call, and salt-cp pointing at that?

Setup (Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

• [x] on-prem machine
• [ ] VM (Virtualbox, KVM, etc. please specify)
• [ ] VM running on a cloud service, please be explicit and add details
• [ ] container (Kubernetes, Docker, containerd, etc. please specify)
• [ ] or a combination, please be explicit
• [ ] jails if it is FreeBSD
• [ ] classic packaging
• [x] onedir packaging
• [ ] used bootstrap to install

Steps to Reproduce the behavior

1. Install salt-common from Salt's Debian repos.
2. Use bash.
3. Try to use any of the tab completions in /usr/share/bash-completions/completions/salt-common.bash/salt.bash

Expected behavior The tab completions in that file should work.

Screenshots N/A

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ```yaml Salt Version: Salt: 3007.0 Python Version: Python: 3.10.13 (main, Feb 19 2024, 03:31:20) [GCC 11.2.0] Dependency Versions: cffi: 1.16.0 cherrypy: unknown dateutil: 2.8.2 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 3.1.3 libgit2: Not Installed looseversion: 1.3.0 M2Crypto: Not Installed Mako: Not Installed msgpack: 1.0.7 msgpack-pure: Not Installed mysql-python: Not Installed packaging: 23.1 pycparser: 2.21 pycrypto: Not Installed pycryptodome: 3.19.1 pygit2: Not Installed python-gnupg: 0.5.2 PyYAML: 6.0.1 PyZMQ: 25.1.2 relenv: 0.15.1 smmap: Not Installed timelib: 0.3.0 Tornado: 6.3.3 ZMQ: 4.3.4 Salt Package Information: Package Type: onedir System Versions: dist: debian n/a trixie locale: utf-8 machine: x86_64 release: 6.7.12-amd64 system: Linux version: Debian GNU/Linux n/a trixie ```

Additional context N/A

[smarsching]

I agree that the install path is wrong. I do not think that it is necessary to create symlinks. Simply installing the file as /usr/share/bash-completion/completions/salt should be sufficient. Completion for commands like salt-key, etc. should work without any additional symlinks.

Instead of simply fixing the path in pkg/debian/salt-common.install, using dh_bash-completion might be considered. Using this helper script should ensure that the file is always installed in the correct location. In order to do, the line

pkg/common/salt.bash /usr/share/bash-completions/completions/salt-common.bash

should be removed from pkg/debian/salt-common.install and a new file pkg/debian/salt-common.bash-completion with the following content should be added:

pkg/common/salt.bash salt

Shall I open a PR for this change?

[dmurphy18]

Given this area of bash completion has not changed in almost a decade, wondering what Debian's own forked packaged version of Salt does for completions ?. Will take a look once I have a VM up and running

[dseomn]

I do not think that it is necessary to create symlinks. Simply installing the file as /usr/share/bash-completion/completions/salt should be sufficient. Completion for commands like salt-key, etc. should work without any additional symlinks.

Wouldn't that only work after using tab completion with the salt command once to load the file? I never use the salt command so I don't think it would work for me.

Debian's own forked packaged version

That's gone: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1069654

[smarsching]

@dseomn You are right, in order for the completions to be loaded when one of the other commands is used, the symbolic links must exist.

@dmurphy18 I checked the Salt Ubuntu package (for Ubuntu 22.04, because the package has been removed in more recent versions), and that package uses dh_bash-completion for installing the completions. Ironically, it does so in a way that means that they still are not going to work, because the file is installed as salt-common without any symbolic links, so it is never going to be used.

It seems like there aren’t a lot of packages that add symlinks for bash completion, and within this set of packages, there seems to be no consistent rule about whether they use dh_bash-completion (for example, the grub package does) or not (the debconf package does not). In fact, dh_bash-completion cannot be used to create the symlinks, so I guess it’s not that useful.

Therefore, I suggest not uing dh_bash-completion and just fixing the path in salt-common.install and adding the necessary symlinks to salt-common.links instead (mirroring what the debconf package does).

[smarsching]

I have created #66821 with the fixes that should be sufficient in my opinion. I am not sure whether the CI pipeline builds Debian packages. If it does, I can test these packages when the pipeline has completed.

[smarsching]

@dmurphy18 I ended up using dh_bash-completion after all: This was needed, because the original file has the name salt.bash, but it is supposed to be installed with the name salt and this cannot be achived through the regular installation mechanism, while dh_bash-completion can handle this.

I tested that bash completion is working correctly when installing the packages that were build by the CI for #66821.

[dmurphy18]

@smarsching Might be a conflict renaming salt.bash to salt, given salt is a command for the salt-master, salt test.ping Looking at your PR, and given this is unchanged for a decade, wondering why no one else has reported this problem in that time.

[smarsching]

@dmurphy18 I don’t think that this causes a conflict: The salt-master package does not install a bash-completion script. We could also install it as _salt-common and create a symlink from salt to _salt-common, but I don’t see any advantage in this.

Regarding the question why it has not been reported before: My best guess is that either most people using the Debian packages simply do not expect bash-completion to work for the Salt commands, or that they enabled it manually, by sourcing /usr/share/bash-completions/completions/salt-common.bash/salt.bash in their .bashrc or copying the file to /etc/bash_completion.d.

In fact, I haven been using Salt for about 10 years without bash-completion, but recently I thought: “Hey, let’s checkout whether somone wrote a bash-completion script for Salt”, and when I found that it is part of the Salt distribution, I wondered why it is not distributed with the Debian packages, until I found out that it actually is distributed with the packages, which made me investigate why it did not work, which is how I found this bug report.

[dmurphy18]

@smarsching Commented on the PR. I am talking about conflict between salt.bash -> salt and the salt from salt-master, users getting confused as to why things don't work, due to various paths used in their PATH. Don't name two things the same which have different functionality, perhaps why it is named salt.bash to begin with (inherited that from the original Debian maintainer 10 years ago, when I started packaging Salt back then).

[smarsching]

@dmurphy18 /usr/share/bash-completion/completions should never be in a user’s PATH, and the file having the same name as the command is the point. That is how bash-completion finds the completion:

When you enter <command> and hit tab, bash-completion looks for the file /usr/share/bash-completion/completions/<command> and loads it. This is the difference between files in /etc/bash_completion.d and /usr/share/bash-completion/completions. Files in /etc/bash_completion.d are loaded when Bash starts, but the other files are only loaded on demand. I guess that the idea behind this mechanism is to make Bash start more quickly and save resources, avoiding the loading of completions that the user is never gonna use within the session.

[dmurphy18]

@smarsching Sorry but it is in the name, Murphy's Law applies and if there is a way for a user to mess things up, it will happen :rofl: , hence don't give the same name to two different things. Best practice is not to give the user a chance to mess up.

[smarsching]

@dmurphy18 I checked the bash-completion source code, it seems like /usr/share/bash-completion/completions/<command>.bash should also work. It is checked after looking for /usr/share/bash-completion/completions/<command>, but it is used if the first one does not exist.

So, do you prefer having the .bash extension? As far as I can tell, no other Debian / Ubuntu package (at least from the set of packages that I have installed) does it this way. They all skip the .bash suffix, so it would go against the general style used by Debian / Ubuntu systems to add this suffix, but technically it is possible.

[smarsching]

@dmurphy18 I created a new PR, #66827, against the 3006.x branch (I opened the original PR against the master branch, because I did not know whether the workflow is that bugfixes a back-ported from master or forward-ported from the stable branches to master).

This PR now includes a test for the fix. If you prefer adding the files with the .bash extension (in my previous comment, I explained why I am in favor of not using this extension for the installed files), let me know. That’s a simple change.

[dmurphy18]

So trying the Debian / Ubuntu packages from the Debian fork of salt, found the following for apt install salt-minion Debian 10 (last Debian Salt release I could find):

root@tdebian10:/home/david# l /usr/share/bash-completion/completions/salt*
-rw-r--r-- 1 root root 10K Nov 17  2021 /usr/share/bash-completion/completions/salt-common
root@tdebian10:/home/david# apt-cache policy salt-minion
salt-minion:
  Installed: 2018.3.4+dfsg1-6+deb10u3
  Candidate: 2018.3.4+dfsg1-6+deb10u3
  Version table:
 *** 2018.3.4+dfsg1-6+deb10u3 500
        500 http://deb.debian.org/debian buster/main amd64 Packages
        500 http://security.debian.org/debian-security buster/updates/main amd64 Packages
        100 /var/lib/dpkg/status
root@tdebian10:/home/david# 

Ubuntu have been more active in maintaining their fork of Salt: Ubuntu 22.04 LTS:

root@david-ubuntu2204:/home/david# l /usr/share/bash-completion/completions/sa*
-rw-r--r-- 1 root root 10K Apr 16  2022 /usr/share/bash-completion/completions/salt-common
root@david-ubuntu2204:/home/david# apt-cache policy salt-minion
salt-minion:
  Installed: 3004.1+dfsg-2
  Candidate: 3004.1+dfsg-2
  Version table:
 *** 3004.1+dfsg-2 500
        500 http://ubuntu.cs.utah.edu/ubuntu jammy/universe amd64 Packages
        500 http://ubuntu.cs.utah.edu/ubuntu jammy/universe i386 Packages
        100 /var/lib/dpkg/status
root@david-ubuntu2204:/home/david#

Debian 12 using Salt Projects latest LTS 2006.9

root@tdebian12:/home/david# l /usr/share/bash-completion/completions/sa*
ls: cannot access '/usr/share/bash-completion/completions/sa*': No such file or directory
root@tdebian12:/home/david# l /usr/share/bash-completions/
total 20K
drwxr-xr-x   3 root root 4.0K Aug 23 17:37 completions
drwxr-xr-x 256 root root  12K Aug 23 17:37 ..
drwxr-xr-x   3 root root 4.0K Aug 23 17:37 .
root@tdebian12:/home/david# l /usr/share/bash-completions/completions/sa*
total 20K
-rw-r--r-- 1 root root  12K Jul 29 01:42 salt.bash
drwxr-xr-x 3 root root 4.0K Aug 23 17:37 ..
drwxr-xr-x 2 root root 4.0K Aug 23 17:37 .
root@tdebian12:/home/david# apt-cache policy salt-minion
salt-minion:
  Installed: 3006.9
  Candidate: 3006.9
  Version table:
 *** 3006.9 500
        500 https://repo.saltproject.io/salt/py3/debian/12/amd64/3006 bookworm/main amd64 Packages
        100 /var/lib/dpkg/status
root@tdebian12:/home/david# 

So agree the need to correct to follow that as in the Debian / Ubuntu fork of Salt and that the directory should be /usr/share/bash-completion/completions, but the file there should be salt-common and not salt.bash.

Thanks for catching this, been this way for almost a decade and you are the first to notice it.

[dmurphy18]

Actually correction for Debian 12 and Salt Project 3006.9

root@tdebian12:/home/david# l /usr/share/bash-completions/completions/
total 12K
drwxr-xr-x 3 root root 4.0K Aug 23 17:37 ..
drwxr-xr-x 3 root root 4.0K Aug 23 17:37 .
drwxr-xr-x 2 root root 4.0K Aug 23 17:37 salt-common.bash
root@tdebian12:/home/david# l /usr/share/bash-completions/completions/salt-common.bash/
total 20K
-rw-r--r-- 1 root root  12K Jul 29 01:42 salt.bash
drwxr-xr-x 3 root root 4.0K Aug 23 17:37 ..
drwxr-xr-x 2 root root 4.0K Aug 23 17:37 .
root@tdebian12:/home/david# 

that salt-common.bash/salt.bash should become salt-common

Note: the salt-common used in Debian 10 and Ubuntu 22.04 is the same file, that is, this file has shown no change between Debian 10 and Ubuntu 22.04

There are differences between the Debian fork salt-common and Salt Project salt.bash, but not too much difference in functionality, and salt-common.bash/salt.bash could probable be named salt-common and in the completions directory, and remove directory salt-common.bash

Would be interested in your thoughts @dseomn

[dmurphy18]

Installed Salt 2004.1 Classic packaging on Ubuntu 20.04 and found the following:

root@tu2004:/home/david# l /usr/share/bash-completion/completions/sa*
-rw-r--r-- 1 root root 10K Dec 20  2017 /usr/share/bash-completion/completions/salt-common
root@tu2004:/home/david# apt-cache policy salt-minion
salt-minion:
  Installed: 3004.1+ds-1
  Candidate: 3004.1+ds-1
  Version table:
 *** 3004.1+ds-1 500
        500 https://repo.saltproject.io/py3/ubuntu/20.04/amd64/archive/3004.1 focal/main amd64 Packages
        100 /var/lib/dpkg/status
root@tu2004:/home/david#

So the Salt Project used to have it correct, similar to the Debian fork of Salt, but it must have gotten messed up with the transition to onedir architecture (or possibly Tiamat). This is definitely a regression, and salt-common is the correct form.

Furthermore the version of salt-common shipped in classic packaged 3004.1 is identical to that supplied in the Debian fork. I will track down why changes were made, unfortunately the culprits are since departed so a full explanation for the changes may not be forthcoming.

[smarsching]

@dmurphy18 Thank you for your efforts! The fix in your PR looks correct to me, I only think that a few symlinks are still missing. I left a detailed comment in #66852.

[dmurphy18]

Closing since https://github.com/saltstack/salt/pull/66852 is merged