Manager: simplify variable expansion

Simplify the variable expansion code. Instead of using a separate goroutine
and two channels, use a struct + a simple function call.

No functional changes.

internal/manager/api_impl/meta.go

@@ -71,18 +71,12 @@ func (f *Flamenco) GetVariables(e echo.Context, audience api.ManagerVariableAudi
 
 func (f *Flamenco) GetSharedStorage(e echo.Context, audience api.ManagerVariableAudience, platform string) error {
 	location := f.config.EffectiveStoragePath()
-
+	varExpand := f.config.NewVariableExpander(config.VariableAudience(audience), config.VariablePlatform(platform))
-	feeder := make(chan string, 1)
-	receiver := make(chan string, 1)
-
-	feeder <- location
-	close(feeder)
-	f.config.ExpandVariables(feeder, receiver, config.VariableAudience(audience), config.VariablePlatform(platform))
 
 	return e.JSON(http.StatusOK, api.SharedStorageLocation{
 		Audience:      audience,
 		Platform:      platform,
-		Location:      <-receiver,
+		Location:      varExpand.Expand(location),
 		ShamanEnabled: f.isShamanEnabled(),
 	})
 }

internal/manager/api_impl/meta_test.go

@@ -90,12 +90,10 @@ func TestGetSharedStorage(t *testing.T) {
 	mf.config.EXPECT().EffectiveStoragePath().Return(`S:\storage\flamenco`).AnyTimes()
 
 	{ // Test user client on Linux.
-		mf.config.EXPECT().
-			ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceUsers, config.VariablePlatformLinux).
-			Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) {
 		// Defer to the actual ExpandVariables() implementation of the above config.
-				conf.ExpandVariables(inputChannel, outputChannel, audience, platform)
+		mf.config.EXPECT().
-			})
+			NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformLinux).
+			DoAndReturn(conf.NewVariableExpander)
 		mf.shaman.EXPECT().IsEnabled().Return(false)
 
 		echoCtx := mf.prepareMockedRequest(nil)
@@ -109,12 +107,10 @@ func TestGetSharedStorage(t *testing.T) {
 	}
 
 	{ // Test worker client on Linux with Shaman enabled.
-		mf.config.EXPECT().
-			ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceWorkers, config.VariablePlatformLinux).
-			Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) {
 		// Defer to the actual ExpandVariables() implementation of the above config.
-				conf.ExpandVariables(inputChannel, outputChannel, audience, platform)
+		mf.config.EXPECT().
-			})
+			NewVariableExpander(config.VariableAudienceWorkers, config.VariablePlatformLinux).
+			DoAndReturn(conf.NewVariableExpander)
 		mf.shaman.EXPECT().IsEnabled().Return(true)
 
 		echoCtx := mf.prepareMockedRequest(nil)
@@ -129,12 +125,10 @@ func TestGetSharedStorage(t *testing.T) {
 	}
 
 	{ // Test user client on Windows.
-		mf.config.EXPECT().
-			ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceUsers, config.VariablePlatformWindows).
-			Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) {
 		// Defer to the actual ExpandVariables() implementation of the above config.
-				conf.ExpandVariables(inputChannel, outputChannel, audience, platform)
+		mf.config.EXPECT().
-			})
+			NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformWindows).
+			DoAndReturn(conf.NewVariableExpander)
 		mf.shaman.EXPECT().IsEnabled().Return(false)
 
 		echoCtx := mf.prepareMockedRequest(nil)

internal/manager/api_impl/mocks/api_impl_mock.gen.go

@@ -855,18 +855,6 @@ func (mr *MockConfigServiceMockRecorder) EffectiveStoragePath() *gomock.Call {
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EffectiveStoragePath", reflect.TypeOf((*MockConfigService)(nil).EffectiveStoragePath))
 }
 
