From 0619d2498fba7e7f67bac4942b190d8698e07fe9 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Fri, 5 May 2023 22:14:12 +0200 Subject: [PATCH 1/3] Restructure code to let linker perform deadcode elimination step Cobra, in its default configuration, will execute a template to generate help, usage and version outputs. Text/template execution calls MethodByName and MethodByName disables dead code elimination in the Go linker, therefore all programs that make use of cobra will be linked with dead code elimination disabled, even if they end up replacing the default usage, help and version formatters with a custom function and no actual text/template evaluations are ever made at runtime. Dead code elimination in the linker helps reduce disk space and memory utilization of programs. For example, for the simple example program used by TestDeadcodeElimination 40% of the final executable size is dead code. For a more realistic example, 12% of the size of Delve's executable is deadcode. This PR changes Cobra so that, in its default configuration, it does not automatically inhibit deadcode elimination by: 1. changing Cobra's default behavior to emit output for usage and help using simple Go functions instead of template execution 2. quarantining all calls to template execution into SetUsageTemplate, SetHelpTemplate and SetVersionTemplate so that the linker can statically determine if they are reachable --- cobra.go | 16 ++-- cobra_test.go | 58 ++++++++++++++ command.go | 218 ++++++++++++++++++++++++++++++++++++++------------ 3 files changed, 236 insertions(+), 56 deletions(-) diff --git a/cobra.go b/cobra.go index e0b0947b0..d9cd2414e 100644 --- a/cobra.go +++ b/cobra.go @@ -176,12 +176,16 @@ func rpad(s string, padding int) string { return fmt.Sprintf(formattedString, s) } -// tmpl executes the given template text on data, writing the result to w. -func tmpl(w io.Writer, text string, data interface{}) error { - t := template.New("top") - t.Funcs(templateFuncs) - template.Must(t.Parse(text)) - return t.Execute(w, data) +func tmpl(text string) *tmplFunc { + return &tmplFunc{ + tmpl: text, + fn: func(w io.Writer, data interface{}) error { + t := template.New("top") + t.Funcs(templateFuncs) + template.Must(t.Parse(text)) + return t.Execute(w, data) + }, + } } // ld compares two strings and returns the levenshtein distance between them. diff --git a/cobra_test.go b/cobra_test.go index 2bba461c5..212d09ab7 100644 --- a/cobra_test.go +++ b/cobra_test.go @@ -15,6 +15,10 @@ package cobra import ( + "os" + "os/exec" + "path/filepath" + "strings" "testing" "text/template" ) @@ -222,3 +226,57 @@ func TestRpad(t *testing.T) { }) } } + +func TestDeadcodeElimination(t *testing.T) { + // check that a simple program using cobra in its default configuration is + // linked with deadcode elimination enabled. + const ( + dirname = "test_deadcode" + progname = "test_deadcode_elimination" + ) + os.Mkdir(dirname, 0770) + filename := filepath.Join(dirname, progname+".go") + err := os.WriteFile(filename, []byte(`package main + +import ( + "fmt" + "os" + + "github.com/spf13/cobra" +) + +var rootCmd = &cobra.Command{ + Version: "1.0", + Use: "example_program", + Short: "example_program - test fixture to check that deadcode elimination is allowed", + Run: func(cmd *cobra.Command, args []string) { + fmt.Println("hello world") + }, + Aliases: []string{"alias1", "alias2"}, + Example: "stringer --help", +} + +func main() { + if err := rootCmd.Execute(); err != nil { + fmt.Fprintf(os.Stderr, "Whoops. There was an error while executing your CLI '%s'", err) + os.Exit(1) + } +} +`), 0660) + if err != nil { + t.Fatalf("could not write test program: %v", err) + } + defer os.RemoveAll(dirname) + buf, err := exec.Command("go", "build", filename).CombinedOutput() + if err != nil { + t.Fatalf("could not compile test program: %s", string(buf)) + } + defer os.Remove(progname) + buf, err = exec.Command("go", "tool", "nm", progname).CombinedOutput() + if err != nil { + t.Fatalf("could not run go tool nm: %v", err) + } + if strings.Contains(string(buf), "MethodByName") { + t.Error("compiled programs contains MethodByName symbol") + } +} diff --git a/command.go b/command.go index 4cd712bcb..7216f1ca6 100644 --- a/command.go +++ b/command.go @@ -168,12 +168,12 @@ type Command struct { // usageFunc is usage func defined by user. usageFunc func(*Command) error // usageTemplate is usage template defined by user. - usageTemplate string + usageTemplate *tmplFunc // flagErrorFunc is func defined by user and it's called when the parsing of // flags returns an error. flagErrorFunc func(*Command, error) error // helpTemplate is help template defined by user. - helpTemplate string + helpTemplate *tmplFunc // helpFunc is help func defined by user. helpFunc func(*Command, []string) // helpCommand is command with usage 'help'. If it's not defined by user, @@ -186,7 +186,7 @@ type Command struct { completionCommandGroupID string // versionTemplate is the version template defined by user. - versionTemplate string + versionTemplate *tmplFunc // errPrefix is the error message prefix defined by user. errPrefix string @@ -313,7 +313,7 @@ func (c *Command) SetUsageFunc(f func(*Command) error) { // SetUsageTemplate sets usage template. Can be defined by Application. func (c *Command) SetUsageTemplate(s string) { - c.usageTemplate = s + c.usageTemplate = tmpl(s) } // SetFlagErrorFunc sets a function to generate an error when flag parsing @@ -349,12 +349,12 @@ func (c *Command) SetCompletionCommandGroupID(groupID string) { // SetHelpTemplate sets help template to be used. Application can use it to set custom template. func (c *Command) SetHelpTemplate(s string) { - c.helpTemplate = s + c.helpTemplate = tmpl(s) } // SetVersionTemplate sets version template to be used. Application can use it to set custom template. func (c *Command) SetVersionTemplate(s string) { - c.versionTemplate = s + c.versionTemplate = tmpl(s) } // SetErrPrefix sets error message prefix to be used. Application can use it to set custom prefix. @@ -435,7 +435,11 @@ func (c *Command) UsageFunc() (f func(*Command) error) { } return func(c *Command) error { c.mergePersistentFlags() - err := tmpl(c.OutOrStderr(), c.UsageTemplate(), c) + fn := defaultUsageFunc + if c.usageTemplate != nil { + fn = c.usageTemplate.fn + } + err := fn(c.OutOrStderr(), c) if err != nil { c.PrintErrln(err) } @@ -461,9 +465,13 @@ func (c *Command) HelpFunc() func(*Command, []string) { } return func(c *Command, a []string) { c.mergePersistentFlags() + fn := defaultHelpFunc + if c.helpTemplate != nil { + fn = c.helpTemplate.fn + } // The help should be sent to stdout // See https://github.com/spf13/cobra/issues/1002 - err := tmpl(c.OutOrStdout(), c.HelpTemplate(), c) + err := fn(c.OutOrStdout(), c) if err != nil { c.PrintErrln(err) } @@ -545,70 +553,38 @@ func (c *Command) NamePadding() int { // UsageTemplate returns usage template for the command. func (c *Command) UsageTemplate() string { - if c.usageTemplate != "" { - return c.usageTemplate + if c.usageTemplate != nil { + return c.usageTemplate.tmpl } if c.HasParent() { return c.parent.UsageTemplate() } - return `Usage:{{if .Runnable}} - {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}} - {{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}} - -Aliases: - {{.NameAndAliases}}{{end}}{{if .HasExample}} - -Examples: -{{.Example}}{{end}}{{if .HasAvailableSubCommands}}{{$cmds := .Commands}}{{if eq (len .Groups) 0}} - -Available Commands:{{range $cmds}}{{if (or .IsAvailableCommand (eq .Name "help"))}} - {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{else}}{{range $group := .Groups}} - -{{.Title}}{{range $cmds}}{{if (and (eq .GroupID $group.ID) (or .IsAvailableCommand (eq .Name "help")))}} - {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{if not .AllChildCommandsHaveGroup}} - -Additional Commands:{{range $cmds}}{{if (and (eq .GroupID "") (or .IsAvailableCommand (eq .Name "help")))}} - {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{end}}{{end}}{{if .HasAvailableLocalFlags}} - -Flags: -{{.LocalFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasAvailableInheritedFlags}} - -Global Flags: -{{.InheritedFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasHelpSubCommands}} - -Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}} - {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableSubCommands}} - -Use "{{.CommandPath}} [command] --help" for more information about a command.{{end}} -` + return defaultUsageTemplate } // HelpTemplate return help template for the command. func (c *Command) HelpTemplate() string { - if c.helpTemplate != "" { - return c.helpTemplate + if c.helpTemplate != nil { + return c.helpTemplate.tmpl } if c.HasParent() { return c.parent.HelpTemplate() } - return `{{with (or .Long .Short)}}{{. | trimTrailingWhitespaces}} - -{{end}}{{if or .Runnable .HasSubCommands}}{{.UsageString}}{{end}}` + return defaultHelpTemplate } // VersionTemplate return version template for the command. func (c *Command) VersionTemplate() string { - if c.versionTemplate != "" { - return c.versionTemplate + if c.versionTemplate != nil { + return c.versionTemplate.tmpl } if c.HasParent() { return c.parent.VersionTemplate() } - return `{{with .DisplayName}}{{printf "%s " .}}{{end}}{{printf "version %s" .Version}} -` + return defaultVersionTemplate } // ErrPrefix return error message prefix for the command @@ -915,7 +891,11 @@ func (c *Command) execute(a []string) (err error) { return err } if versionVal { - err := tmpl(c.OutOrStdout(), c.VersionTemplate(), c) + fn := defaultVersionFunc + if c.versionTemplate != nil { + fn = c.versionTemplate.fn + } + err := fn(c.OutOrStdout(), c) if err != nil { c.Println(err) } @@ -1897,3 +1877,141 @@ func commandNameMatches(s string, t string) bool { return s == t } + +// tmplFunc holds a template and a function that will execute said template. +type tmplFunc struct { + tmpl string + fn func(io.Writer, interface{}) error +} + +var defaultUsageTemplate = `Usage:{{if .Runnable}} + {{.UseLine}}{{end}}{{if .HasAvailableSubCommands}} + {{.CommandPath}} [command]{{end}}{{if gt (len .Aliases) 0}} + +Aliases: + {{.NameAndAliases}}{{end}}{{if .HasExample}} + +Examples: +{{.Example}}{{end}}{{if .HasAvailableSubCommands}}{{$cmds := .Commands}}{{if eq (len .Groups) 0}} + +Available Commands:{{range $cmds}}{{if (or .IsAvailableCommand (eq .Name "help"))}} + {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{else}}{{range $group := .Groups}} + +{{.Title}}{{range $cmds}}{{if (and (eq .GroupID $group.ID) (or .IsAvailableCommand (eq .Name "help")))}} + {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{if not .AllChildCommandsHaveGroup}} + +Additional Commands:{{range $cmds}}{{if (and (eq .GroupID "") (or .IsAvailableCommand (eq .Name "help")))}} + {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{end}}{{end}}{{if .HasAvailableLocalFlags}} + +Flags: +{{.LocalFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasAvailableInheritedFlags}} + +Global Flags: +{{.InheritedFlags.FlagUsages | trimTrailingWhitespaces}}{{end}}{{if .HasHelpSubCommands}} + +Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}} + {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableSubCommands}} + +Use "{{.CommandPath}} [command] --help" for more information about a command.{{end}} +` + +// defaultUsageFunc is equivalent to executing defaultUsageTemplate. The two should be changed in sync. +func defaultUsageFunc(w io.Writer, in interface{}) error { + c := in.(*Command) + fmt.Fprint(w, "Usage:") + if c.Runnable() { + fmt.Fprintf(w, "\n %s", c.UseLine()) + } + if c.HasAvailableSubCommands() { + fmt.Fprintf(w, "\n %s [command]", c.CommandPath()) + } + if len(c.Aliases) > 0 { + fmt.Fprintf(w, "\n\nAliases:\n") + fmt.Fprintf(w, " %s", c.NameAndAliases()) + } + if c.HasExample() { + fmt.Fprintf(w, "\n\nExamples:\n") + fmt.Fprintf(w, "%s", c.Example) + } + if c.HasAvailableSubCommands() { + cmds := c.Commands() + if len(c.Groups()) == 0 { + fmt.Fprintf(w, "\n\nAvailable Commands:") + for _, subcmd := range cmds { + if subcmd.IsAvailableCommand() || subcmd.Name() == "help" { + fmt.Fprintf(w, "\n %s %s", rpad(subcmd.Name(), subcmd.NamePadding()), subcmd.Short) + } + } + } else { + for _, group := range c.Groups() { + fmt.Fprintf(w, "\n\n%s", group.Title) + for _, subcmd := range cmds { + if subcmd.GroupID == group.ID && (subcmd.IsAvailableCommand() || subcmd.Name() == "help") { + fmt.Fprintf(w, "\n %s %s", rpad(subcmd.Name(), subcmd.NamePadding()), subcmd.Short) + } + } + } + if !c.AllChildCommandsHaveGroup() { + fmt.Fprintf(w, "\n\nAdditional Commands:") + for _, subcmd := range cmds { + if subcmd.GroupID == "" && (subcmd.IsAvailableCommand() || subcmd.Name() == "help") { + fmt.Fprintf(w, "\n %s %s", rpad(subcmd.Name(), subcmd.NamePadding()), subcmd.Short) + } + } + } + } + } + if c.HasAvailableLocalFlags() { + fmt.Fprintf(w, "\n\nFlags:\n") + fmt.Fprint(w, trimRightSpace(c.LocalFlags().FlagUsages())) + } + if c.HasAvailableInheritedFlags() { + fmt.Fprintf(w, "\n\nGlobal Flags:\n") + fmt.Fprint(w, trimRightSpace(c.InheritedFlags().FlagUsages())) + } + if c.HasHelpSubCommands() { + fmt.Fprintf(w, "\n\nAdditional help topcis:") + for _, subcmd := range c.Commands() { + if subcmd.IsAdditionalHelpTopicCommand() { + fmt.Fprintf(w, "\n %s %s", rpad(subcmd.CommandPath(), subcmd.CommandPathPadding()), subcmd.Short) + } + } + } + if c.HasAvailableSubCommands() { + fmt.Fprintf(w, "\n\nUse \"%s [command] --help\" for more information about a command.", c.CommandPath()) + } + fmt.Fprintln(w) + return nil +} + +var defaultHelpTemplate = `{{with (or .Long .Short)}}{{. | trimTrailingWhitespaces}} + +{{end}}{{if or .Runnable .HasSubCommands}}{{.UsageString}}{{end}}` + +// defaultHelpFunc is equivalent to executing defaultHelpTemplate. The two should be changed in sync. +func defaultHelpFunc(w io.Writer, in interface{}) error { + c := in.(*Command) + usage := c.Long + if usage == "" { + usage = c.Short + } + usage = trimRightSpace(usage) + if usage != "" { + fmt.Fprintln(w, usage) + fmt.Fprintln(w) + } + if c.Runnable() || c.HasSubCommands() { + fmt.Fprint(w, c.UsageString()) + } + return nil +} + +var defaultVersionTemplate = `{{with .DisplayName}}{{printf "%s " .}}{{end}}{{printf "version %s" .Version}} +` + +// defaultVersionFunc is equivalent to executing defaultVersionTemplate. The two should be changed in sync. +func defaultVersionFunc(w io.Writer, in interface{}) error { + c := in.(*Command) + _, err := fmt.Fprintf(w, "%s version %s\n", c.DisplayName(), c.Version) + return err +} From 5e44e3ed9e1ab4711befcc07caabb615af266843 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Tue, 15 Oct 2024 10:33:24 +0200 Subject: [PATCH 2/3] addressing review comments --- cobra_test.go | 20 +++++++++++++++++--- site/content/user_guide.md | 3 ++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/cobra_test.go b/cobra_test.go index 212d09ab7..c86728251 100644 --- a/cobra_test.go +++ b/cobra_test.go @@ -227,6 +227,20 @@ func TestRpad(t *testing.T) { } } +// TestDeadcodeElimination checks that a simple program using cobra in its +// default configuration is linked taking full advantage of the linker's +// deadcode elimination step. +// +// If reflect.Value.MethodByName/reflect.Value.Method are reachable the +// linker will not always be able to prove that exported methods are +// unreachable, making deadcode elimination less effective. Using +// text/template and html/template makes reflect.Value.MethodByName +// reachable. +// Since cobra can use text/template templates this test checks that in its +// default configuration that code path can be proven to be unreachable by +// the linker. +// +// See also: https://github.com/spf13/cobra/pull/1956 func TestDeadcodeElimination(t *testing.T) { // check that a simple program using cobra in its default configuration is // linked with deadcode elimination enabled. @@ -234,7 +248,8 @@ func TestDeadcodeElimination(t *testing.T) { dirname = "test_deadcode" progname = "test_deadcode_elimination" ) - os.Mkdir(dirname, 0770) + _ = os.Mkdir(dirname, 0770) + defer os.RemoveAll(dirname) filename := filepath.Join(dirname, progname+".go") err := os.WriteFile(filename, []byte(`package main @@ -262,11 +277,10 @@ func main() { os.Exit(1) } } -`), 0660) +`), 0600) if err != nil { t.Fatalf("could not write test program: %v", err) } - defer os.RemoveAll(dirname) buf, err := exec.Command("go", "build", filename).CombinedOutput() if err != nil { t.Fatalf("could not compile test program: %s", string(buf)) diff --git a/site/content/user_guide.md b/site/content/user_guide.md index dd26de4f8..01cef7c01 100644 --- a/site/content/user_guide.md +++ b/site/content/user_guide.md @@ -552,7 +552,8 @@ cmd.SetHelpFunc(f func(*Command, []string)) cmd.SetHelpTemplate(s string) ``` -The latter two will also apply to any children commands. +The latter two will also apply to any children commands. Templates specified with SetHelpTemplate are evaluated using +`text/template` which can increase the size of the compiled executable. ## Usage Message From 45cac892e43fe3a8674a878aaac18612f7a58235 Mon Sep 17 00:00:00 2001 From: aarzilli Date: Fri, 6 Dec 2024 16:38:53 +0100 Subject: [PATCH 3/3] make string "help" a constant --- command.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/command.go b/command.go index 7216f1ca6..ba0e8428e 100644 --- a/command.go +++ b/command.go @@ -33,6 +33,8 @@ import ( const ( FlagSetByCobraAnnotation = "cobra_annotation_flag_set_by_cobra" CommandDisplayNameAnnotation = "cobra_annotation_command_display_name" + + helpFlagName = "help" ) // FParseErrWhitelist configures Flag parse errors to be ignored @@ -1168,7 +1170,7 @@ func (c *Command) checkCommandGroups() { // If c already has help flag, it will do nothing. func (c *Command) InitDefaultHelpFlag() { c.mergePersistentFlags() - if c.Flags().Lookup("help") == nil { + if c.Flags().Lookup(helpFlagName) == nil { usage := "help for " name := c.DisplayName() if name == "" { @@ -1176,8 +1178,8 @@ func (c *Command) InitDefaultHelpFlag() { } else { usage += name } - c.Flags().BoolP("help", "h", false, usage) - _ = c.Flags().SetAnnotation("help", FlagSetByCobraAnnotation, []string{"true"}) + c.Flags().BoolP(helpFlagName, "h", false, usage) + _ = c.Flags().SetAnnotation(helpFlagName, FlagSetByCobraAnnotation, []string{"true"}) } }