Makefile: add "shell-completion" target

[thaJeztah]

• relates too [master] deb,rpm: use cobra generated completions docker-ce-packaging#1150

Makefile: add "shell-completion" target

Add a target to build the (cobra) generated completion and store
them inside build/completions.

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.42%. Comparing base (17c5fe6) to head (6ab9b92).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5770   +/-   ##
=======================================
  Coverage   59.42%   59.42%           
=======================================
  Files         347      347           
  Lines       29402    29402           
=======================================
  Hits        17472    17472           
  Misses      10958    10958           
  Partials      972      972           

[laurazard]

What's reason for this change instead of doing the build in completion still (since it still depends on binary? For docker/docker-ce-packaging#1150, couldn't we run the completion target during the build too?

Overall LGTM :')

[thaJeztah]

Yeah I was going back and forth a bit; so for docker/docker-ce-packaging#1150 we would need the "build, but don't install" bit, and the make completion target was there to do both.

The alternative I was considering was to change make completion only do the building, but not the installing (which I think would still be OK, as it was really for testing, and not widely communicated).

Either that, or a make generate-completion our make build-completion, but at that moment I started to consider; is there any reason to not just always (re)build these when building (dyn)binary? 😂

Happy too hear your thoughts! I opened it as draft because I was still making my mind up 😅

[thaJeztah]

Rabbit hole time!! I noticed here we're using /etc/bash_completion.d/, but that our deb packages use /usr/share/bash-completion/completions/. StackOverflow had a nice summary; https://unix.stackexchange.com/a/605051/78656, but also linked to the docs;

In bash-completion >= 2.12, we search the data directory of bash-completion under the installation prefix where the target command is installed. When one can assume that the version of the target bash-completion is 2.12 or higher, the completion script can actually be installed to $PREFIX/share/bash-completion/completions/ under the same installation prefix as the target program installed under $PREFIX/bin/ or $PREFIX/sbin/. For the detailed search order, see also "Q. What is the search order for the completion file of each target command?" below.

But here's the fun bits; the new locations were "planning to use this in 2.12 and up" scop/bash-completion@530017d

But Debian 12 (bookworm);

Operating System: Debian GNU/Linux 12 (bookworm)

Has;

apt-cache madison bash-completion
bash-completion |   1:2.11-6 | mirror+file:/etc/apt/mirrors/debian.list bookworm/main amd64 Packages
bash-completion |   1:2.11-6 | mirror+file:/etc/apt/mirrors/debian.list bookworm/main Sources

But the deb packages (through dh_bash-completion), install in the "new" location (recommended for 2.12 and up);

ls -l /usr/share/bash-completion/completions/docker
-rw-r--r-- 1 root root 114566 Jan 22 13:39 /usr/share/bash-completion/completions/docker

...not the old location;

ls -l /etc/bash_completion.d/
total 4
-rw-r--r-- 1 root root 439 Jun 16  2024 git-prompt

[thaJeztah]

So for these we could consider this make target to either be;

• Install where the deb/rpm packages would install them
• User-override (BASH_COMPLETION_USER_DIR or XDG_DATA_DIRS) to prevent overwriting existing ones installed from a deb/rpm package.

Q. What is the search order for the completion file of each target command?

A. The completion files of commands are looked up by the shell function
__load_completion. Here, the search order in bash-completion >= 2.12 is
explained.

1. BASH_COMPLETION_USER_DIR. The subdirectory completions of each paths
   in BASH_COMPLETION_USER_DIR separated by colons is considered for a
   completion directory.
2. The location of the main bash_completion file. The subdirectory
   completions in the same directory as bash_completion is considered.
3. The location of the target command. When the real location of the command
   is in the directory <prefix>/bin or <prefix>/sbin, the directory
   <prefix>/share/bash-completion/completions is considered.
4. XDG_DATA_DIRS (or the system directories /usr/local/share:/usr/share
   if empty). The subdirectory bash-completion/completions of each paths
   in XDG_DATA_DIRS separated by colons is considered.

The completion files of the name <cmd> or <cmd>.bash, where <cmd> is
the name of the target command, are searched in the above completion
directories in order. The file that is found first is used. When no
completion file is found in any completion directories in this process, the
completion files of the name _<cmd> is next searched in the completion
directories in order.

[tianon]

/usr is 100% arguably the "correct" location for these (per FHS), and supported since scop/bash-completion@c89dcbb, which is bash-completion version 2.2 (hence why Debian uses it).

[thaJeztah]

THANK YOU! I overlooked that it was just moving existing logic, and probably just the docs missing here.

I'll update, and add a note "for future self" to consider using user-specific override paths, but that's a whole other can of worms (XDG, other shells ...)

[thaJeztah]

@laurazard @tianon updated; I pushed an extra commit to split the "generating" from the "(dyn)binary" targets. I can squash the commits if this approach looks better to you.

[thaJeztah]

Slightly hacky here; the make binary and make dynbinary targets overwrite the build/binary file; I'm trying to avoid cases where either a Dynamic or a Static binary was built, and generating the completion-scripts now replacing the binary.

[vvoland]

How about something like:

Suggested change

	.PHONY: shell-completion
	shell-completion: ## generate shell-completion scripts
	# requires either "binary" or "dynbinary" to be built.
	# but only build if it's not there, to prevent existing binaries
	# from being replaced.
	@ [ -f build/docker ] || $(MAKE) binary
	@ ./scripts/build/completion
	build/docker:
	echo "Run 'make binary' or 'make dynbinary' first" && exit 1
	.PHONY: shell-completion
	shell-completion: build/docker ## generate shell-completion scripts
	# requires either "binary" or "dynbinary" to be built.
	# but only build if it's not there, to prevent existing binaries
	# from being replaced.
	@ ./scripts/build/completion

[thaJeztah]

Oh! Good suggestion; I think I tried that but something failed for some reason; let me actually try that again, because it was late, and maybe I tried the wrong thing.

[thaJeztah]

I guess we should also add some check for macOS, which has a different (BSD flavor) install, which uses a lowercase -d;

./scripts/build/shell-completion: line 17: ./build/docker: Permission denied
install: illegal option -- D
usage: install [-bCcpSsv] [-B suffix] [-f flags] [-g group] [-m mode]
               [-o owner] file1 file2
       install [-bCcpSsv] [-B suffix] [-f flags] [-g group] [-m mode]
               [-o owner] file1 ... fileN directory
       install -d [-v] [-g group] [-m mode] [-o owner] directory ...

     The options are as follows:

     -B suffix
             Use suffix as the backup suffix if -b is given.

     -b      Back up any existing files before overwriting them by renaming them to file.old.  See -B for specifying a different backup suffix.

     -C      Copy the file.  If the target file already exists and the files are the same, then don't change the modification time of the target.

     -c      Copy the file.  This is actually the default.  The -c option is only included for backwards compatibility.

     -d      Create directories.  Missing parent directories are created as required.

That's an existing issue though, and the make completion was intended for the dev-container, so let's not bother for now.

[thaJeztah]

@vvoland I took a variant of your suggestion; PTAL

[Benehiko]

LGTM