Two-way variable replacement: change path separators to target platform

Two-way variable replacement now also changes the path separators. Since
the two-way replacement is made for paths, it makes sense to also clean up
the path for the target platform.

CHANGELOG.md

@@ -13,6 +13,7 @@ bugs in actually-released versions.
 - Change path inside the Linux and macOS tarballs, so that they contain an
   embedded `flamenco-3.x.y-xxxx/` directory with all the files (instead of
   putting all the files in the root of the tarball).
+- Two-way variable replacement now also changes the path separators to the target platform.
 
 
 ## 3.0-beta1 - released 2022-08-03

internal/manager/api_impl/varrepl_test.go

@@ -87,14 +87,14 @@ func TestReplacePathsWindows(t *testing.T) {
 	replacedTask := replaceTaskVariables(&conf, task, worker)
 
 	assert.Equal(t,
-		"s:/flamenco/jobs/sybren/2017-06-08-181223.625800-sybren-flamenco-test.flamenco/flamenco-test.flamenco.blend",
+		`s:\flamenco\jobs\sybren\2017-06-08-181223.625800-sybren-flamenco-test.flamenco\flamenco-test.flamenco.blend`,
 		replacedTask.Commands[2].Parameters["filepath"],
 	)
 	assert.Equal(t,
-		[]string{"--render-out", "s:/flamenco/render/long/sybren/blender-cloud-addon/flamenco-test__intermediate/render-smpl-0001-0084-frm-######"},
+		[]string{"--render-out", `s:\flamenco\render\long\sybren\blender-cloud-addon\flamenco-test__intermediate\render-smpl-0001-0084-frm-######`},
 		replacedTask.Commands[2].Parameters["args"],
 	)
-	assert.Equal(t, "{hey}/haha", replacedTask.Commands[2].Parameters["otherpath"])
+	assert.Equal(t, `{hey}\haha`, replacedTask.Commands[2].Parameters["otherpath"])
 }
 
 func TestReplacePathsUnknownOS(t *testing.T) {

internal/manager/config/config.go

@@ -428,10 +428,12 @@ func (c *Conf) ExpandVariables(inputChannel <-chan string, outputChannel chan<-
 	}
 
 	doValueReplacement := func(valueToExpand string) string {
+		expanded := valueToExpand
+
 		// Expand variables from {varname} to their value for the target platform.
 		for varname, varvalue := range varsForPlatform {
 			placeholder := fmt.Sprintf("{%s}", varname)
-			valueToExpand = strings.Replace(valueToExpand, placeholder, varvalue, -1)
+			expanded = strings.Replace(expanded, placeholder, varvalue, -1)
 		}
 
 		// Go through the two-way variables, to make sure that the result of
@@ -446,13 +448,16 @@ func (c *Conf) ExpandVariables(inputChannel <-chan string, outputChannel chan<-
 			if !ok {
 				continue
 			}
-			if !strings.HasPrefix(valueToExpand, managerValue) {
+			if !strings.HasPrefix(expanded, managerValue) {
 				continue
 			}
-			valueToExpand = targetValue + valueToExpand[len(managerValue):]
+			expanded = targetValue + expanded[len(managerValue):]
 		}
 
-		return valueToExpand
+		// Since two-way variables are meant for path replacement, we know this
+		// should be a path. For added bonus, translate it to the target platform's
+		// path separators.
+		return crosspath.ToPlatform(expanded, string(platform))
 	}
 
 	for valueToExpand := range inputChannel {

internal/manager/config/settings_test.go

@@ -109,7 +109,7 @@ func TestExpandVariables(t *testing.T) {
 			Values: []VariableValue{
 				{Value: "/path/to/ffmpeg", Audience: VariableAudienceUsers, Platform: VariablePlatformLinux},
 				{Value: "/path/to/ffmpeg/on/darwin", Audience: VariableAudienceUsers, Platform: VariablePlatformDarwin},
-				{Value: "C:/flamenco/ffmpeg", Audience: VariableAudienceUsers, Platform: VariablePlatformWindows},
+				{Value: `C:\flamenco\ffmpeg`, Audience: VariableAudienceUsers, Platform: VariablePlatformWindows},
 			},
 		}
 	})
@@ -128,7 +128,7 @@ func TestExpandVariables(t *testing.T) {
 	assert.Equal(t, "unchanged value", <-receiver)
 
 	feeder <- "{ffmpeg}"
-	assert.Equal(t, "C:/flamenco/ffmpeg", <-receiver)
+	assert.Equal(t, `C:\flamenco\ffmpeg`, <-receiver)
 
 	feeder <- "{demo}"
 	assert.Equal(t, "{demo}", <-receiver, "missing value on the platform should not be replaced")
@@ -156,7 +156,7 @@ func TestExpandVariablesWithTwoWay(t *testing.T) {
 			Values: []VariableValue{
 				{Value: "/path/on/linux", Platform: VariablePlatformLinux, Audience: VariableAudienceWorkers},
 				{Value: "/path/on/darwin", Platform: VariablePlatformDarwin, Audience: VariableAudienceWorkers},
-				{Value: "C:/path/on/windows", Platform: VariablePlatformWindows, Audience: VariableAudienceWorkers},
+				{Value: `C:\path\on\windows`, Platform: VariablePlatformWindows, Audience: VariableAudienceWorkers},
 			},
 		}
 	})
