clean output

Signed-off-by: Harper, Jason M <[email protected]>

Author: harp-intel

cmd/config/restore.go

@@ -6,7 +6,9 @@ package config
 
 import (
 	"bufio"
+	"bytes"
 	"fmt"
+	"io"
 	"log/slog"
 	"os"
 	"os/exec"
@@ -114,7 +116,8 @@ func runRestoreCmd(cmd *cobra.Command, args []string) error {
 	// parse the configuration file
 	flagValues, err := parseConfigFile(configFilePath)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "Error: failed to parse configuration file: %v\n", err)
+		err = fmt.Errorf("failed to parse configuration file: %v", err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
 		slog.Error(err.Error())
 		cmd.SilenceUsage = true
 		return err
@@ -135,7 +138,11 @@ func runRestoreCmd(cmd *cobra.Command, args []string) error {
 	// build the command to execute
 	executable, err := os.Executable()
 	if err != nil {
-		return fmt.Errorf("failed to get executable path: %v", err)
+		err = fmt.Errorf("failed to get executable path: %v", err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+		slog.Error(err.Error())
+		cmd.SilenceUsage = true
+		return err
 	}
 
 	// build arguments: perfspect config --target ... --flag1 value1 --flag2 value2 ...
@@ -169,6 +176,9 @@ func runRestoreCmd(cmd *cobra.Command, args []string) error {
 		cmdArgs = append(cmdArgs, fmt.Sprintf("--%s", fv.flagName), fv.value)
 	}
 
+	// always add --no-summary to avoid printing config summary before and after changes
+	cmdArgs = append(cmdArgs, "--no-summary")
+
 	// show the command that will be executed
 	fmt.Printf("Command: %s %s\n\n", executable, strings.Join(cmdArgs, " "))
 
@@ -178,7 +188,11 @@ func runRestoreCmd(cmd *cobra.Command, args []string) error {
 		reader := bufio.NewReader(os.Stdin)
 		response, err := reader.ReadString('\n')
 		if err != nil {
-			return fmt.Errorf("failed to read user input: %v", err)
+			err = fmt.Errorf("failed to read user input: %v", err)
+			fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+			slog.Error(err.Error())
+			cmd.SilenceUsage = true
+			return err
 		}
 		response = strings.TrimSpace(strings.ToLower(response))
 		if response != "y" && response != "yes" {
@@ -193,15 +207,31 @@ func runRestoreCmd(cmd *cobra.Command, args []string) error {
 
 	execCmd := exec.Command(executable, cmdArgs...)
 	execCmd.Stdout = os.Stdout
-	execCmd.Stderr = os.Stderr
 	execCmd.Stdin = os.Stdin
 
+	// capture stderr for parsing
+	var stderrBuf bytes.Buffer
+	stderrWriter := io.MultiWriter(os.Stderr, &stderrBuf)
+	execCmd.Stderr = stderrWriter
+
 	err = execCmd.Run()
+
+	// parse stderr output and present results in flag order
+	parseAndPresentResults(stderrBuf.String(), flagValues)
+
 	if err != nil {
 		if exitErr, ok := err.(*exec.ExitError); ok {
-			return fmt.Errorf("config command failed with exit code %d", exitErr.ExitCode())
+			err = fmt.Errorf("config command failed with exit code %d", exitErr.ExitCode())
+			fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+			slog.Error(err.Error())
+			cmd.SilenceUsage = true
+			return err
 		}
-		return fmt.Errorf("failed to execute config command: %v", err)
+		err = fmt.Errorf("failed to execute config command: %v", err)
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+		slog.Error(err.Error())
+		cmd.SilenceUsage = true
+		return err
 	}
 
 	return nil
@@ -371,3 +401,64 @@ func parseEnableDisableOrOption(value string, validOptions []string) (string, er
 
 	return "", fmt.Errorf("invalid value '%s', valid options are: %s", value, strings.Join(validOptions, ", "))
 }
+
+// parseAndPresentResults parses the stderr output from the config command and presents
+// successes and errors in the same order as the config flags were specified
+// example: "configuration update complete: set gov to powersave, set c1-demotion to disable, set tdp to 350, set c6 to enable, set epb to 0, set core-max to 3.2, set cores to 86, set elc to default, failed to set pref-l2hw to enable, set pref-dcuhw to enable, set pref-llc to disable, set pref-aop to enable, set pref-l2adj to enable, set uncore-max-compute to 2.2, failed to set llc to 336, set pref-dcunp to enable, set pref-homeless to enable, set pref-amp to enable, set pref-dcuip to enable, set pref-llcpp to enable, set uncore-max-io to 2.5, set uncore-min-compute to 0.8, set uncore-min-io to 0.8"
+func parseAndPresentResults(stderrOutput string, flagValues []flagValue) {
+	if stderrOutput == "" {
+		return
+	}
+
+	// Parse stderr for success and error messages
+	// Looking for patterns like:
+	// - "set <flag> to <value>"
+	// - "failed to set <flag> to <value>"
+	// - "error: ..." messages related to flags
+
+	// Build a map of flag names to their results
+	flagResults := make(map[string]string)
+
+	// Regex patterns to match success and error messages
+	// Flag names can contain hyphens, so use [\w-]+ instead of \S+
+	successPattern := regexp.MustCompile(`set ([\w-]+) to ([^,]+)`)
+	errorPattern := regexp.MustCompile(`failed to set ([\w-]+) to ([^,]+)`)
+
+	// Parse stderr line by line
+	lines := strings.Split(stderrOutput, "\n")
+	for _, line := range lines {
+		// Check for success messages - use FindAllStringSubmatch to find all matches on the line
+		successMatches := successPattern.FindAllStringSubmatch(line, -1)
+		for _, matches := range successMatches {
+			if len(matches) >= 3 {
+				flagName := matches[1]
+				value := strings.TrimSpace(matches[2])
+				flagResults[flagName] = fmt.Sprintf("✓ Set %s to %s", flagName, value)
+			}
+		}
+
+		// Check for error messages - use FindAllStringSubmatch to find all matches on the line
+		errorMatches := errorPattern.FindAllStringSubmatch(line, -1)
+		for _, matches := range errorMatches {
+			if len(matches) >= 3 {
+				flagName := matches[1]
+				value := strings.TrimSpace(matches[2])
+				flagResults[flagName] = fmt.Sprintf("✗ Failed to set %s to %s", flagName, value)
+			}
+		}
+	}
+
+	// Present results in the order of flagValues
+	if len(flagValues) > 0 {
+		fmt.Println("\nConfiguration Results:")
+		for _, fv := range flagValues {
+			if result, found := flagResults[fv.flagName]; found {
+				fmt.Printf("  %s\n", result)
+			} else {
+				// If no explicit success or error was found, show unknown status
+				fmt.Printf("  ? %s: status unknown\n", fv.flagName)
+			}
+		}
+		fmt.Println()
+	}
+}

cmd/config/restore_test.go

@@ -4,7 +4,10 @@ package config
 // SPDX-License-Identifier: BSD-3-Clause
 
 import (
+	"bytes"
+	"io"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -190,3 +193,180 @@ func TestParseEnableDisableOrOption(t *testing.T) {
 		})
 	}
 }
+
+func TestParseAndPresentResults(t *testing.T) {
+	tests := []struct {
+		name           string
+		stderrOutput   string
+		flagValues     []flagValue
+		expectedOutput []string // lines we expect to see in output
+	}{
+		{
+			name:         "Example from function header comment",
+			stderrOutput: "configuration update complete: set gov to powersave, set c1-demotion to disable, set tdp to 350, set c6 to enable, set epb to 0, set core-max to 3.2, set cores to 86, set elc to default, failed to set pref-l2hw to enable, set pref-dcuhw to enable, set pref-llc to disable, set pref-aop to enable, set pref-l2adj to enable, set uncore-max-compute to 2.2, failed to set llc to 336, set pref-dcunp to enable, set pref-homeless to enable, set pref-amp to enable, set pref-dcuip to enable, set pref-llcpp to enable, set uncore-max-io to 2.5, set uncore-min-compute to 0.8, set uncore-min-io to 0.8",
+			flagValues: []flagValue{
+				{flagName: "cores", value: "86"},
+				{flagName: "llc", value: "336"},
+				{flagName: "tdp", value: "350"},
+				{flagName: "core-max", value: "3.2"},
+				{flagName: "uncore-max-compute", value: "2.2"},
+				{flagName: "uncore-min-compute", value: "0.8"},
+				{flagName: "uncore-max-io", value: "2.5"},
+				{flagName: "uncore-min-io", value: "0.8"},
+				{flagName: "epb", value: "0"},
+				{flagName: "gov", value: "powersave"},
+				{flagName: "elc", value: "default"},
+				{flagName: "pref-l2hw", value: "enable"},
+				{flagName: "pref-l2adj", value: "enable"},
+				{flagName: "pref-dcuhw", value: "enable"},
+				{flagName: "pref-dcuip", value: "enable"},
+				{flagName: "pref-dcunp", value: "enable"},
+				{flagName: "pref-amp", value: "enable"},
+				{flagName: "pref-llcpp", value: "enable"},
+				{flagName: "pref-aop", value: "enable"},
+				{flagName: "pref-homeless", value: "enable"},
+				{flagName: "pref-llc", value: "disable"},
+				{flagName: "c6", value: "enable"},
+				{flagName: "c1-demotion", value: "disable"},
+			},
+			expectedOutput: []string{
+				"✓ Set cores to 86",
+				"✗ Failed to set llc to 336",
+				"✓ Set tdp to 350",
+				"✓ Set core-max to 3.2",
+				"✓ Set uncore-max-compute to 2.2",
+				"✓ Set uncore-min-compute to 0.8",
+				"✓ Set uncore-max-io to 2.5",
+				"✓ Set uncore-min-io to 0.8",
+				"✓ Set epb to 0",
+				"✓ Set gov to powersave",
+				"✓ Set elc to default",
+				"✗ Failed to set pref-l2hw to enable",
+				"✓ Set pref-l2adj to enable",
+				"✓ Set pref-dcuhw to enable",
+				"✓ Set pref-dcuip to enable",
+				"✓ Set pref-dcunp to enable",
+				"✓ Set pref-amp to enable",
+				"✓ Set pref-llcpp to enable",
+				"✓ Set pref-aop to enable",
+				"✓ Set pref-homeless to enable",
+				"✓ Set pref-llc to disable",
+				"✓ Set c6 to enable",
+				"✓ Set c1-demotion to disable",
+			},
+		},
+		{
+			name:         "Empty stderr output",
+			stderrOutput: "",
+			flagValues: []flagValue{
+				{flagName: "cores", value: "86"},
+			},
+			expectedOutput: []string{}, // nothing should be printed
+		},
+		{
+			name: "Mixed success and error messages on separate lines",
+			stderrOutput: "gnr                   ⣾  preparing target\n" +
+				"gnr                   ⣽  configuration update complete: set cores to 86, failed to set llc to 336, set tdp to 350\n",
+			flagValues: []flagValue{
+				{flagName: "cores", value: "86"},
+				{flagName: "llc", value: "336"},
+				{flagName: "tdp", value: "350"},
+			},
+			expectedOutput: []string{
+				"✓ Set cores to 86",
+				"✗ Failed to set llc to 336",
+				"✓ Set tdp to 350",
+			},
+		},
+		{
+			name:         "Flag name with multiple hyphens",
+			stderrOutput: "set uncore-max-compute to 2.2, set uncore-min-io to 0.8",
+			flagValues: []flagValue{
+				{flagName: "uncore-max-compute", value: "2.2"},
+				{flagName: "uncore-min-io", value: "0.8"},
+			},
+			expectedOutput: []string{
+				"✓ Set uncore-max-compute to 2.2",
+				"✓ Set uncore-min-io to 0.8",
+			},
+		},
+		{
+			name:         "No matching flags in output",
+			stderrOutput: "some other message without flag updates",
+			flagValues: []flagValue{
+				{flagName: "cores", value: "86"},
+			},
+			expectedOutput: []string{
+				"? cores: status unknown",
+			},
+		},
+		{
+			name:         "Flag with underscore and numbers",
+			stderrOutput: "set pref_test123 to enable, failed to set flag_456 to disable",
+			flagValues: []flagValue{
+				{flagName: "pref_test123", value: "enable"},
+				{flagName: "flag_456", value: "disable"},
+			},
+			expectedOutput: []string{
+				"✓ Set pref_test123 to enable",
+				"✗ Failed to set flag_456 to disable",
+			},
+		},
+		{
+			name:         "Some flags updated, others not mentioned",
+			stderrOutput: "set cores to 86, set tdp to 350",
+			flagValues: []flagValue{
+				{flagName: "cores", value: "86"},
+				{flagName: "llc", value: "336"},
+				{flagName: "tdp", value: "350"},
+			},
+			expectedOutput: []string{
+				"✓ Set cores to 86",
+				"? llc: status unknown",
+				"✓ Set tdp to 350",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Capture stdout
+			oldStdout := os.Stdout
+			r, w, _ := os.Pipe()
+			os.Stdout = w
+
+			// Call the function
+			parseAndPresentResults(tt.stderrOutput, tt.flagValues)
+
+			// Restore stdout
+			w.Close()
+			os.Stdout = oldStdout
+
+			// Read captured output
+			var buf bytes.Buffer
+			io.Copy(&buf, r)
+			output := buf.String()
+
+			// Verify expected output
+			if len(tt.expectedOutput) == 0 {
+				// Should be empty or just whitespace
+				if strings.TrimSpace(output) != "" {
+					t.Errorf("Expected no output, got: %q", output)
+				}
+				return
+			}
+
+			// Check that each expected line is in the output
+			for _, expected := range tt.expectedOutput {
+				if !strings.Contains(output, expected) {
+					t.Errorf("Expected output to contain %q, but it didn't.\nFull output:\n%s", expected, output)
+				}
+			}
+
+			// Verify the output contains "Configuration Results:" header
+			if !strings.Contains(output, "Configuration Results:") {
+				t.Errorf("Expected output to contain 'Configuration Results:' header")
+			}
+		})
+	}
+}