ctags: allow binary to be anything with validation (#652)

Previously we enforced the binary was called universal-ctags. However,
we let users override the name of the binary, so if they override it we
should use it. To prevent footguns, we now validate the binary was built
with the interactive feature.

In the case of scip-ctags we use the old validation of just checking the
name.

Additionally we fix a bug that was introduced where if symbols are
optional we continue if parsing fails.

Test Plan: indexed with and without universal-ctags. Ctags on my mbp
points to something from xcode which wouldn't work

  $ CTAGS_COMMAND=ctags go run ./cmd/zoekt-git-index -require_ctags .
  2023/10/04 17:08:12 indexGitRepo(/Users/keegan/src/github.com/sourcegraph/zoekt, delta=false): build.NewBuilder: ctags.NewParserMap: ctags binary is not universal-ctags or is not compiled with +interactive feature: bin=ctags
  exit status 1

  $ CTAGS_COMMAND=universal-ctags go run ./cmd/zoekt-git-index -require_ctags .
  2023/10/04 17:08:29 finished github.com%2Fsourcegraph%2Fzoekt_v16.00000.zoekt: 8657338 index bytes (overhead 2.9)

  $ CTAGS_COMMAND=ctags go run ./cmd/zoekt-git-index .
  2023/10/04 17:08:40 finished github.com%2Fsourcegraph%2Fzoekt_v16.00000.zoekt: 8538246 index bytes (overhead 2.9)

build/ctags.go

@@ -53,6 +53,10 @@ func ctagsAddSymbolsParserMap(todo []*zoekt.Document, languageMap ctags.Language
 		parser := parserMap[parserKind]
 		if parser == nil {
 			parser = parserMap[ctags.UniversalCTags]
+			if parser == nil {
+				// this happens if CTagsMustSucceed is not true and we didn't find universal-ctags
+				continue
+			}
 		}
 
 		es, err := parser.Parse(doc.Name, doc.Content)

ctags/json.go

@@ -15,9 +15,11 @@
 package ctags
 
 import (
+	"bytes"
 	"fmt"
 	"log"
 	"os"
+	"os/exec"
 	"strings"
 	"sync"
 	"time"
@@ -119,19 +121,41 @@ func (lp *lockedParser) close() {
 
 // NewParser creates a parser that is implemented by the given
 // universal-ctags binary. The parser is safe for concurrent use.
-func NewParser(bin string) (Parser, error) {
-	if strings.Contains(bin, "universal-ctags") || strings.Contains(bin, "scip-ctags") {
-		opts := goctags.Options{
-			Bin: bin,
+func NewParser(parserType CTagsParserType, bin string) (Parser, error) {
+	if err := checkBinary(parserType, bin); err != nil {
+		return nil, err
+	}
+
+	opts := goctags.Options{
+		Bin: bin,
+	}
+	if debug {
+		opts.Info = log.New(os.Stderr, "CTAGS INF: ", log.LstdFlags)
+		opts.Debug = log.New(os.Stderr, "CTAGS DBG: ", log.LstdFlags)
+	}
+	return &lockedParser{
+		opts: opts,
+	}, nil
+}
+
+// checkBinary does checks on bin to ensure we can correctly use the binary
+// for symbols. It is more user friendly to fail early in this case.
+func checkBinary(typ CTagsParserType, bin string) error {
+	switch typ {
+	case UniversalCTags:
+		helpOutput, err := exec.Command(bin, "--help").CombinedOutput()
+		if err != nil {
+			return fmt.Errorf("failed to check if %s is universal-ctags: %w\n--help output:\n%s", bin, err, string(helpOutput))
 		}
-		if debug {
-			opts.Info = log.New(os.Stderr, "CTAGS INF: ", log.LstdFlags)
-			opts.Debug = log.New(os.Stderr, "CTAGS DBG: ", log.LstdFlags)
+		if !bytes.Contains(helpOutput, []byte("+interactive")) {
+			return fmt.Errorf("ctags binary is not universal-ctags or is not compiled with +interactive feature: bin=%s", bin)
+		}
+
+	case ScipCTags:
+		if !strings.Contains(bin, "scip-ctags") {
+			return fmt.Errorf("only supports scip-ctags, not %s", bin)
 		}
-		return &lockedParser{
-			opts: opts,
-		}, nil
 	}
 
-	return nil, fmt.Errorf("only supports universal-ctags, not %s", bin)
+	return nil
 }

ctags/json_test.go

@@ -27,7 +27,7 @@ func TestJSON(t *testing.T) {
 		t.Skip(err)
 	}
 
-	p, err := NewParser("universal-ctags")
+	p, err := NewParser(UniversalCTags, "universal-ctags")
 	if err != nil {
 		t.Fatal("newProcess", err)
 	}

ctags/parser_map.go

@@ -66,7 +66,7 @@ func NewParserMap(bins ParserBinMap, cTagsMustSucceed bool) (ParserMap, error) {
 	for _, parserType := range []CTagsParserType{UniversalCTags, ScipCTags} {
 		bin := bins[parserType]
 		if bin != "" {
-			parser, err := NewParser(bin)
+			parser, err := NewParser(parserType, bin)
 
 			if err != nil && cTagsMustSucceed {
 				return nil, fmt.Errorf("ctags.NewParserMap: %v", err)