@@ -174,11 +174,11 @@ func TestExpandVariablesWithTwoWay(t *testing.T) {
 
 	// Simple two-way variable replacement.
 	feeder <- "/path/on/linux/file.txt"
-	assert.Equal(t, "C:/path/on/windows/file.txt", <-receiver)
+	assert.Equal(t, `C:\path\on\windows\file.txt`, <-receiver)
 
 	// {locally-set-path} expands to a value that's then further replaced by a two-way variable.
 	feeder <- "{locally-set-path}/should/be/remapped"
-	assert.Equal(t, "C:/path/on/windows/should/be/remapped", <-receiver)
+	assert.Equal(t, `C:\path\on\windows\should\be\remapped`, <-receiver)
 
 	close(feeder)
 	wg.Wait()

pkg/crosspath/crosspath.go

@@ -104,3 +104,48 @@ func IsRoot(path string) bool {
 	runes := []rune(path)
 	return validDriveLetter(runes[0])
 }
+
+// ToPlatform returns the path, with path separators adjusted for the given platform.
+// It is assumed that all forward and backward slashes in the path are path
+// separators, and that apart from the style of separators the path makes sense
+// on the target platform.
+func ToPlatform(path, platform string) string {
+	if path == "" {
+		return ""
+	}
+
+	components := strings.FieldsFunc(path, isPathSep)
+
+	// FieldsFunc() removes leading path separators, turning an absolute path on
+	// Linux to a relative path, and turning `\\NAS\share` on Windows into
+	// `NAS\share`.
+	extraComponents := []string{}
+	for _, r := range path {
+		if !isPathSep(r) {
+			break
+		}
+		extraComponents = append(extraComponents, "")
+	}
+	components = append(extraComponents, components...)
+
+	pathsep := pathSepForPlatform(platform)
+	translated := strings.Join(components, pathsep)
+
+	return translated
+}
+
+// pathSepForPlatform returns the path separator for the given platform.
+// This is rather simple, and just returns `\` on Windows and `/` on all other
+// platforms.
+func pathSepForPlatform(platform string) string {
+	switch platform {
+	case "windows":
+		return `\`
+	default:
+		return "/"
+	}
+}
+
+func isPathSep(r rune) bool {
+	return r == '/' || r == '\\'
+}

pkg/crosspath/crosspath_test.go

@@ -200,3 +200,40 @@ func TestIsRoot(t *testing.T) {
 		})
 	}
 }
+
+func TestToPlatform(t *testing.T) {
+	type args struct {
+		path     string
+		platform string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{"empty-win", args{``, "windows"}, ``},
+		{"empty-lnx", args{``, "linux"}, ``},
+		{"single-win", args{`path with spaces`, "windows"}, `path with spaces`},
+		{"single-lnx", args{`path with spaces`, "linux"}, `path with spaces`},
+		{"native-win", args{`native\path`, "windows"}, `native\path`},
+		{"native-lnx", args{`native/path`, "linux"}, `native/path`},
+		{"opposite-win", args{`opposite/path`, "windows"}, `opposite\path`},
+		{"opposite-lnx", args{`opposite\path`, "linux"}, `opposite/path`},
+		{"mixed-win", args{`F:/mixed/path\to\file.blend`, "windows"}, `F:\mixed\path\to\file.blend`},
+		{"mixed-lnx", args{`F:/mixed/path\to\file.blend`, "linux"}, `F:/mixed/path/to/file.blend`},
+		{"absolute-win", args{`F:/absolute/path`, "windows"}, `F:\absolute\path`},
+		{"absolute-lnx", args{`/absolute/path`, "linux"}, `/absolute/path`},
+		{"drive-relative-win", args{`/absolute/path`, "windows"}, `\absolute\path`},
+
+		// UNC notation should survive, even when it no longer makes sense (like on Linux).
+		{"unc-win", args{`\\NAS\share\path`, "windows"}, `\\NAS\share\path`},
+		{"unc-lnx", args{`\\NAS\share\path`, "linux"}, `//NAS/share/path`},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := ToPlatform(tt.args.path, tt.args.platform); got != tt.want {
+				t.Errorf("ToPlatform() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}