feat(miniooni): make CLI much more user friendly (#913)

Part of https://github.com/ooni/probe/issues/2184, because I wanted
to allow swapping commands and options more freely.

As a side effect, this PR closes https://github.com/ooni/probe/issues/2248.

AFAICT, every usage that was legal before is still legal. What has
changed seems the freedom to swap commands and options and a much
better help that lists the available options.

internal/cmd/miniooni/consent.go

@@ -13,7 +13,7 @@ import (
 
 // acquireUserConsent ensures the user is okay with using miniooni. This function
 // panics if we do not have acquired the user consent.
-func acquireUserConsent(miniooniDir string, currentOptions Options) {
+func acquireUserConsent(miniooniDir string, currentOptions *Options) {
 	consentFile := path.Join(miniooniDir, "informed")
 	err := maybeWriteConsentFile(currentOptions.Yes, consentFile)
 	runtimex.PanicOnError(err, "cannot write informed consent file")

internal/cmd/miniooni/libminiooni.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"strings"
 	"time"
 
 	"github.com/apex/log"
@@ -14,9 +15,10 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/humanize"
 	"github.com/ooni/probe-cli/v3/internal/legacy/assetsdir"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/registry"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/version"
-	"github.com/pborman/getopt/v2"
+	"github.com/spf13/cobra"
 )
 
 // Options contains the options you can set from the CLI.
@@ -38,118 +40,227 @@ type Options struct {
 	TorBinary        string
 	Tunnel           string
 	Verbose          bool
-	Version          bool
 	Yes              bool
 }
 
-var globalOptions Options
-
-func init() {
-	getopt.FlagLong(
-		&globalOptions.Annotations, "annotation", 'A', "Add annotaton", "KEY=VALUE",
-	)
-	getopt.FlagLong(
-		&globalOptions.ExtraOptions, "option", 'O',
-		"Pass an option to the experiment", "KEY=VALUE",
-	)
-	getopt.FlagLong(
-		&globalOptions.InputFilePaths, "input-file", 'f',
-		"Path to input file to supply test-dependent input. File must contain one input per line.", "PATH",
-	)
-	getopt.FlagLong(
-		&globalOptions.HomeDir, "home", 0,
-		"Force specific home directory", "PATH",
-	)
-	getopt.FlagLong(
-		&globalOptions.Inputs, "input", 'i',
-		"Add test-dependent input to the test input", "INPUT",
-	)
-	getopt.FlagLong(
-		&globalOptions.MaxRuntime, "max-runtime", 0,
-		"Maximum runtime in seconds when looping over a list of inputs (zero means infinite)", "N",
-	)
-	getopt.FlagLong(
-		&globalOptions.NoJSON, "no-json", 'N', "Disable writing to disk",
-	)
-	getopt.FlagLong(
-		&globalOptions.NoCollector, "no-collector", 'n', "Don't use a collector",
-	)
-	getopt.FlagLong(
-		&globalOptions.ProbeServicesURL, "probe-services", 0,
-		"Set the URL of the probe-services instance you want to use", "URL",
-	)
-	getopt.FlagLong(
-		&globalOptions.Proxy, "proxy", 0, "Set the proxy URL", "URL",
-	)
-	getopt.FlagLong(
-		&globalOptions.Random, "random", 0, "Randomize inputs",
-	)
-	getopt.FlagLong(
-		&globalOptions.RepeatEvery, "repeat-every", 0,
-		"Repeat the measurement every INTERVAL number of seconds", "INTERVAL",
-	)
-	getopt.FlagLong(
-		&globalOptions.ReportFile, "reportfile", 'o',
-		"Set the report file path", "PATH",
-	)
-	getopt.FlagLong(
-		&globalOptions.TorArgs, "tor-args", 0,
-		"Extra args for tor binary (may be specified multiple times)",
-	)
-	getopt.FlagLong(
-		&globalOptions.TorBinary, "tor-binary", 0,
-		"Specify path to a specific tor binary",
-	)
-	getopt.FlagLong(
-		&globalOptions.Tunnel, "tunnel", 0,
-		"Name of the tunnel to use (one of `tor`, `psiphon`)",
-	)
-	getopt.FlagLong(
-		&globalOptions.Verbose, "verbose", 'v', "Increase verbosity",
-	)
-	getopt.FlagLong(
-		&globalOptions.Version, "version", 0, "Print version and exit",
-	)
-	getopt.FlagLong(
-		&globalOptions.Yes, "yes", 'y',
-		"Assume yes as the answer to all questions",
-	)
-}
-
-// main is the main function of miniooni. This function parses the command line
-// options and uses a global state. Use MainWithConfiguration if you want to avoid
-// using any global state and relying on command line options.
-//
-// This function will panic in case of a fatal error. It is up to you that
-// integrate this function to either handle the panic of ignore it.
+// main is the main function of miniooni.
 func main() {
-	getopt.Parse()
-	if globalOptions.Version {
-		fmt.Printf("%s\n", version.Version)
-		os.Exit(0)
+	var globalOptions Options
+	rootCmd := &cobra.Command{
+		Use:     "miniooni",
+		Short:   "miniooni is OONI's research client",
+		Args:    cobra.NoArgs,
+		Version: version.Version,
+	}
+	rootCmd.SetVersionTemplate("{{ .Version }}\n")
+	flags := rootCmd.PersistentFlags()
+
+	flags.StringSliceVarP(
+		&globalOptions.Annotations,
+		"annotation",
+		"A",
+		[]string{},
+		"add KEY=VALUE annotation to the report (can be repeated multiple times)",
+	)
+
+	flags.StringVar(
+		&globalOptions.HomeDir,
+		"home",
+		"",
+		"force specific home directory",
+	)
+
+	flags.BoolVarP(
+		&globalOptions.NoJSON,
+		"no-json",
+		"N",
+		false,
+		"disable writing to disk",
+	)
+
+	flags.BoolVarP(
+		&globalOptions.NoCollector,
+		"no-collector",
+		"n",
+		false,
+		"do not submit measurements to the OONI collector",
+	)
+
+	flags.StringVar(
+		&globalOptions.ProbeServicesURL,
+		"probe-services",
+		"",
+		"URL of the OONI backend instance you want to use",
+	)
+
+	flags.StringVar(
+		&globalOptions.Proxy,
+		"proxy",
+		"",
+		"set proxy URL to communicate with the OONI backend (mutually exclusive with --tunnel)",
+	)
+
+	flags.Int64Var(
+		&globalOptions.RepeatEvery,
+		"repeat-every",
+		0,
+		"wait the given number of seconds and then repeat the same measurement",
+	)
+
+	flags.StringVarP(
+		&globalOptions.ReportFile,
+		"reportfile",
+		"o",
+		"",
+		"set the output report file path (default: \"report.jsonl\")",
+	)
+
+	flags.StringSliceVar(
+		&globalOptions.TorArgs,
+		"tor-args",
+		[]string{},
+		"extra arguments for the tor binary (may be specified multiple times)",
+	)
+
+	flags.StringVar(
+		&globalOptions.TorBinary,
+		"tor-binary",
+		"",
+		"execute a specific tor binary",
+	)
+
+	flags.StringVar(
+		&globalOptions.Tunnel,
+		"tunnel",
+		"",
+		"tunnel to use to communicate with the OONI backend (one of: tor, psiphon)",
+	)
+
+	flags.BoolVarP(
+		&globalOptions.Verbose,
+		"verbose",
+		"v",
+		false,
+		"increase verbosity level",
+	)
+
+	flags.BoolVarP(
+		&globalOptions.Yes,
+		"yes",
+		"y",
+		false,
+		"assume yes as the answer to all questions",
+	)
+
+	rootCmd.MarkFlagsMutuallyExclusive("proxy", "tunnel")
+
+	registerAllExperiments(rootCmd, &globalOptions)
+	registerOONIRun(rootCmd, &globalOptions)
+
+	if err := rootCmd.Execute(); err != nil {
+		os.Exit(1)
 	}
-	runtimex.PanicIfFalse(len(getopt.Args()) == 1, "Missing experiment name")
-	runtimex.PanicOnError(engine.CheckEmbeddedPsiphonConfig(), "Invalid embedded psiphon config")
-	MainWithConfiguration(getopt.Arg(0), globalOptions)
 }
 
-// tunnelAndProxy is the text printed when the user specifies
-// both the --tunnel and the --proxy options
-const tunnelAndProxy = `USAGE ERROR: The --tunnel option and the --proxy
-option cannot be specified at the same time. The --tunnel option is actually
-just syntactic sugar for --proxy. Setting --tunnel=psiphon is currently the
-equivalent of setting --proxy=psiphon:///. This MAY change in a future version
-of miniooni, when we will allow a tunnel to use a proxy.
-`
+// TODO(bassosimone): the current implementation is basically a cobra application
+// where we hammered the previous miniooni code to make it work. We should
+// obviously strive for more correctness. For example, it's a bit disgusting
+// that MainWithConfiguration is invoked for both oonirun and random experiments.
+
+// registerOONIRun registers the oonirun subcommand
+func registerOONIRun(rootCmd *cobra.Command, globalOptions *Options) {
+	subCmd := &cobra.Command{
+		Use:   "oonirun",
+		Short: "Runs a given OONI Run v2 link",
+		Args:  cobra.NoArgs,
+		Run: func(cmd *cobra.Command, args []string) {
+			MainWithConfiguration(cmd.Use, globalOptions)
+		},
+	}
+	rootCmd.AddCommand(subCmd)
+	flags := subCmd.Flags()
+	flags.StringSliceVarP(
+		&globalOptions.Inputs,
+		"input",
+		"i",
+		[]string{},
+		"URL of the OONI Run v2 descriptor to run (may be specified multiple times)",
+	)
+}
+
+// registerAllExperiments registers a subcommand for each experiment
+func registerAllExperiments(rootCmd *cobra.Command, globalOptions *Options) {
+	for name, factory := range registry.AllExperiments {
+		subCmd := &cobra.Command{
+			Use:   name,
+			Short: fmt.Sprintf("Runs the %s experiment", name),
+			Args:  cobra.NoArgs,
+			Run: func(cmd *cobra.Command, args []string) {
+				MainWithConfiguration(cmd.Use, globalOptions)
+			},
+		}
+		rootCmd.AddCommand(subCmd)
+		flags := subCmd.Flags()
+
+		switch factory.InputPolicy() {
+		case model.InputOrQueryBackend,
+			model.InputStrictlyRequired,
+			model.InputOptional,
+			model.InputOrStaticDefault:
+
+			flags.StringSliceVarP(
+				&globalOptions.InputFilePaths,
+				"input-file",
+				"f",
+				[]string{},
+				"path to file to supply test dependent input (may be specified multiple times)",
+			)
+
+			flags.StringSliceVarP(
+				&globalOptions.Inputs,
+				"input",
+				"i",
+				[]string{},
+				"add test-dependent input (may be specified multiple times)",
+			)
+
+			flags.Int64Var(
+				&globalOptions.MaxRuntime,
+				"max-runtime",
+				0,
+				"maximum runtime in seconds for the experiment (zero means infinite)",
+			)
+
+			flags.BoolVar(
+				&globalOptions.Random,
+				"random",
+				false,
+				"randomize the inputs list",
+			)
+
+		default:
+			// nothing
+		}
+
+		if doc := documentationForOptions(name, factory); doc != "" {
+			flags.StringSliceVarP(
+				&globalOptions.ExtraOptions,
+				"option",
+				"O",
+				[]string{},
+				doc,
+			)
+		}
+	}
+}
 
 // MainWithConfiguration is the miniooni main with a specific configuration
 // represented by the experiment name and the current options.
 //
 // This function will panic in case of a fatal error. It is up to you that
 // integrate this function to either handle the panic of ignore it.
-func MainWithConfiguration(experimentName string, currentOptions Options) {
-	runtimex.PanicIfTrue(currentOptions.Proxy != "" && currentOptions.Tunnel != "",
-		tunnelAndProxy)
+func MainWithConfiguration(experimentName string, currentOptions *Options) {
+	runtimex.PanicOnError(engine.CheckEmbeddedPsiphonConfig(), "Invalid embedded psiphon config")
 	if currentOptions.Tunnel != "" {
 		currentOptions.Proxy = fmt.Sprintf("%s:///", currentOptions.Tunnel)
 	}
@@ -175,7 +286,7 @@ func MainWithConfiguration(experimentName string, currentOptions Options) {
 
 // mainSingleIteration runs a single iteration. There may be multiple iterations
 // when the user specifies the --repeat-every command line flag.
-func mainSingleIteration(logger model.Logger, experimentName string, currentOptions Options) {
+func mainSingleIteration(logger model.Logger, experimentName string, currentOptions *Options) {
 	extraOptions := mustMakeMapStringAny(currentOptions.ExtraOptions)
 	annotations := mustMakeMapStringString(currentOptions.Annotations)
 
@@ -225,3 +336,21 @@ func mainSingleIteration(logger model.Logger, experimentName string, currentOpti
 	// Otherwise just run OONI experiments as we normally do.
 	runx(ctx, sess, experimentName, annotations, extraOptions, currentOptions)
 }
+
+func documentationForOptions(name string, factory *registry.Factory) string {
+	var sb strings.Builder
+	options, err := factory.Options()
+	if err != nil || len(options) < 1 {
+		return ""
+	}
+	fmt.Fprint(&sb, "Pass KEY=VALUE options to the experiment. Available options:\n")
+	for name, info := range options {
+		if info.Doc == "" {
+			continue
+		}
+		fmt.Fprintf(&sb, "\n")
+		fmt.Fprintf(&sb, "  -O, --option %s=<%s>\n", name, info.Type)
+		fmt.Fprintf(&sb, "      %s\n", info.Doc)
+	}
+	return sb.String()
+}

internal/cmd/miniooni/main_test.go

@@ -6,7 +6,7 @@ func TestSimple(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")
 	}
-	MainWithConfiguration("example", Options{
+	MainWithConfiguration("example", &Options{
 		Yes: true,
 	})
 }

internal/cmd/miniooni/oonirun.go

@@ -16,7 +16,7 @@ import (
 // ooniRunMain runs the experiments described by the given OONI Run URLs. This
 // function works with both v1 and v2 OONI Run URLs.
 func ooniRunMain(ctx context.Context,
-	sess *engine.Session, currentOptions Options, annotations map[string]string) {
+	sess *engine.Session, currentOptions *Options, annotations map[string]string) {
 	runtimex.PanicIfTrue(
 		len(currentOptions.Inputs) <= 0,
 		"in oonirun mode you need to specify at least one URL using `-i URL`",

internal/cmd/miniooni/runx.go

@@ -13,7 +13,7 @@ import (
 
 // runx runs the given experiment by name
 func runx(ctx context.Context, sess oonirun.Session, experimentName string,
-	annotations map[string]string, extraOptions map[string]any, currentOptions Options) {
+	annotations map[string]string, extraOptions map[string]any, currentOptions *Options) {
 	desc := &oonirun.Experiment{
 		Annotations:    annotations,
 		ExtraOptions:   extraOptions,

internal/cmd/miniooni/session.go

@@ -20,7 +20,7 @@ const (
 )
 
 // newSessionOrPanic creates and starts a new session or panics on failure
-func newSessionOrPanic(ctx context.Context, currentOptions Options,
+func newSessionOrPanic(ctx context.Context, currentOptions *Options,
 	miniooniDir string, logger model.Logger) *engine.Session {
 	var proxyURL *url.URL
 	if currentOptions.Proxy != "" {