From 7ffbd72dce1284ce81ff11949aa7ef50ad7f1d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 25 Jun 2024 11:24:08 +0200 Subject: [PATCH] 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 | 17 ++++++++++++ internal/manager/config/variables.go | 30 +++++++++++++++----- internal/manager/config/variables_test.go | 34 ++++++++++++++++++++--- 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/internal/manager/config/config.go b/internal/manager/config/config.go index 386d0b98..9b183514 100644 --- a/internal/manager/config/config.go +++ b/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 diff --git a/internal/manager/config/variables.go b/internal/manager/config/variables.go index 858c40a2..04fc7aca 100644 --- a/internal/manager/config/variables.go +++ b/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 } diff --git a/internal/manager/config/variables_test.go b/internal/manager/config/variables_test.go index c6c1e2a6..f073d17b 100644 --- a/internal/manager/config/variables_test.go +++ b/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`)) }