wrappers: symlink completion snippets when symlinking binaries (#3724)

This also modifies complete.sh to support this, as well as some minor changes to dirs and snap to accommodate. Symlinking the snippets is itself simple enough that I didn't think it was worth creating an ad-hoc helper, so it's just a few lines glommed onto the end of the existing wrapper funcs (and tests). To be clear, I did write the helper, but between it being very few lines of code, it needing to return enough information to the caller to clean up behind it if something else went wrong, and especially as the other user of that helper would be the aliases and they don't have access to the AppInfo (which would be the obvious thing to pass to such helpers), I decided against it. How aliases are going to play into completion is still an open question on the implementation side, which I need to look into soon, but this is not that work.

11 files changed, 177 insertions, 20 deletions

diff --git a/data/completion/complete.sh b/data/completion/complete.sh
index 6cbad3ecc1..a8934293b4 100644
--- a/data/completion/complete.sh
+++ b/data/completion/complete.sh
@@ -36,6 +36,7 @@
 # from 'etelpmoc.sh', validates the results and puts the validated results
 # into the bash completion environment variables
 # 8. bash displays the results to the user
+type -t _complete_from_snap > /dev/null ||
 _complete_from_snap() {
 {
 # De-serialize the output of 'snap run --command=complete ...' into the format
@@ -100,17 +101,29 @@ _complete_from_snap() {
 
 }
 
-# _complete_from_snap_maybe calls _complete_from_snap if the command is in
-# bin/snap, and otherwise does bash-completion's _completion_loader (which is
-# what -D would've done before).
-_complete_from_snap_maybe() {
- # catch /snap/bin and /var/lib/snapd/snap/bin
- if [[ "$(which "$1")" =~ /snap/bin/ && ( -e /var/lib/snapd/snap/core/current/usr/lib/snapd/etelpmoc.sh || -e /snap/core/current/usr/lib/snapd/etelpmoc.sh ) ]]; then
- _complete_from_snap "$1"
- return $?
- fi
- # fallback to the old -D
- _completion_loader "$1"
-}
+# this file can be sourced directly as e.g. /usr/lib/snapd/complete.sh, or via
+# a symlink from /usr/share/bash-completion/completions/. In the first case we
+# want to load the default loader; in the second, the specific one.
+#
+if [[ "${BASH_SOURCE[0]}" =~ ^/usr/share/bash-completion/completions/ ]]; then
+ complete -F _complete_from_snap "$1"
+else
+
+ # _complete_from_snap_maybe calls _complete_from_snap if the command is in
+ # bin/snap, and otherwise does bash-completion's _completion_loader (which is
+ # what -D would've done before).
+ type -t _complete_from_snap_maybe > /dev/null ||
+ _complete_from_snap_maybe() {
+ local etel=snap/core/current/usr/lib/snapd/etelpmoc.sh
+ # catch /snap/bin and /var/lib/snapd/snap/bin
+ if [[ "$(which "$1")" =~ /snap/bin/ && ( -e "/var/lib/snapd/$etel" || -e "/$etel" ) ]]; then
+ complete -F _complete_from_snap "$1"
+ return 124
+ fi
+ # fallback to the old -D
+ _completion_loader "$1"
+ }
+
+ complete -D -F _complete_from_snap_maybe
+fi
 
-complete -D -F _complete_from_snap_maybe
diff --git a/data/completion/snap b/data/completion/snap
index e29f7f87f1..7b7c292aba 100644
--- a/data/completion/snap
+++ b/data/completion/snap
@@ -14,7 +14,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program. If not, see <http://www.gnu.org/licenses/>.
 
-_complete() {
+_complete_snap() {
 # TODO: add support for sourcing this function from the core snap.
 local cur prev words cword
 _init_completion -n : || return
@@ -62,4 +62,4 @@ _complete() {
 return 0
 }
 
-complete -F _complete snap
+complete -F _complete_snap snap
diff --git a/dirs/dirs.go b/dirs/dirs.go
index dd996d09a6..0fe981eded 100644
--- a/dirs/dirs.go
+++ b/dirs/dirs.go
@@ -80,6 +80,8 @@ var (
	XdgRuntimeDirGlob string
 
	CompletionHelper string
+	CompletersDir string
+	CompleteSh string
 )
 
 const (
@@ -193,4 +195,6 @@ func SetRootDir(rootdir string) {
	XdgRuntimeDirGlob = filepath.Join(rootdir, XdgRuntimeDirBase, "*/")
 
	CompletionHelper = filepath.Join(CoreLibExecDir, "etelpmoc.sh")
+	CompletersDir = filepath.Join(rootdir, "/usr/share/bash-completion/completions/")
+	CompleteSh = filepath.Join(SnapMountDir, "core/current/usr/lib/snapd/complete.sh")
 }
diff --git a/snap/info.go b/snap/info.go
index 0a33d21820..57c041b371 100644
--- a/snap/info.go
+++ b/snap/info.go
@@ -474,6 +474,18 @@ func (app *AppInfo) WrapperPath() string {
	return filepath.Join(dirs.SnapBinariesDir, binName)
 }
 
+// CompleterPath returns the path to the completer snippet for the app binary.
+func (app *AppInfo) CompleterPath() string {
+	var binName string
+	if app.Name == app.Snap.Name() {
+	binName = filepath.Base(app.Name)
+	} else {
+	binName = fmt.Sprintf("%s.%s", app.Snap.Name(), filepath.Base(app.Name))
+	}
+
+	return filepath.Join(dirs.CompletersDir, binName)
+}
+
 func (app *AppInfo) launcherCommand(command string) string {
	if command != "" {
	command = " " + command
diff --git a/snap/info_test.go b/snap/info_test.go
index 8d4761b81c..81769b9d8a 100644
--- a/snap/info_test.go
+++ b/snap/info_test.go
@@ -110,6 +110,18 @@ apps:
	c.Check(info.Apps["foo"].WrapperPath(), Equals, filepath.Join(dirs.SnapBinariesDir, "foo"))
 }
 
+func (s *infoSuite) TestAppInfoCompleterPath(c *C) {
+	info, err := snap.InfoFromSnapYaml([]byte(`name: foo
+apps:
+ foo:
+ bar:
+`))
+	c.Assert(err, IsNil)
+
+	c.Check(info.Apps["bar"].CompleterPath(), Equals, filepath.Join(dirs.CompletersDir, "foo.bar"))
+	c.Check(info.Apps["foo"].CompleterPath(), Equals, filepath.Join(dirs.CompletersDir, "foo"))
+}
+
 func (s *infoSuite) TestAppInfoLauncherCommand(c *C) {
	dirs.SetRootDir("")
 
diff --git a/tests/completion/indirect/task.exp b/tests/completion/indirect/task.exp
index b71d663709..df587d63ad 100644
--- a/tests/completion/indirect/task.exp
+++ b/tests/completion/indirect/task.exp
@@ -3,6 +3,7 @@ send "source $::env(SPREAD_PATH)/$::env(SPREAD_SUITE)/$::env(SPREAD_VARIANT).sh\
 next
 send "source /usr/share/bash-completion/bash_completion\n"
 next
+# because this sources complete.sh, it won't be using the snippets
 send "source $::env(SPREAD_PATH)/data/completion/complete.sh\n"
 next
 chat "complexion \t\t" $::env(_OUT0)
diff --git a/tests/completion/indirect/task.yaml b/tests/completion/indirect/task.yaml
index ac73a79530..8cc7d9180b 100644
--- a/tests/completion/indirect/task.yaml
+++ b/tests/completion/indirect/task.yaml
@@ -6,6 +6,9 @@ prepare: |
 snap try
 mv complexion.bash-completer complexion.bash-completer.orig
 cp "${SPREAD_PATH}/${SPREAD_SUITE}/${SPREAD_VARIANT}.complete" complexion.bash-completer
+ # current ordering should ensure this anyway, but just in case, make sure we're
+ # using complete.sh and not the generated snippet by removing the snippet
+ rm /usr/share/bash-completion/completions/complexion
 )
 
 restore: |
diff --git a/tests/completion/snippets/task.exp b/tests/completion/snippets/task.exp
new file mode 100644
index 0000000000..218ae15911
--- /dev/null
+++ b/tests/completion/snippets/task.exp
@@ -0,0 +1,17 @@
+source "$::env(SPREAD_PATH)/$::env(SPREAD_SUITE)/lib.exp0"
+send "source $::env(SPREAD_PATH)/$::env(SPREAD_SUITE)/$::env(SPREAD_VARIANT).sh\n"
+next
+send "source /usr/share/bash-completion/bash_completion\n"
+next
+# compared to indirect/, this one does not source complete.sh
+chat "complexion \t\t" $::env(_OUT0)
+cancel
+# completion when the cursor is not at the end of the line:
+set back1 [string repeat "\b" [string length $::env(_KEY1)]]
+chat "complexion $::env(_KEY1)$back1\t\t" $::env(_OUT0)
+cancel
+chat "complexion $::env(_KEY1)\t" $::env(_OUT1)
+cancel
+chat "complexion $::env(_KEY2)\t\t" $::env(_OUT2)
+cancel
+brexit
diff --git a/tests/completion/snippets/task.yaml b/tests/completion/snippets/task.yaml
new file mode 100644
index 0000000000..ac73a79530
--- /dev/null
+++ b/tests/completion/snippets/task.yaml
@@ -0,0 +1,22 @@
+summary: indirect completion
+
+prepare: |
+ (
+ cd ../../lib/snaps/complexion
+ snap try
+ mv complexion.bash-completer complexion.bash-completer.orig
+ cp "${SPREAD_PATH}/${SPREAD_SUITE}/${SPREAD_VARIANT}.complete" complexion.bash-completer
+ )
+
+restore: |
+ (
+ cd ../../lib/snaps/complexion
+ mv complexion.bash-completer.orig complexion.bash-completer
+ snap remove complexion
+ )
+
+execute: |
+ d="$PWD"
+ source "${SPREAD_PATH}/${SPREAD_SUITE}/${SPREAD_VARIANT}.vars"
+ export _OUT0 _OUT1 _OUT2 _KEY1 _KEY2 _COMP
+ sudo PATH=$PATH -E -u test expect -d -f "$d"/task.exp
diff --git a/wrappers/binaries.go b/wrappers/binaries.go
index c1ef81aa17..992d62c010 100644
--- a/wrappers/binaries.go
+++ b/wrappers/binaries.go
@@ -24,6 +24,7 @@ import (
	"os"
 
	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/osutil"
	"github.com/snapcore/snapd/snap"
 )
 
@@ -43,6 +44,7 @@ func AddSnapBinaries(s *snap.Info) (err error) {
	return err
	}
 
+	noCompletion := !osutil.FileExists(dirs.CompletersDir) || !osutil.FileExists(dirs.CompleteSh)
	for _, app := range s.Apps {
	if app.IsService() {
	continue
@@ -56,6 +58,17 @@ func AddSnapBinaries(s *snap.Info) (err error) {
	return err
	}
	created = append(created, wrapperPath)
+
+	if noCompletion || app.Completer == "" {
+	continue
+	}
+	// symlink the completion snippet
+	compPath := app.CompleterPath()
+	if err := os.Symlink(dirs.CompleteSh, compPath); err == nil {
+	created = append(created, compPath)
+	} else if !os.IsExist(err) {
+	return err
+	}
	}
 
	return nil
@@ -65,6 +78,13 @@ func AddSnapBinaries(s *snap.Info) (err error) {
 func RemoveSnapBinaries(s *snap.Info) error {
	for _, app := range s.Apps {
	os.Remove(app.WrapperPath())
+	if app.Completer == "" {
+	continue
+	}
+	compPath := app.CompleterPath()
+	if target, err := os.Readlink(compPath); err == nil && target == dirs.CompleteSh {
+	os.Remove(compPath)
+	}
	}
 
	return nil
diff --git a/wrappers/binaries_test.go b/wrappers/binaries_test.go
index a3e65d4c97..e3c853facd 100644
--- a/wrappers/binaries_test.go
+++ b/wrappers/binaries_test.go
@@ -20,6 +20,7 @@
 package wrappers_test
 
 import (
+	"io/ioutil"
	"os"
	"path/filepath"
	"testing"
@@ -57,6 +58,9 @@ description: Hello...
 apps:
 hello:
 command: bin/hello
+ world:
+ command: bin/world
+ completer: world-completer.sh
 svc1:
 command: bin/hello
 stop-command: bin/goodbye
@@ -66,20 +70,69 @@ apps:
 const contentsHello = "HELLO"
 
 func (s *binariesTestSuite) TestAddSnapBinariesAndRemove(c *C) {
+	// no completers support -> no problem \o/
+	c.Assert(osutil.FileExists(dirs.CompletersDir), Equals, false)
+
+	s.testAddSnapBinariesAndRemove(c)
+}
+
+func (s *binariesTestSuite) TestAddSnapBinariesAndRemoveWithCompleters(c *C) {
+	c.Assert(os.MkdirAll(dirs.CompletersDir, 0755), IsNil)
+	c.Assert(os.MkdirAll(filepath.Dir(dirs.CompleteSh), 0755), IsNil)
+	c.Assert(ioutil.WriteFile(dirs.CompleteSh, nil, 0644), IsNil)
+	// full completers support -> we get completers \o/
+
+	s.testAddSnapBinariesAndRemove(c)
+}
+
+func (s *binariesTestSuite) TestAddSnapBinariesAndRemoveWithExistingCompleters(c *C) {
+	c.Assert(os.MkdirAll(dirs.CompletersDir, 0755), IsNil)
+	c.Assert(os.MkdirAll(filepath.Dir(dirs.CompleteSh), 0755), IsNil)
+	c.Assert(ioutil.WriteFile(dirs.CompleteSh, nil, 0644), IsNil)
+	// existing completers -> they're left alone \o/
+	c.Assert(ioutil.WriteFile(filepath.Join(dirs.CompletersDir, "hello-snap.world"), nil, 0644), IsNil)
+
+	s.testAddSnapBinariesAndRemove(c)
+}
+
+func (s *binariesTestSuite) testAddSnapBinariesAndRemove(c *C) {
	info := snaptest.MockSnap(c, packageHello, contentsHello, &snap.SideInfo{Revision: snap.R(11)})
+	completer := filepath.Join(dirs.CompletersDir, "hello-snap.world")
+	completerExisted := osutil.FileExists(completer)
 
	err := wrappers.AddSnapBinaries(info)
	c.Assert(err, IsNil)
 
-	link := filepath.Join(dirs.SnapBinariesDir, "hello-snap.hello")
-	target, err := os.Readlink(link)
-	c.Assert(err, IsNil)
-	c.Check(target, Equals, "/usr/bin/snap")
+	bins := []string{"hello-snap.hello", "hello-snap.world"}
+
+	for _, bin := range bins {
+	link := filepath.Join(dirs.SnapBinariesDir, bin)
+	target, err := os.Readlink(link)
+	c.Assert(err, IsNil, Commentf(bin))
+	c.Check(target, Equals, "/usr/bin/snap", Commentf(bin))
+	}
+
+	if osutil.FileExists(dirs.CompletersDir) {
+	if completerExisted {
+	// there was a completer there before, so it should _not_ be a symlink to our complete.sh
+	c.Assert(osutil.IsSymlink(completer), Equals, false)
+	} else {
+	target, err := os.Readlink(completer)
+	c.Assert(err, IsNil)
+	c.Check(target, Equals, dirs.CompleteSh)
+	}
+	}
 
	err = wrappers.RemoveSnapBinaries(info)
	c.Assert(err, IsNil)
 
-	c.Check(osutil.FileExists(link), Equals, false)
+	for _, bin := range bins {
+	link := filepath.Join(dirs.SnapBinariesDir, bin)
+	c.Check(osutil.FileExists(link), Equals, false, Commentf(bin))
+	}
+
+	// we left the existing completer alone, but removed it otherwise
+	c.Check(osutil.FileExists(completer), Equals, completerExisted)
 }
 
 func (s *binariesTestSuite) TestAddSnapBinariesCleansUpOnFailure(c *C) {