refactor(buildtool): improve how we set environment variables

internal/cmd/buildtool/android.go

@@ -123,6 +123,7 @@ func androidBuildCLIAll(deps buildtoolmodel.Dependencies) {
 				deps,
 				product,
 				arch,
+				androidHome,
 				ndkDir,
 			)
 		}
@@ -134,9 +135,10 @@ func androidBuildCLIProductArch(
 	deps buildtoolmodel.Dependencies,
 	product *product,
 	ooniArch string,
+	androidHome string,
 	ndkDir string,
 ) {
-	cgo := newAndroidCBuildEnv(ndkDir, ooniArch)
+	cgo := newAndroidCBuildEnv(androidHome, ndkDir, ooniArch)
 
 	log.Infof("building %s for android/%s", product.Pkg, ooniArch)
 
@@ -148,6 +150,9 @@ func androidBuildCLIProductArch(
 	argv.Append("-o", product.DestinationPath("android", ooniArch))
 	argv.Append(product.Pkg)
 
+	// For more complex use cases such as building cdeps we have dedicated
+	// extracting functions (e.g., cBuildExportAutotools), but this code is
+	// simple enough that it's OK to keep here without refactoring.
 	envp := &shellx.Envp{}
 	envp.Append("CGO_ENABLED", "1")
 	envp.Append("CC", cgo.CC)
@@ -175,40 +180,58 @@ func androidBuildCLIProductArch(
 
 // newAndroidCBuildEnv creates a new [cBuildEnv] for the
 // given ooniArch ("arm", "arm64", "386", "amd64").
-func newAndroidCBuildEnv(ndkDir string, ooniArch string) *cBuildEnv {
+func newAndroidCBuildEnv(androidHome, ndkDir, ooniArch string) *cBuildEnv {
+	binpath := androidNDKBinPath(ndkDir)
+	destdir := runtimex.Try1(filepath.Abs(filepath.Join( // must be absolute
+		"internal", "libtor", "android", ooniArch,
+	)))
 	out := &cBuildEnv{
-		BINPATH:  androidNDKBinPath(ndkDir),
-		CC:       "",
-		CFLAGS:   androidCflags(ooniArch),
-		CXX:      "",
-		CXXFLAGS: androidCflags(ooniArch),
-		GOARCH:   "",
-		GOARM:    "",
+		ANDROID_HOME:       androidHome,
+		ANDROID_NDK_HOME:   ndkDir,
+		AS:                 "", // later
+		AR:                 filepath.Join(binpath, "llvm-ar"),
+		BINPATH:            binpath,
+		CC:                 "", // later
+		CFLAGS:             androidCflags(ooniArch),
+		CONFIGURE_HOST:     "", // later
+		DESTDIR:            destdir,
+		CXX:                "", // later
+		CXXFLAGS:           androidCflags(ooniArch),
+		GOARCH:             ooniArch,
+		GOARM:              "", // maybe later
+		LD:                 filepath.Join(binpath, "ld"),
+		LDFLAGS:            []string{}, // empty
+		OPENSSL_API_DEFINE: "-D__ANDROID_API__=21",
+		OPENSSL_COMPILER:   "", // later
+		RANLIB:             filepath.Join(binpath, "llvm-ranlib"),
+		STRIP:              filepath.Join(binpath, "llvm-strip"),
 	}
 	switch ooniArch {
 	case "arm":
 		out.CC = filepath.Join(out.BINPATH, "armv7a-linux-androideabi21-clang")
 		out.CXX = filepath.Join(out.BINPATH, "armv7a-linux-androideabi21-clang++")
-		out.GOARCH = ooniArch
 		out.GOARM = "7"
+		out.CONFIGURE_HOST = "arm-linux-androideabi"
+		out.OPENSSL_COMPILER = "android-arm"
 	case "arm64":
 		out.CC = filepath.Join(out.BINPATH, "aarch64-linux-android21-clang")
 		out.CXX = filepath.Join(out.BINPATH, "aarch64-linux-android21-clang++")
-		out.GOARCH = ooniArch
-		out.GOARM = ""
+		out.CONFIGURE_HOST = "aarch64-linux-android"
+		out.OPENSSL_COMPILER = "android-arm64"
 	case "386":
 		out.CC = filepath.Join(out.BINPATH, "i686-linux-android21-clang")
 		out.CXX = filepath.Join(out.BINPATH, "i686-linux-android21-clang++")
-		out.GOARCH = ooniArch
-		out.GOARM = ""
+		out.CONFIGURE_HOST = "i686-linux-android"
+		out.OPENSSL_COMPILER = "android-x86"
 	case "amd64":
 		out.CC = filepath.Join(out.BINPATH, "x86_64-linux-android21-clang")
 		out.CXX = filepath.Join(out.BINPATH, "x86_64-linux-android21-clang++")
-		out.GOARCH = ooniArch
-		out.GOARM = ""
+		out.CONFIGURE_HOST = "x86_64-linux-android"
+		out.OPENSSL_COMPILER = "android-x86_64"
 	default:
 		panic(errors.New("unsupported ooniArch"))
 	}
+	out.AS = out.CC
 	return out
 }
 

internal/cmd/buildtool/cbuildenv.go

@@ -16,6 +16,18 @@ import (
 // this structure as the enviroment variables you would use and export
 // in a bash script (hence the all uppercase naming).
 type cBuildEnv struct {
+	// ANDROID_HOME is the android home variable.
+	ANDROID_HOME string
+
+	// ANDROID_NDK_HOME is the android NDK home variable.
+	ANDROID_NDK_HOME string
+
+	// AS is the full path to the assembler.
+	AS string
+
+	// AR is the full path to the ar tool.
+	AR string
+
 	// BINPATH is the path containing the C and C++ compilers.
 	BINPATH string
 
@@ -43,40 +55,96 @@ type cBuildEnv struct {
 	// GOARM is the GOARM subarchitecture.
 	GOARM string
 
-	// lfdlags contains the LDFLAGS to use when compiling.
+	// LD is the full path to the linker.
+	LD string
+
+	// LDFLAGS contains the LDFLAGS to use when compiling.
 	LDFLAGS []string
 
 	// OPENSSL_API_DEFINE is an extra define we need to add on Android.
 	OPENSSL_API_DEFINE string
 
 	// OPENSSL_COMPILER is the compiler name for OpenSSL.
 	OPENSSL_COMPILER string
+
+	// RANLIB is the path to the ranlib tool.
+	RANLIB string
+
+	// STRIP is the path to the strip tool.
+	STRIP string
 }
 
-// cBuildExportEnviron merges the gloval and the local c build environment
-// to produce the environment variables to export for the build. More specifically,
-// this appends the local variables to the remote variables for any string slice
-// type inside [cBuildEnv]. We ignore all the other variables.
-func cBuildExportEnviron(global, local *cBuildEnv) *shellx.Envp {
-	envp := &shellx.Envp{}
-
-	cflags := append([]string{}, global.CFLAGS...)
-	cflags = append(cflags, local.CFLAGS...)
-	if len(cflags) > 0 {
-		envp.Append("CFLAGS", strings.Join(cflags, " "))
+// cBuildMerge merges the global and the local [cBuildEnv] to produce a
+// new [cBuildEnv] where the following holds:
+//
+// 1. all the scalar variables come from the global one;
+//
+// 2. all the slice variables are the ones in the global one with
+// appended the ones in the local one.
+//
+// This kind of merging allows a build rule to include more
+// environment variables to CFLAGS, CXXFLAGS, etc.
+func cBuildMerge(global, local *cBuildEnv) *cBuildEnv {
+	out := &cBuildEnv{
+		ANDROID_HOME:       global.ANDROID_HOME,
+		ANDROID_NDK_HOME:   global.ANDROID_NDK_HOME,
+		AR:                 global.AR,
+		AS:                 global.AS,
+		BINPATH:            global.BINPATH,
+		CC:                 global.CC,
+		CFLAGS:             append([]string{}, global.CFLAGS...),
+		CONFIGURE_HOST:     global.CONFIGURE_HOST,
+		DESTDIR:            global.DESTDIR,
+		CXX:                global.CXX,
+		CXXFLAGS:           append([]string{}, global.CXXFLAGS...),
+		GOARCH:             global.GOARCH,
+		GOARM:              global.GOARM,
+		LD:                 global.LD,
+		LDFLAGS:            append([]string{}, global.LDFLAGS...),
+		OPENSSL_API_DEFINE: global.OPENSSL_API_DEFINE,
+		OPENSSL_COMPILER:   global.OPENSSL_COMPILER,
+		RANLIB:             global.RANLIB,
+		STRIP:              global.STRIP,
 	}
+	out.CFLAGS = append(out.CFLAGS, local.CFLAGS...)
+	out.CXXFLAGS = append(out.CXXFLAGS, local.CXXFLAGS...)
+	out.LDFLAGS = append(out.LDFLAGS, local.LDFLAGS...)
+	return out
+}
 
-	cxxflags := append([]string{}, global.CXXFLAGS...)
-	cxxflags = append(cxxflags, local.CXXFLAGS...)
-	if len(cxxflags) > 0 {
-		envp.Append("CXXFLAGS", strings.Join(cxxflags, " "))
+// cBuildMaybeAppendScalar is a convenience function for appending a
+// scalar environment variable to out.
+func cBuildMaybeAppend(out *shellx.Envp, name, value string) {
+	if value != "" {
+		out.Append(name, value)
 	}
+}
 
-	ldflags := append([]string{}, global.LDFLAGS...)
-	ldflags = append(ldflags, local.LDFLAGS...)
-	if len(ldflags) > 0 {
-		envp.Append("LDFLAGS", strings.Join(ldflags, " "))
-	}
+// cBuildExportAutotools exports all the environment variables
+// inside the given [cBuildEnv] required by autotools builds.
+func cBuildExportAutotools(env *cBuildEnv) *shellx.Envp {
+	out := &shellx.Envp{}
+	cBuildMaybeAppend(out, "AR", env.AR)
+	cBuildMaybeAppend(out, "AS", env.AS)
+	cBuildMaybeAppend(out, "CC", env.CC)
+	cBuildMaybeAppend(out, "CFLAGS", strings.Join(env.CFLAGS, " "))
+	cBuildMaybeAppend(out, "CXX", env.CXX)
+	cBuildMaybeAppend(out, "CXXFLAGS", strings.Join(env.CXXFLAGS, " "))
+	cBuildMaybeAppend(out, "LD", env.LD)
+	cBuildMaybeAppend(out, "LDFLAGS", strings.Join(env.LDFLAGS, " "))
+	cBuildMaybeAppend(out, "RANLIB", env.RANLIB)
+	cBuildMaybeAppend(out, "STRIP", env.STRIP)
+	return out
+}
 
-	return envp
+// cBuildExportOpenSSL exports all the environment variables
+// inside the given [cBuildEnv] required by OpenSSL builds.
+func cBuildExportOpenSSL(env *cBuildEnv) *shellx.Envp {
+	out := &shellx.Envp{}
+	cBuildMaybeAppend(out, "ANDROID_HOME", env.ANDROID_HOME)
+	cBuildMaybeAppend(out, "ANDROID_NDK_HOME", env.ANDROID_NDK_HOME)
+	cBuildMaybeAppend(out, "CFLAGS", strings.Join(env.CFLAGS, " "))
+	cBuildMaybeAppend(out, "CXXFLAGS", strings.Join(env.CXXFLAGS, " "))
+	cBuildMaybeAppend(out, "LDFLAGS", strings.Join(env.LDFLAGS, " "))
+	return out
 }

internal/cmd/buildtool/cbuildenv_test.go

@@ -1,49 +1,121 @@
 package main
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/shellx"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 )
 
-func TestCBuildEnv(t *testing.T) {
-	t.Run("we can correctly merge build flags", func(t *testing.T) {
-		global := &cBuildEnv{
-			CFLAGS:   []string{"a", "b", "c"},
-			CXXFLAGS: []string{"A", "B", "C"},
-			LDFLAGS:  []string{"1", "2", "3"},
-		}
-		local := &cBuildEnv{
-			CFLAGS:   []string{"d", "e"},
-			CXXFLAGS: []string{"D", "E"},
-			LDFLAGS:  []string{"4", "5"},
-		}
-		envp := cBuildExportEnviron(global, local)
-		expected := []string{
-			"CFLAGS=a b c d e",
-			"CXXFLAGS=A B C D E",
-			"LDFLAGS=1 2 3 4 5",
-		}
-		if diff := cmp.Diff(expected, envp.V); diff != "" {
-			t.Fatal(diff)
+func TestCBuildMerge(t *testing.T) {
+	t.Run("we correctly merge slice fields", func(t *testing.T) {
+		ff := &testingx.FakeFiller{}
+		global := &cBuildEnv{}
+		ff.Fill(global)
+		local := &cBuildEnv{}
+		ff.Fill(local)
+
+		merged := cBuildMerge(global, local)
+		typeinfo := reflect.TypeOf(merged).Elem()
+
+		for idx := 0; idx < typeinfo.NumField(); idx++ {
+			field := reflect.ValueOf(merged).Elem().Field(idx)
+			switch got := field.Interface().(type) {
+			case []string:
+				gvalue := reflect.ValueOf(global).Elem().Field(idx).Interface().([]string)
+				lvalue := reflect.ValueOf(local).Elem().Field(idx).Interface().([]string)
+				expect := append([]string{}, gvalue...)
+				expect = append(expect, lvalue...)
+				if diff := cmp.Diff(expect, got); diff != "" {
+					name := typeinfo.Field(idx).Name
+					t.Errorf("field %s: expected %v, got %v", name, expect, got)
+				}
+			default:
+				// nothing
+			}
 		}
 	})
 
-	t.Run("we do nothing with empty variables", func(t *testing.T) {
-		global := &cBuildEnv{
-			CFLAGS:   []string{},
-			CXXFLAGS: []string{},
-			LDFLAGS:  []string{},
-		}
-		local := &cBuildEnv{
-			CFLAGS:   []string{},
-			CXXFLAGS: []string{},
-			LDFLAGS:  []string{},
-		}
-		envp := cBuildExportEnviron(global, local)
-		var expected []string
-		if diff := cmp.Diff(expected, envp.V); diff != "" {
-			t.Fatal(diff)
+	t.Run("we correctly copy scalar fields", func(t *testing.T) {
+		ff := &testingx.FakeFiller{}
+		global := &cBuildEnv{}
+		ff.Fill(global)
+		local := &cBuildEnv{}
+		ff.Fill(local)
+
+		merged := cBuildMerge(global, local)
+		typeinfo := reflect.TypeOf(merged).Elem()
+
+		for idx := 0; idx < typeinfo.NumField(); idx++ {
+			field := reflect.ValueOf(merged).Elem().Field(idx)
+			switch got := field.Interface().(type) {
+			case string:
+				gvalue := reflect.ValueOf(global).Elem().Field(idx).Interface().(string)
+				if diff := cmp.Diff(gvalue, got); diff != "" {
+					name := typeinfo.Field(idx).Name
+					t.Errorf("field %s: expected %v, got %v", name, gvalue, got)
+				}
+			default:
+				// nothing
+			}
 		}
 	})
 }
+
+func TestCBuildExportAutotools(t *testing.T) {
+	global := &cBuildEnv{
+		AR:       "ar",
+		AS:       "gas",
+		CC:       "clang",
+		CFLAGS:   []string{"-Wall", "-Wextra"},
+		CXX:      "clang++",
+		CXXFLAGS: []string{"-Wall", "-Wextra", "-std=c++11"},
+		LD:       "ld",
+		LDFLAGS:  []string{"-L/usr/local/lib"},
+		RANLIB:   "ranlib",
+		STRIP:    "strip",
+	}
+	expect := &shellx.Envp{
+		V: []string{
+			"AR=ar",
+			"AS=gas",
+			"CC=clang",
+			"CFLAGS=-Wall -Wextra",
+			"CXX=clang++",
+			"CXXFLAGS=-Wall -Wextra -std=c++11",
+			"LD=ld",
+			"LDFLAGS=-L/usr/local/lib",
+			"RANLIB=ranlib",
+			"STRIP=strip",
+		},
+	}
+	got := cBuildExportAutotools(global)
+	if diff := cmp.Diff(expect, got); diff != "" {
+		t.Fatal(diff)
+	}
+}
+
+func TestCBuildExportOpenSSL(t *testing.T) {
+	global := &cBuildEnv{
+		ANDROID_HOME:     "/android",
+		ANDROID_NDK_HOME: "/android/sdk/ndk",
+		CFLAGS:           []string{"-Wall", "-Wextra"},
+		CXXFLAGS:         []string{"-Wall", "-Wextra", "-std=c++11"},
+		LDFLAGS:          []string{"-L/usr/local/lib"},
+	}
+	expect := &shellx.Envp{
+		V: []string{
+			"ANDROID_HOME=/android",
+			"ANDROID_NDK_HOME=/android/sdk/ndk",
+			"CFLAGS=-Wall -Wextra",
+			"CXXFLAGS=-Wall -Wextra -std=c++11",
+			"LDFLAGS=-L/usr/local/lib",
+		},
+	}
+	got := cBuildExportOpenSSL(global)
+	if diff := cmp.Diff(expect, got); diff != "" {
+		t.Fatal(diff)
+	}
+}