-// ExpandVariables mocks base method.
-func (m *MockConfigService) ExpandVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) {
-	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "ExpandVariables", arg0, arg1, arg2, arg3)
-}
-
-// ExpandVariables indicates an expected call of ExpandVariables.
-func (mr *MockConfigServiceMockRecorder) ExpandVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call {
-	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpandVariables", reflect.TypeOf((*MockConfigService)(nil).ExpandVariables), arg0, arg1, arg2, arg3)
-}
-
 // ForceFirstRun mocks base method.
 func (m *MockConfigService) ForceFirstRun() {
 	m.ctrl.T.Helper()
@@ -908,6 +896,20 @@ func (mr *MockConfigServiceMockRecorder) IsFirstRun() *gomock.Call {
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsFirstRun", reflect.TypeOf((*MockConfigService)(nil).IsFirstRun))
 }
 
+// NewVariableExpander mocks base method.
+func (m *MockConfigService) NewVariableExpander(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.VariableExpander {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "NewVariableExpander", arg0, arg1)
+	ret0, _ := ret[0].(*config.VariableExpander)
+	return ret0
+}
+
+// NewVariableExpander indicates an expected call of NewVariableExpander.
+func (mr *MockConfigServiceMockRecorder) NewVariableExpander(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableExpander", reflect.TypeOf((*MockConfigService)(nil).NewVariableExpander), arg0, arg1)
+}
+
 // NewVariableToValueConverter mocks base method.
 func (m *MockConfigService) NewVariableToValueConverter(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.ValueToVariableReplacer {
 	m.ctrl.T.Helper()

internal/manager/api_impl/mocks/varrepl.gen.go

@@ -34,16 +34,18 @@ func (m *MockVariableReplacer) EXPECT() *MockVariableReplacerMockRecorder {
 	return m.recorder
 }
 
-// ExpandVariables mocks base method.
+// NewVariableExpander mocks base method.
-func (m *MockVariableReplacer) ExpandVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) {
+func (m *MockVariableReplacer) NewVariableExpander(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.VariableExpander {
 	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "ExpandVariables", arg0, arg1, arg2, arg3)
+	ret := m.ctrl.Call(m, "NewVariableExpander", arg0, arg1)
+	ret0, _ := ret[0].(*config.VariableExpander)
+	return ret0
 }
 
-// ExpandVariables indicates an expected call of ExpandVariables.
+// NewVariableExpander indicates an expected call of NewVariableExpander.
-func (mr *MockVariableReplacerMockRecorder) ExpandVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call {
+func (mr *MockVariableReplacerMockRecorder) NewVariableExpander(arg0, arg1 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpandVariables", reflect.TypeOf((*MockVariableReplacer)(nil).ExpandVariables), arg0, arg1, arg2, arg3)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableExpander", reflect.TypeOf((*MockVariableReplacer)(nil).NewVariableExpander), arg0, arg1)
 }
 
 // NewVariableToValueConverter mocks base method.

internal/manager/api_impl/support_test.go

@@ -101,8 +101,8 @@ func (mf *mockedFlamenco) expectExpandVariables(
 
 	// Defer the mocked call to the fake configuration.
 	return mf.config.EXPECT().
-		ExpandVariables(gomock.Any(), gomock.Any(), expectAudience, expectPlatform).
+		NewVariableExpander(expectAudience, expectPlatform).
-		DoAndReturn(c.ExpandVariables)
+		DoAndReturn(c.NewVariableExpander)
 }
 
 func (mf *mockedFlamenco) expectConvertTwoWayVariables(

internal/manager/api_impl/varrepl.go

@@ -3,8 +3,6 @@ package api_impl
 // SPDX-License-Identifier: GPL-3.0-or-later
 
 import (
-	"sync"
-
 	"git.blender.org/flamenco/internal/manager/config"
 	"git.blender.org/flamenco/internal/manager/persistence"
 	"git.blender.org/flamenco/pkg/api"
@@ -12,36 +10,28 @@ import (
 
 //go:generate go run github.com/golang/mock/mockgen -destination mocks/varrepl.gen.go -package mocks git.blender.org/flamenco/internal/manager/api_impl VariableReplacer
 type VariableReplacer interface {
-	ExpandVariables(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform)
+	NewVariableExpander(audience config.VariableAudience, platform config.VariablePlatform) *config.VariableExpander
 	ResolveVariables(audience config.VariableAudience, platform config.VariablePlatform) map[string]config.ResolvedVariable
 	NewVariableToValueConverter(audience config.VariableAudience, platform config.VariablePlatform) *config.ValueToVariableReplacer
 }
 
 // replaceTaskVariables performs variable replacement for worker tasks.
 func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, worker persistence.Worker) api.AssignedTask {
-	feeder := make(chan string, 1)
+	varExpander := replacer.NewVariableExpander(
-	receiver := make(chan string, 1)
+		config.VariableAudienceWorkers,
-
+		config.VariablePlatform(worker.Platform),
-	wg := sync.WaitGroup{}
+	)
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		replacer.ExpandVariables(feeder, receiver,
-			config.VariableAudienceWorkers, config.VariablePlatform(worker.Platform))
-	}()
 
 	for cmdIndex, cmd := range task.Commands {
 		for key, value := range cmd.Parameters {
 			switch v := value.(type) {
 			case string:
-				feeder <- v
+				task.Commands[cmdIndex].Parameters[key] = varExpander.Expand(v)
-				task.Commands[cmdIndex].Parameters[key] = <-receiver
 
 			case []string:
 				replaced := make([]string, len(v))
 				for idx := range v {
-					feeder <- v[idx]
+					replaced[idx] = varExpander.Expand(v[idx])
-					replaced[idx] = <-receiver
 				}
 				task.Commands[cmdIndex].Parameters[key] = replaced
 
@@ -50,8 +40,7 @@ func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, work
 				for idx := range v {
 					switch itemValue := v[idx].(type) {
 					case string:
-						feeder <- itemValue
+						replaced[idx] = varExpander.Expand(itemValue)
-						replaced[idx] = <-receiver
 					default:
 						replaced[idx] = itemValue
 					}
@@ -64,10 +53,6 @@ func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, work
 		}
 	}
 
-	close(feeder)
-	wg.Wait()
-	close(receiver)
-
 	return task
 }
 

internal/manager/config/service.go

@@ -71,8 +71,8 @@ func (s *Service) Save() error {
 }
 
 // Expose some functions of Conf here, for easier mocking of functionality via interfaces.
-func (s *Service) ExpandVariables(inputChannel <-chan string, outputChannel chan<- string, audience VariableAudience, platform VariablePlatform) {
+func (s *Service) NewVariableExpander(audience VariableAudience, platform VariablePlatform) *VariableExpander {
-	s.config.ExpandVariables(inputChannel, outputChannel, audience, platform)
+	return s.config.NewVariableExpander(audience, platform)
 }
 func (s *Service) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer {
 	return s.config.NewVariableToValueConverter(audience, platform)

internal/manager/config/variables.go

@@ -12,6 +12,14 @@ type ValueToVariableReplacer struct {
 	twoWayVars map[string]string // Mapping from variable name to value.
 }
 
+// VariableExpander expands variables and applies two-way variable replacement to the values.
+type VariableExpander struct {
+	oneWayVars        map[string]string // Mapping from variable name to value.
+	managerTwoWayVars map[string]string // Mapping from variable name to value for the Manager platform.
+	targetTwoWayVars  map[string]string // Mapping from variable name to value for the target platform.
+	targetPlatform    VariablePlatform
+}
+
 // NewVariableToValueConverter returns a ValueToVariableReplacer for the given audience & platform.
 func (c *Conf) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer {
 	// Get the variables for the given audience & platform.
@@ -29,6 +37,25 @@ 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 {
+	// Get the variables for the given audience & platform.
+	varsForPlatform := c.getVariables(audience, platform)
+	if len(varsForPlatform) == 0 {
+		log.Warn().
+			Str("audience", string(audience)).
+			Str("platform", string(platform)).
+			Msg("no variables defined for this platform given this audience")
+	}
+
+	return &VariableExpander{
+		oneWayVars:        varsForPlatform,
+		managerTwoWayVars: c.GetTwoWayVariables(audience, c.currentGOOS),
+		targetTwoWayVars:  c.GetTwoWayVariables(audience, platform),
+		targetPlatform:    platform,
+	}
+}
+
 // ValueToVariableReplacer replaces any variable values it recognises in
 // valueToConvert to the actual variable. For example, `/path/to/file.blend` can
 // be changed to `{my_storage}/file.blend`.
@@ -72,3 +99,42 @@ func isValueMatch(valueToMatch, variableValue string) bool {
 
 	return false
 }
+
+// Replace converts "{variable name}" to the value that belongs to the audience and platform.
+func (ve *VariableExpander) Expand(valueToExpand string) string {
+	expanded := valueToExpand
+
+	// Expand variables from {varname} to their value for the target platform.
+	for varname, varvalue := range ve.oneWayVars {
+		placeholder := fmt.Sprintf("{%s}", varname)
+		expanded = strings.Replace(expanded, placeholder, varvalue, -1)
+	}
+
+	// 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
+	// the Manager's platform, usable for the target platform.
+	//
+	// 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 {
+			continue
+		}
+		if !isValueMatch(expanded, managerValue) {
+			continue
+		}
+		expanded = targetValue + expanded[len(managerValue):]
+
+		// Since two-way variables are meant for path replacement, we know this
+		// should be a path.
+		isPathValue = true
+	}
+
+	if isPathValue {
+		expanded = crosspath.ToPlatform(expanded, string(ve.targetPlatform))
+	}
+	return expanded
+}