Manager: normalise slashes when expanding two-way variables

When expanding the `{shared}` variable in a path like
`{shared}\shot\file.blend`, all slashes will be normalised to the target
platform, so for example result in:

- Windows: `Y:\shared\flamenco\shot\file.blend`
- Linux  : `/shared/flamenco/shot/file.blend`

Due to this normalisation, the same paths will be served to these
platforms when the path was `{shared}/shot/file.blend`.

internal/manager/config/config.go

@@ -517,6 +517,23 @@ func (c *Conf) GetTwoWayVariables(audience VariableAudience, platform VariablePl
 	return twoWayVars
 }
 
+// GetOneWayVariables returns the regular (one-way) variable values for this (audience,
+// platform) combination. If no variables are found, just returns an empty map.
+// If a value is defined for both the "all" platform and specifically the given
+// platform, the specific platform definition wins.
+func (c *Conf) GetOneWayVariables(audience VariableAudience, platform VariablePlatform) map[string]string {
+	varsForPlatform := c.getVariables(audience, platform)
+
+	// Only keep the two-way variables.
+	oneWayVars := map[string]string{}
+	for varname, value := range varsForPlatform {
+		if !c.isTwoWay(varname) {
+			oneWayVars[varname] = value
+		}
+	}
+	return oneWayVars
+}
+
 // ResolveVariables returns the variables for this (audience, platform) combination.
 // If no variables are found, just returns an empty map. If a value is defined
 // for both the "all" platform and specifically the given platform, the specific

internal/manager/config/variables.go

@@ -40,7 +40,7 @@ func (c *Conf) NewVariableToValueConverter(audience VariableAudience, platform V
 // NewVariableExpander returns a new VariableExpander for the given audience & platform.
 func (c *Conf) NewVariableExpander(audience VariableAudience, platform VariablePlatform) *VariableExpander {
 	return &VariableExpander{
-		oneWayVars:        varsForPlatform,
+		oneWayVars:        c.GetOneWayVariables(audience, platform),
 		managerTwoWayVars: c.GetTwoWayVariables(audience, c.currentGOOS),
 		targetTwoWayVars:  c.GetTwoWayVariables(audience, platform),
 		targetPlatform:    platform,
@@ -80,15 +80,16 @@ func isValueMatch(valueToMatch, variableValue string) bool {
 		return true
 	}
 
-	// If the variable value has a backslash, assume it is a Windows path.
+	// If any of the values have backslashes, assume it is a Windows path.
 	// Convert it to slash notation just to see if that would provide a
 	// match.
 	if strings.ContainsRune(variableValue, '\\') {
-		slashedValue := crosspath.ToSlash(variableValue)
-		return strings.HasPrefix(valueToMatch, slashedValue)
+		variableValue = crosspath.ToSlash(variableValue)
 	}
-
-	return false
+	if strings.ContainsRune(valueToMatch, '\\') {
+		valueToMatch = crosspath.ToSlash(valueToMatch)
+	}
+	return strings.HasPrefix(valueToMatch, variableValue)
 }
 
 // Replace converts "{variable name}" to the value that belongs to the audience and platform.
@@ -101,6 +102,17 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 		expanded = strings.Replace(expanded, placeholder, varvalue, -1)
 	}
 
+	// Go through the two-way variables for the target platform.
+	isPathValue := false
+	for varname, varvalue := range ve.targetTwoWayVars {
+		placeholder := fmt.Sprintf("{%s}", varname)
+		expanded = strings.Replace(expanded, placeholder, varvalue, -1)
+
+		// Since two-way variables are meant for path replacement, we know this
+		// should be a path.
+		isPathValue = true
+	}
+
 	// Go through the two-way variables, to make sure that the result of
 	// expanding variables gets the two-way variables applied as well. This is
 	// necessary to make implicitly-defined variable, which are only defined for
@@ -108,7 +120,6 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 	//
 	// Practically, this replaces "value for the Manager platform" with "value
 	// for the target platform".
-	isPathValue := false
 	for varname, managerValue := range ve.managerTwoWayVars {
 		targetValue, ok := ve.targetTwoWayVars[varname]
 		if !ok {
@@ -128,6 +139,11 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 		expanded = crosspath.ToPlatform(expanded, string(ve.targetPlatform))
 	}
 
+	log.Trace().
+		Str("valueToExpand", valueToExpand).
+		Str("expanded", expanded).
+		Bool("isPathValue", isPathValue).
+		Msg("expanded variable")
 	return expanded
 }
 

internal/manager/config/variables_test.go

@@ -6,7 +6,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestReplaceTwowayVariables(t *testing.T) {
+func TestReplaceTwowayVariablesMixedSlashes(t *testing.T) {
 	c := DefaultConfig(func(c *Conf) {
 		c.Variables["shared"] = Variable{
 			IsTwoWay: true,
@@ -17,10 +17,36 @@ func TestReplaceTwowayVariables(t *testing.T) {
 		}
 	})
 
-	replacer := c.NewVariableToValueConverter(VariableAudienceUsers, VariablePlatformWindows)
+	replacerWin := c.NewVariableToValueConverter(VariableAudienceWorkers, VariablePlatformWindows)
+	replacerLnx := c.NewVariableToValueConverter(VariableAudienceWorkers, VariablePlatformLinux)
 
 	// This is the real reason for this test: forward slashes in the path should
 	// still be matched to the backslashes in the variable value.
-	assert.Equal(t, `{shared}\shot\file.blend`, replacer.Replace(`Y:\shared\flamenco\shot\file.blend`))
-	assert.Equal(t, `{shared}/shot/file.blend`, replacer.Replace(`Y:/shared/flamenco/shot/file.blend`))
+	assert.Equal(t, `{shared}\shot\file.blend`, replacerWin.Replace(`Y:\shared\flamenco\shot\file.blend`))
+	assert.Equal(t, `{shared}/shot/file.blend`, replacerWin.Replace(`Y:/shared/flamenco/shot/file.blend`))
+
+	assert.Equal(t, `{shared}\shot\file.blend`, replacerLnx.Replace(`/shared\flamenco\shot\file.blend`))
+	assert.Equal(t, `{shared}/shot/file.blend`, replacerLnx.Replace(`/shared/flamenco/shot/file.blend`))
+}
+
+func TestExpandTwowayVariablesMixedSlashes(t *testing.T) {
+	c := DefaultConfig(func(c *Conf) {
+		c.Variables["shared"] = Variable{
+			IsTwoWay: true,
+			Values: []VariableValue{
+				{Value: "/shared/flamenco", Platform: VariablePlatformLinux},
+				{Value: `Y:\shared\flamenco`, Platform: VariablePlatformWindows},
+			},
+		}
+	})
+
+	expanderWin := c.NewVariableExpander(VariableAudienceWorkers, VariablePlatformWindows)
+	expanderLnx := c.NewVariableExpander(VariableAudienceWorkers, VariablePlatformLinux)
+
+	// Slashes should always be normalised for the target platform, on the entire path, not just the replaced part.
+	assert.Equal(t, `Y:\shared\flamenco\shot\file.blend`, expanderWin.Expand(`{shared}\shot\file.blend`))
+	assert.Equal(t, `Y:\shared\flamenco\shot\file.blend`, expanderWin.Expand(`{shared}/shot/file.blend`))
+
+	assert.Equal(t, `/shared/flamenco/shot/file.blend`, expanderLnx.Expand(`{shared}\shot\file.blend`))
+	assert.Equal(t, `/shared/flamenco/shot/file.blend`, expanderLnx.Expand(`{shared}/shot/file.blend`))
 }