From e02152f86ece24bd757fad5b28ab8fddd570053d Mon Sep 17 00:00:00 2001 From: rosica Date: Wed, 6 Feb 2019 08:55:55 -0800 Subject: [PATCH] Remove fail("Unreachable") from features' and action_configs' declaration statements If toolchain A and B need feature f, but toolchain C doesn't, the generated declaration statement for feature f would be: if ctx.attr.cpu == 'A.cpu': f = feature(...) elif ctx.attr.cpu == "B.cpu" and ctx.attr.compiler == "B.compiler": f = feature(...) else: fail("Unreachable") This will break the rule implementation in the case of toolchain C because it will reach the fail("Unreachable") although it doesn't need to declare feature f This cl fixes that Issue #5380 PiperOrigin-RevId: 232683639 --- tools/migration/crosstool_to_starlark_lib.go | 29 +++++--- .../crosstool_to_starlark_lib_test.go | 67 +++++++++++++++++++ 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/tools/migration/crosstool_to_starlark_lib.go b/tools/migration/crosstool_to_starlark_lib.go index e19b42d..c05e99c 100644 --- a/tools/migration/crosstool_to_starlark_lib.go +++ b/tools/migration/crosstool_to_starlark_lib.go @@ -347,7 +347,7 @@ func getStringStatement(crosstool *crosstoolpb.CrosstoolRelease, mappedValuesToIds := getMappedStringValuesToIdentifiers(identifiers, fieldValues) return getAssignmentStatement(field, mappedValuesToIds, crosstool, - cToolchainIdentifiers, depth /* isPlainString= */, true) + cToolchainIdentifiers, depth /* isPlainString= */, true, /* shouldFail= */ true) } func getFeatures(crosstool *crosstoolpb.CrosstoolRelease) ( @@ -392,7 +392,8 @@ func getFeaturesDeclaration(crosstool *crosstoolpb.CrosstoolRelease, crosstool, cToolchainIdentifiers, depth, - /* isPlainString= */ false)) + /* isPlainString= */ false, + /* shouldFail= */ false)) } return strings.Join(res, "") } @@ -467,7 +468,8 @@ func getActionConfigsDeclaration( crosstool, cToolchainIdentifiers, depth, - /* isPlainString= */ false)) + /* isPlainString= */ false, + /* shouldFail= */ false)) } return strings.Join(res, "") } @@ -650,7 +652,8 @@ func getArtifactNamePatterns(crosstool *crosstoolpb.CrosstoolRelease, crosstool, cToolchainIdentifiers, depth, - /* isPlainString= */ false)) + /* isPlainString= */ false, + /* shouldFail= */ true)) return strings.Join(res, "\n") } @@ -826,7 +829,8 @@ func getToolPaths(crosstool *crosstoolpb.CrosstoolRelease, crosstool, cToolchainIdentifiers, depth, - /* isPlainString= */ false)) + /* isPlainString= */ false, + /* shouldFail= */ true)) return strings.Join(res, "\n") } @@ -865,7 +869,8 @@ func getMakeVariables(crosstool *crosstoolpb.CrosstoolRelease, crosstool, cToolchainIdentifiers, depth, - /* isPlainString= */ false)) + /* isPlainString= */ false, + /* shouldFail= */ true)) return strings.Join(res, "\n") } @@ -1034,7 +1039,7 @@ func makeStringArr(arr []string, depth int, isPlainString bool) string { func getAssignmentStatement(field string, valToIds map[string][]string, crosstool *crosstoolpb.CrosstoolRelease, toCToolchainIdentifier map[string]CToolchainIdentifier, - depth int, isPlainString bool) string { + depth int, isPlainString, shouldFail bool) string { var b bytes.Buffer if len(valToIds) <= 1 { // if there is only one possible value for this field, we don't need if statements @@ -1063,10 +1068,12 @@ func getAssignmentStatement(field string, valToIds map[string][]string, toCToolchainIdentifier, depth, isPlainString)) first = false } - b.WriteString( - fmt.Sprintf( - "%selse:\n%sfail(\"Unreachable\")\n", - getTabs(depth), getTabs(depth+1))) + if shouldFail { + b.WriteString( + fmt.Sprintf( + "%selse:\n%sfail(\"Unreachable\")\n", + getTabs(depth), getTabs(depth+1))) + } } b.WriteString("\n") return b.String() diff --git a/tools/migration/crosstool_to_starlark_lib_test.go b/tools/migration/crosstool_to_starlark_lib_test.go index 56f208f..0090ce8 100644 --- a/tools/migration/crosstool_to_starlark_lib_test.go +++ b/tools/migration/crosstool_to_starlark_lib_test.go @@ -1080,6 +1080,73 @@ func TestAllAndNoneAvailableErrorsWhenMoreThanOneElement(t *testing.T) { } } +func TestNoFailUnreachableInFeaturesAndActionConfigsDeclaration(t *testing.T) { + toolchainFeatureAEnabled := getCToolchain("1", "cpuA", "compilerA", + []string{getFeature([]string{"name: 'A'", "enabled: true"})}, + ) + toolchainFeatureADisabled := getCToolchain("2", "cpuA", "compilerB", + []string{getFeature([]string{"name: 'A'", "enabled: false"})}, + ) + + toolchainWithoutFeatureA := getCToolchain("3", "cpuA", "compilerC", []string{}) + + toolchainActionConfigAEnabled := getCToolchain("4", "cpuA", "compilerD", + []string{getActionConfig([]string{ + "config_name: 'A'", + "action_name: 'A'", + "enabled: true", + })}) + + toolchainActionConfigADisabled := getCToolchain("5", "cpuA", "compilerE", + []string{getActionConfig([]string{ + "config_name: 'A'", + "action_name: 'A'", + })}) + + toolchainWithoutActionConfigA := getCToolchain("6", "cpuA", "compilerF", []string{}) + + testCases := []struct { + field string + toolchains []string + expectedText string + }{ + {field: "features", + toolchains: []string{ + toolchainFeatureAEnabled, toolchainFeatureADisabled, toolchainWithoutFeatureA}, + expectedText: ` + if (ctx.attr.cpu == "cpuA" and ctx.attr.compiler == "compilerB"): + a_feature = feature(name = "A") + elif (ctx.attr.cpu == "cpuA" and ctx.attr.compiler == "compilerA"): + a_feature = feature(name = "A", enabled = True) + +` /* empty line after the elif means there's no else statement */}, + {field: "action_config", + toolchains: []string{ + toolchainActionConfigAEnabled, toolchainActionConfigADisabled, toolchainWithoutActionConfigA}, + expectedText: ` + if (ctx.attr.cpu == "cpuA" and ctx.attr.compiler == "compilerE"): + a_action = action_config(action_name = "A") + elif (ctx.attr.cpu == "cpuA" and ctx.attr.compiler == "compilerD"): + a_action = action_config(action_name = "A", enabled = True) + +` /* empty line after the elif means there's no else statement */ }, + } + + for _, tc := range testCases { + crosstool := makeCrosstool(tc.toolchains) + got, err := Transform(crosstool) + if err != nil { + t.Fatalf("CROSSTOOL conversion failed: %v", 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) + } + } +} + func TestActionConfigDeclaration(t *testing.T) { toolchainEmpty1 := getCToolchain("1", "cpuA", "compilerA", []string{}) toolchainEmpty2 := getCToolchain("2", "cpuB", "compilerA", []string{})