[fint] Initialize zircon_extra_args
It needs to be initialized to an empty scope, if not already set, to
allow other GN args to concisely set values within the scope (e.g.
`zircon_extra_args.foo = "bar"`). A corresponding recipe change was
made in fxrev.dev/439695.
I also added a test to make sure that the GN args are correctly ordered:
`zircon_extra_args` must be set after all imports (some imported
files assume that it hasn't yet been set when they are loaded into the
args namespace) but before any other variables are set.
Making assertions about ordering made the `assertContains` (now renamed
to `assertSubset`) helper function significantly more complicated, so I
ended up writing tests for the test helper function itself.
Bug: 61997
Change-Id: Ibd45e5e27f23e859573461fc91bbf785cbbce7e1
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/443317
Reviewed-by: Gary Miguel <garymm@google.com>
Testability-Review: Gary Miguel <garymm@google.com>
Commit-Queue: Oliver Newman <olivernewman@google.com>
diff --git a/tools/integration/cmd/fint/set.go b/tools/integration/cmd/fint/set.go
index e103f3f..ffd83b4 100644
--- a/tools/integration/cmd/fint/set.go
+++ b/tools/integration/cmd/fint/set.go
@@ -164,10 +164,6 @@
if staticSpec.UseGoma {
return nil, fmt.Errorf("goma is not supported for builds using a custom gcc toolchain")
}
- // TODO(olivernewman): Do we need to check if `zircon_extra_args` is
- // undefined and initialize it to an empty scope if so? The recipes pass
- // this as an arg, but it's ugly and may be unnecessary:
- // `if (!defined(zircon_extra_args)) { zircon_extra_args = {} }`
vars["zircon_extra_args.gcc_tool_dir"] = filepath.Join(contextSpec.GccToolchainDir, "bin")
}
if contextSpec.RustToolchainDir != "" {
@@ -223,8 +219,8 @@
continue
}
// If product is set, append to the corresponding list variable instead
- // of overwriting it to avoid overwriting the packages set by the
- // product.
+ // of overwriting it to avoid overwriting any packages set in the
+ // imported product file.
// TODO(olivernewman): Is it safe to always append whether or not
// product is set?
if staticSpec.Product == "" {
@@ -245,24 +241,43 @@
// if staticSpec.CollectMetrics is set, once we have a plan for exposing the
// tracelog's location to the recipe for upload.
- var args []string
- args = append(args, staticSpec.GnArgs...)
+ var normalArgs []string
+ var importArgs []string
+ for _, arg := range staticSpec.GnArgs {
+ if strings.HasPrefix(arg, "import(") {
+ importArgs = append(importArgs, arg)
+ } else {
+ normalArgs = append(normalArgs, arg)
+ }
+ }
for k, v := range vars {
- args = append(args, fmt.Sprintf("%s=%s", k, toGNValue(v)))
+ normalArgs = append(normalArgs, fmt.Sprintf("%s=%s", k, toGNValue(v)))
}
for k, v := range appends {
- args = append(args, fmt.Sprintf("%s+=%s", k, toGNValue(v)))
+ normalArgs = append(normalArgs, fmt.Sprintf("%s+=%s", k, toGNValue(v)))
}
+ sort.Strings(normalArgs)
- sort.Strings(imports)
- var importArgs []string
for _, p := range imports {
importArgs = append(importArgs, fmt.Sprintf(`import("//%s")`, p))
}
+ sort.Strings(importArgs)
- sort.Strings(args)
- return append(importArgs, args...), nil
+ var finalArgs []string
+
+ // Ensure that imports come before args that set or modify variables, as
+ // otherwise the imported files might blindly redefine variables set or
+ // modified by other arguments.
+ finalArgs = append(finalArgs, importArgs...)
+ // Initialize `zircon_extra_args` before any variable-setting args, so that
+ // it's safe for subsequent args to do things like `zircon_extra_args.foo =
+ // "bar"` without worrying about initializing zircon_extra_args if it hasn't
+ // yet been defined. But do it after all imports in case one of the imported
+ // files sets `zircon_extra_args`.
+ finalArgs = append(finalArgs, "if (!defined(zircon_extra_args)) { zircon_extra_args = {} }")
+ finalArgs = append(finalArgs, normalArgs...)
+ return finalArgs, nil
}
// toGNValue converts a Go value to a string representation of the corresponding
diff --git a/tools/integration/cmd/fint/set_test.go b/tools/integration/cmd/fint/set_test.go
index 043a8a0..a8cde90 100644
--- a/tools/integration/cmd/fint/set_test.go
+++ b/tools/integration/cmd/fint/set_test.go
@@ -97,18 +97,6 @@
})
}
-func assertContains(t *testing.T, list []string, targets []string) {
- seen := make(map[string]bool)
- for _, item := range list {
- seen[item] = true
- }
- for _, target := range targets {
- if !seen[target] {
- t.Errorf("Expected to find %q in %+v", target, list)
- }
- }
-}
-
func TestGenArgs(t *testing.T) {
platform := "linux-x64"
// A magic string that will be replaced with the actual path to a mock
@@ -122,6 +110,9 @@
// Args that are expected to be included in the return value. Order does
// not matter.
expectedArgs []string
+ // Whether `expectedArgs` must be found in the same relative order in
+ // the return value. Disabled by default to make tests less fragile.
+ orderMatters bool
// Whether we expect genArgs to return an error.
expectErr bool
// Relative paths to files to create in the checkout dir prior to
@@ -282,6 +273,22 @@
},
expectedArgs: []string{`sdk_id="789"`, `build_sdk_archives=true`},
},
+ {
+ name: "sorts imports first",
+ staticSpec: &fintpb.Static{
+ GnArgs: []string{`foo="bar"`, `import("//foo.gni")`},
+ Product: "products/core.gni",
+ },
+ expectedArgs: []string{
+ `import("//foo.gni")`,
+ `import("//products/core.gni")`,
+ // This must come after all imports but before any other variables.
+ `if (!defined(zircon_extra_args)) { zircon_extra_args = {} }`,
+ `build_info_product="core"`,
+ `foo="bar"`,
+ },
+ orderMatters: true,
+ },
}
for _, tc := range testCases {
@@ -323,7 +330,114 @@
t.Fatalf("Expected genArgs() to return an error, but got nil")
}
- assertContains(t, args, tc.expectedArgs)
+ assertSubset(t, tc.expectedArgs, args, tc.orderMatters)
+ })
+ }
+}
+
+// assertSubset checks that every item in `subset` is also in `set`. If
+// `orderMatters`, then we'll also check that the relative ordering of the items
+// in `subset` is the same as their relative ordering in `set`.
+func assertSubset(t *testing.T, subset, set []string, orderMatters bool) {
+ if isSub, msg := isSubset(subset, set, orderMatters); !isSub {
+ t.Fatalf(msg)
+ }
+}
+
+// isSubset is extracted from `assertSubset()` to make it possible to test this
+// logic.
+func isSubset(subset, set []string, orderMatters bool) (bool, string) {
+ indices := make(map[string]int)
+ for i, item := range set {
+ if duplicateIndex, ok := indices[item]; ok {
+ // Disallowing duplicates makes this function simpler, and we have
+ // no need to handle duplicates.
+ return false, fmt.Sprintf("Duplicate item %q found at indices %d and %d", item, duplicateIndex, i)
+ }
+ indices[item] = i
+ }
+
+ var previousIndex int
+ for i, target := range subset {
+ index, ok := indices[target]
+ if !ok {
+ return false, fmt.Sprintf("Expected to find %q in %+v", target, set)
+ } else if orderMatters && index < previousIndex {
+ return false, fmt.Sprintf("Expected %q to precede %q, but it came after", subset[i-1], target)
+ }
+ previousIndex = index
+ }
+ return true, ""
+}
+
+func TestAssertSubset(t *testing.T) {
+ testCases := []struct {
+ name string
+ subset []string
+ set []string
+ orderMatters bool
+ expectFailure bool
+ }{
+ {
+ name: "empty subset and set",
+ subset: []string{},
+ set: []string{},
+ },
+ {
+ name: "empty subset",
+ subset: []string{},
+ set: []string{"foo"},
+ },
+ {
+ name: "empty set",
+ subset: []string{"foo"},
+ set: []string{},
+ expectFailure: true,
+ },
+ {
+ name: "non-empty and equal",
+ subset: []string{"foo", "bar"},
+ set: []string{"foo", "bar"},
+ },
+ {
+ name: "non-empty strict subset",
+ subset: []string{"foo"},
+ set: []string{"foo", "bar"},
+ },
+ {
+ name: "one item missing from set",
+ subset: []string{"foo", "bar", "baz"},
+ set: []string{"foo", "bar"},
+ expectFailure: true,
+ },
+ {
+ name: "order does not matter",
+ subset: []string{"foo", "bar"},
+ set: []string{"bar", "foo"},
+ },
+ {
+ name: "order matters if specified",
+ subset: []string{"foo", "bar"},
+ set: []string{"bar", "foo"},
+ orderMatters: true,
+ expectFailure: true,
+ },
+ {
+ name: "duplicate in set",
+ subset: []string{"foo"},
+ set: []string{"foo", "foo"},
+ expectFailure: true,
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ isSub, msg := isSubset(tc.subset, tc.set, tc.orderMatters)
+ if tc.expectFailure && isSub {
+ t.Errorf("Expected assertSubset() to fail but it passed")
+ } else if !tc.expectFailure && !isSub {
+ t.Errorf("Expected assertSubset() to pass but it failed: %s", msg)
+ }
})
}
}
