Make crosstool to starlark converter error out if it comes across multiple expand_if_all_available or expand_if_none_available in a single flag_group

PiperOrigin-RevId: 232499399

tools/migration/crosstool_to_starlark_lib.go

@@ -8,6 +8,7 @@ package crosstooltostarlarklib
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -352,7 +353,7 @@ func getStringStatement(crosstool *crosstoolpb.CrosstoolRelease,
 }
 
 func getFeatures(crosstool *crosstoolpb.CrosstoolRelease) (
-	map[string][]string, map[string]map[string][]string) {
+	map[string][]string, map[string]map[string][]string, error) {
 	featureNameToFeature := make(map[string]map[string][]string)
 	toolchainToFeatures := make(map[string][]string)
 	for _, toolchain := range crosstool.GetToolchain() {
@@ -364,7 +365,11 @@ func getFeatures(crosstool *crosstoolpb.CrosstoolRelease) (
 			featureName := strings.ToLower(feature.GetName()) + "_feature"
 			featureName = strings.Replace(featureName, "+", "p", -1)
 			featureName = strings.Replace(featureName, ".", "_", -1)
-			stringFeature := parseFeature(feature, 1)
+			stringFeature, err := parseFeature(feature, 1)
+			if err != nil {
+				return nil, nil, fmt.Errorf(
+					"Error in feature '%s': %v", feature.GetName(), err)
+			}
 			if _, ok := featureNameToFeature[featureName]; !ok {
 				featureNameToFeature[featureName] = make(map[string][]string)
 			}
@@ -373,7 +378,7 @@ func getFeatures(crosstool *crosstoolpb.CrosstoolRelease) (
 			toolchainToFeatures[id] = append(toolchainToFeatures[id], featureName)
 		}
 	}
-	return toolchainToFeatures, featureNameToFeature
+	return toolchainToFeatures, featureNameToFeature, nil
 }
 
 func getFeaturesDeclaration(crosstool *crosstoolpb.CrosstoolRelease,
@@ -412,7 +417,7 @@ func getFeaturesStmt(cToolchainIdentifiers map[string]CToolchainIdentifier,
 }
 
 func getActions(crosstool *crosstoolpb.CrosstoolRelease) (
-	map[string][]string, map[string]map[string][]string) {
+	map[string][]string, map[string]map[string][]string, error) {
 	actionNameToAction := make(map[string]map[string][]string)
 	toolchainToActions := make(map[string][]string)
 	for _, toolchain := range crosstool.GetToolchain() {
@@ -429,7 +434,11 @@ func getActions(crosstool *crosstoolpb.CrosstoolRelease) (
 				actionName = strings.Replace(actionName, "+", "p", -1)
 				actionName = strings.Replace(actionName, ".", "_", -1)
 			}
-			stringAction := parseAction(action, 1)
+			stringAction, err := parseAction(action, 1)
+			if err != nil {
+				return nil, nil, fmt.Errorf(
+					"Error in action_config '%s': %v", action.GetActionName(), err)
+			}
 			if _, ok := actionNameToAction[actionName]; !ok {
 				actionNameToAction[actionName] = make(map[string][]string)
 			}
@@ -440,7 +449,7 @@ func getActions(crosstool *crosstoolpb.CrosstoolRelease) (
 				strings.TrimPrefix(strings.ToLower(actionName), "action_names.")+"_action")
 		}
 	}
-	return toolchainToActions, actionNameToAction
+	return toolchainToActions, actionNameToAction, nil
 }
 
 func getActionConfigsDeclaration(
@@ -482,7 +491,7 @@ func getActionConfigsStmt(
 	return strings.Join(res, "\n")
 }
 
-func parseAction(action *crosstoolpb.CToolchain_ActionConfig, depth int) string {
+func parseAction(action *crosstoolpb.CToolchain_ActionConfig, depth int) (string, error) {
 	actionName := action.GetActionName()
 	aName := ""
 	if val, ok := actionNames[actionName]; ok {
@@ -496,9 +505,11 @@ func parseAction(action *crosstoolpb.CToolchain_ActionConfig, depth int) string
 		fields = append(fields, "enabled = True")
 	}
 	if len(action.GetFlagSet()) != 0 {
-		flagSets := "flag_sets = " +
+		flagSets, err := parseFlagSets(action.GetFlagSet(), depth+1)
-			parseFlagSets(action.GetFlagSet(), depth+1)
+		if err != nil {
-		fields = append(fields, flagSets)
+			return "", err
+		}
+		fields = append(fields, "flag_sets = "+flagSets)
 	}
 	if len(action.GetImplies()) != 0 {
 		implies := "implies = " +
@@ -509,7 +520,7 @@ func parseAction(action *crosstoolpb.CToolchain_ActionConfig, depth int) string
 		tools := "tools = " + parseTools(action.GetTool(), depth+1)
 		fields = append(fields, tools)
 	}
-	return createObject("action_config", fields, depth)
+	return createObject("action_config", fields, depth), nil
 }
 
 func getStringArrStatement(attr string, arrValToIds map[string][]string,
@@ -660,7 +671,7 @@ func parseArtifactNamePattern(
 	return createObject("artifact_name_pattern", fields, depth)
 }
 
-func parseFeature(feature *crosstoolpb.CToolchain_Feature, depth int) string {
+func parseFeature(feature *crosstoolpb.CToolchain_Feature, depth int) (string, error) {
 	name := fmt.Sprintf("name = \"%s\"", feature.GetName())
 
 	fields := []string{name}
@@ -669,9 +680,11 @@ func parseFeature(feature *crosstoolpb.CToolchain_Feature, depth int) string {
 	}
 
 	if len(feature.GetFlagSet()) > 0 {
-		flagSets := "flag_sets = " +
+		flagSets, err := parseFlagSets(feature.GetFlagSet(), depth+1)
-			parseFlagSets(feature.GetFlagSet(), depth+1)
+		if err != nil {
-		fields = append(fields, flagSets)
+			return "", err
+		}
+		fields = append(fields, "flag_sets = "+flagSets)
 	}
 	if len(feature.GetEnvSet()) > 0 {
 		envSets := "env_sets = " + parseEnvSets(feature.GetEnvSet(), depth+1)
@@ -691,19 +704,22 @@ func parseFeature(feature *crosstoolpb.CToolchain_Feature, depth int) string {
 			makeStringArr(feature.GetProvides(), depth+1 /* isPlainString= */, true)
 		fields = append(fields, provides)
 	}
-	return createObject("feature", fields, depth)
+	return createObject("feature", fields, depth), nil
 }
 
-func parseFlagSets(flagSets []*crosstoolpb.CToolchain_FlagSet, depth int) string {
+func parseFlagSets(flagSets []*crosstoolpb.CToolchain_FlagSet, depth int) (string, error) {
 	var res []string
 	for _, flagSet := range flagSets {
-		parsedFlagset := parseFlagSet(flagSet, depth+1)
+		parsedFlagset, err := parseFlagSet(flagSet, depth+1)
+		if err != nil {
+			return "", err
+		}
 		res = append(res, parsedFlagset)
 	}
-	return makeStringArr(res, depth /* isPlainString= */, false)
+	return makeStringArr(res, depth /* isPlainString= */, false), nil
 }
 
-func parseFlagSet(flagSet *crosstoolpb.CToolchain_FlagSet, depth int) string {
+func parseFlagSet(flagSet *crosstoolpb.CToolchain_FlagSet, depth int) (string, error) {
 	var fields []string
 	if len(flagSet.GetAction()) > 0 {
 		actionArr := processActions(flagSet.GetAction(), depth)
@@ -711,27 +727,33 @@ func parseFlagSet(flagSet *crosstoolpb.CToolchain_FlagSet, depth int) string {
 		fields = append(fields, actions)
 	}
 	if len(flagSet.GetFlagGroup()) > 0 {
-		flagGroups := "flag_groups = " +
+		flagGroups, err := parseFlagGroups(flagSet.GetFlagGroup(), depth+1)
-			parseFlagGroups(flagSet.GetFlagGroup(), depth+1)
+		if err != nil {
-		fields = append(fields, flagGroups)
+			return "", err
+		}
+		fields = append(fields, "flag_groups = "+flagGroups)
 	}
 	if len(flagSet.GetWithFeature()) > 0 {
 		withFeatures := "with_features = " +
 			parseWithFeatureSets(flagSet.GetWithFeature(), depth+1)
 		fields = append(fields, withFeatures)
 	}
-	return createObject("flag_set", fields, depth)
+	return createObject("flag_set", fields, depth), nil
 }
 
-func parseFlagGroups(flagGroups []*crosstoolpb.CToolchain_FlagGroup, depth int) string {
+func parseFlagGroups(flagGroups []*crosstoolpb.CToolchain_FlagGroup, depth int) (string, error) {
 	var res []string
 	for _, flagGroup := range flagGroups {
-		res = append(res, parseFlagGroup(flagGroup, depth+1))
+		flagGroupString, err := parseFlagGroup(flagGroup, depth+1)
+		if err != nil {
+			return "", err
+		}
+		res = append(res, flagGroupString)
 	}
-	return makeStringArr(res, depth /* isPlainString= */, false)
+	return makeStringArr(res, depth /* isPlainString= */, false), nil
 }
 
-func parseFlagGroup(flagGroup *crosstoolpb.CToolchain_FlagGroup, depth int) string {
+func parseFlagGroup(flagGroup *crosstoolpb.CToolchain_FlagGroup, depth int) (string, error) {
 	var res []string
 	if len(flagGroup.GetFlag()) != 0 {
 		res = append(res, "flags = "+makeStringArr(flagGroup.GetFlag(), depth+1, true))
@@ -740,7 +762,14 @@ func parseFlagGroup(flagGroup *crosstoolpb.CToolchain_FlagGroup, depth int) stri
 		res = append(res, fmt.Sprintf("iterate_over = \"%s\"", flagGroup.GetIterateOver()))
 	}
 	if len(flagGroup.GetFlagGroup()) != 0 {
-		res = append(res, "flag_groups = "+parseFlagGroups(flagGroup.GetFlagGroup(), depth+1))
+		flagGroupString, err := parseFlagGroups(flagGroup.GetFlagGroup(), depth+1)
+		if err != nil {
+			return "", err
+		}
+		res = append(res, "flag_groups = "+flagGroupString)
+	}
+	if len(flagGroup.GetExpandIfAllAvailable()) > 1 {
+		return "", errors.New("Flag group must not have more than one 'expand_if_all_available' field")
 	}
 	if len(flagGroup.GetExpandIfAllAvailable()) != 0 {
 		res = append(res,
@@ -748,6 +777,9 @@ func parseFlagGroup(flagGroup *crosstoolpb.CToolchain_FlagGroup, depth int) stri
 				"expand_if_available = \"%s\"",
 				flagGroup.GetExpandIfAllAvailable()[0]))
 	}
+	if len(flagGroup.GetExpandIfNoneAvailable()) > 1 {
+		return "", errors.New("Flag group must not have more than one 'expand_if_none_available' field")
+	}
 	if len(flagGroup.GetExpandIfNoneAvailable()) != 0 {
 		res = append(res,
 			fmt.Sprintf(
@@ -767,7 +799,7 @@ func parseFlagGroup(flagGroup *crosstoolpb.CToolchain_FlagGroup, depth int) stri
 			"expand_if_equal = "+parseVariableWithValue(
 				flagGroup.GetExpandIfEqual(), depth+1))
 	}
-	return createObject("flag_group", res, depth)
+	return createObject("flag_group", res, depth), nil
 }
 
 func parseVariableWithValue(variable *crosstoolpb.CToolchain_VariableWithValue, depth int) string {
@@ -1242,9 +1274,15 @@ func Transform(crosstool *crosstoolpb.CrosstoolRelease) (string, error) {
 
 	cToolchainIdentifiers := toolchainToCToolchainIdentifier(crosstool)
 
-	toolchainToFeatures, featureNameToFeature := getFeatures(crosstool)
+	toolchainToFeatures, featureNameToFeature, err := getFeatures(crosstool)
+	if err != nil {
+		return "", err
+	}
 
-	toolchainToActions, actionNameToAction := getActions(crosstool)
+	toolchainToActions, actionNameToAction, err := getActions(crosstool)
+	if err != nil {
+		return "", err
+	}
 
 	header := getCcToolchainConfigHeader()
 	if _, err := b.WriteString(header); err != nil {

tools/migration/crosstool_to_starlark_lib_test.go

@@ -992,106 +992,90 @@ func TestActionConfigListAssignment(t *testing.T) {
 	}
 }
 
-func TestFeatureAssignment(t *testing.T) {
+func TestAllAndNoneAvailableErrorsWhenMoreThanOneElement(t *testing.T) {
-	toolchainEmpty1 := getCToolchain("1", "cpuA", "compilerA", []string{})
+	toolchainFeatureAllAvailable := getCToolchain("1", "cpu", "compiler",
-	toolchainEmpty2 := getCToolchain("2", "cpuB", "compilerA", []string{})
+		[]string{getFeature([]string{
-	toolchainA1 := getCToolchain("3", "cpuC", "compilerA",
+			"name: 'A'",
-		[]string{
+			"flag_set {",
-			getActionConfig([]string{"action_name: 'A'", "config_name: 'A'"}),
+			"  action: 'A'",
-		},
+			"  flag_group {",
+			"    flag: 'f'",
+			"    expand_if_all_available: 'e1'",
+			"    expand_if_all_available: 'e2'",
+			"  }",
+			"}",
+		})},
 	)
-	toolchainA2 := getCToolchain("4", "cpuC", "compilerB",
+	toolchainFeatureNoneAvailable := getCToolchain("1", "cpu", "compiler",
-		[]string{
+		[]string{getFeature([]string{
-			getActionConfig([]string{"action_name: 'A'", "config_name: 'A'"}),
+			"name: 'A'",
-		},
+			"flag_set {",
+			"  action: 'A'",
+			"  flag_group {",
+			"    flag: 'f'",
+			"    expand_if_none_available: 'e1'",
+			"    expand_if_none_available: 'e2'",
+			"  }",
+			"}",
+		})},
 	)
-	toolchainAB := getCToolchain("5", "cpuC", "compilerC",
+	toolchainActionConfigAllAvailable := getCToolchain("1", "cpu", "compiler",
-		[]string{
+		[]string{getActionConfig([]string{
-			getActionConfig([]string{"action_name: 'A'", "config_name: 'A'"}),
+			"config_name: 'A'",
-			getActionConfig([]string{"action_name: 'B'", "config_name: 'B'"}),
+			"action_name: 'A'",
-		},
+			"flag_set {",
+			"  action: 'A'",
+			"  flag_group {",
+			"    flag: 'f'",
+			"    expand_if_all_available: 'e1'",
+			"    expand_if_all_available: 'e2'",
+			"  }",
+			"}",
+		})},
 	)
-	toolchainBA := getCToolchain("6", "cpuD", "compilerA",
+	toolchainActionConfigNoneAvailable := getCToolchain("1", "cpu", "compiler",
-		[]string{
+		[]string{getActionConfig([]string{
-			getActionConfig([]string{"action_name: 'B'", "config_name: 'B'"}),
+			"config_name: 'A'",
-			getActionConfig([]string{"action_name: 'A'", "config_name: 'A'"}),
+			"action_name: 'A'",
-		},
+			"flag_set {",
+			"  action: 'A'",
+			"  flag_group {",
+			"    flag: 'f'",
+			"    expand_if_none_available: 'e1'",
+			"    expand_if_none_available: 'e2'",
+			"  }",
+			"}",
+		})},
 	)
-	toolchainsEmpty := []string{toolchainEmpty1, toolchainEmpty2}
-
-	toolchainsOneNonempty := []string{toolchainEmpty1, toolchainA1}
-
-	toolchainsSameNonempty := []string{toolchainA1, toolchainA2}
-
-	toolchainsDifferentOrder := []string{toolchainAB, toolchainBA}
-
-	allToolchains := []string{
-		toolchainEmpty1,
-		toolchainEmpty2,
-		toolchainA1,
-		toolchainA2,
-		toolchainAB,
-		toolchainBA,
-	}
 
 	testCases := []struct {
 		field        string
-		toolchains   []string
+		toolchain    string
 		expectedText string
 	}{
+		{field: "features",
+			toolchain: toolchainFeatureAllAvailable,
+			expectedText: "Error in feature 'A': Flag group must not have more " +
+				"than one 'expand_if_all_available' field"},
+		{field: "features",
+			toolchain: toolchainFeatureNoneAvailable,
+			expectedText: "Error in feature 'A': Flag group must not have more " +
+				"than one 'expand_if_none_available' field"},
 		{field: "action_configs",
-			toolchains: toolchainsEmpty,
+			toolchain: toolchainActionConfigAllAvailable,
-			expectedText: `
+			expectedText: "Error in action_config 'A': Flag group must not have more " +
-    action_configs = []`},
+				"than one 'expand_if_all_available' field"},
 		{field: "action_configs",
-			toolchains: toolchainsOneNonempty,
+			toolchain: toolchainActionConfigNoneAvailable,
-			expectedText: `
+			expectedText: "Error in action_config 'A': Flag group must not have more " +
-    if (ctx.attr.cpu == "cpuA"):
+				"than one 'expand_if_none_available' field"},
-        action_configs = []
+	}
-    elif (ctx.attr.cpu == "cpuC"):
-        action_configs = [a_action]
-    else:
-        fail("Unreachable")`},
-		{field: "action_configs",
-			toolchains: toolchainsSameNonempty,
-			expectedText: `
-    action_configs = [a_action]`},
-		{field: "action_configs",
-			toolchains: toolchainsDifferentOrder,
-			expectedText: `
-    if (ctx.attr.cpu == "cpuC"):
-        action_configs = [a_action, b_action]
-    elif (ctx.attr.cpu == "cpuD"):
-        action_configs = [b_action, a_action]
-    else:
-        fail("Unreachable")`},
-		{field: "action_configs",
-			toolchains: allToolchains,
-			expectedText: `
-    if (ctx.attr.cpu == "cpuA"
-        or ctx.attr.cpu == "cpuB"):
-        action_configs = []
-    elif (ctx.attr.cpu == "cpuC" and ctx.attr.compiler == "compilerA"
-        or ctx.attr.cpu == "cpuC" and ctx.attr.compiler == "compilerB"):
-        action_configs = [a_action]
-    elif (ctx.attr.cpu == "cpuC" and ctx.attr.compiler == "compilerC"):
-        action_configs = [a_action, b_action]
-    elif (ctx.attr.cpu == "cpuD"):
-        action_configs = [b_action, a_action]
-    else:
-        fail("Unreachable")`}}
 
 	for _, tc := range testCases {
-		crosstool := makeCrosstool(tc.toolchains)
+		crosstool := makeCrosstool([]string{tc.toolchain})
-		got, err := Transform(crosstool)
+		_, err := Transform(crosstool)
-		if err != nil {
+		if err == nil || !strings.Contains(err.Error(), tc.expectedText) {
-			t.Fatalf("CROSSTOOL conversion failed: %v", err)
+			t.Errorf("Expected error: %s, got: %v", tc.expectedText, err)
-		}
-		if !strings.Contains(got, tc.expectedText) {
-			t.Errorf("Failed to correctly convert '%s' field, expected to contain:\n%v\n",
-				tc.field, tc.expectedText)
-			t.Fatalf("Tested CROSSTOOL:\n%v\n\nGenerated rule:\n%v\n",
-				strings.Join(tc.toolchains, "\n"), got)
 		}
 	}
 }