diff --git a/CHANGELOG.md b/CHANGELOG.md index 4efc579b..50714bcd 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/manager/api_impl/varrepl_test.go b/internal/manager/api_impl/varrepl_test.go index eabb43f0..8826e1cc 100644 --- a/internal/manager/api_impl/varrepl_test.go +++ b/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) { diff --git a/internal/manager/config/config.go b/internal/manager/config/config.go index a0ff4b23..182d9fac 100644 --- a/internal/manager/config/config.go +++ b/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 { diff --git a/internal/manager/config/settings_test.go b/internal/manager/config/settings_test.go index e74fc88f..bf06a5aa 100644 --- a/internal/manager/config/settings_test.go +++ b/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() diff --git a/pkg/crosspath/crosspath.go b/pkg/crosspath/crosspath.go index e8f57948..31ce52b0 100644 --- a/pkg/crosspath/crosspath.go +++ b/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 == '\\' +} diff --git a/pkg/crosspath/crosspath_test.go b/pkg/crosspath/crosspath_test.go index 9a692a12..01ed3ef4 100644 --- a/pkg/crosspath/crosspath_test.go +++ b/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) + } + }) + } +}