From e5a20425c474ec93edbe03d2667ec5184f32d3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 30 Aug 2022 10:36:04 +0200 Subject: [PATCH] Separate variables for Blender executable and its arguments. Split "executable" from "its arguments" in blender & ffmpeg commands. Use `{blenderArgs}` variable to hold the default Blender arguments, instead of having both the executable and its arguments in `{blender}`. The reason for this is to support backslashes in the Blender executable path. These were interpreted as escape characters by the shell lexer. The shell lexer based splitting is now only performed on the default arguments, with the result that `C:\Program Files\Blender Foundation\3.3\blender.exe` is now a valid value for `{blender}`. This does mean that this is backward incompatible change, and that it requires setting up Flamenco Manager again, and that older jobs will not be able to be rerun. It is recommended to remove `flamenco-manager.yaml`, restart Flamenco Manager, and reconfigure via the setup assistant. --- CHANGELOG.md | 13 ++++++++- internal/manager/api_impl/meta.go | 16 ++++++----- internal/manager/api_impl/meta_test.go | 27 ++++++++++++------- internal/manager/config/defaults.go | 11 +++++--- internal/manager/config/settings_test.go | 5 +++- .../job_compilers/job_compilers_test.go | 3 +++ .../scripts/simple_blender_render.js | 1 + internal/worker/command_blender.go | 20 ++++++++------ internal/worker/command_blender_test.go | 20 ++++++++++---- internal/worker/command_ffmpeg.go | 20 ++++++++------ internal/worker/command_ffmpeg_test.go | 16 ++++++----- 11 files changed, 104 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e3a04ba..92c76782 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ bugs in actually-released versions. ## 3.0-beta2 - in development +WARNING: this version is backward incompatible. Any job created with Flamenco +3.0-beta1 will not run with Flamenco 3.0-beta2. Only upgrade after +currently-active jobs have finished, or cancel them. + +It is recommended to remove `flamenco-manager.yaml`, restart Flamenco Manager, +and reconfigure via the setup assistant. + - Manager & Add-on: avoid error that could occur when submitting jobs with UDIM files ([44ccc6c3ca70](https://developer.blender.org/rF44ccc6c3ca706fdd268bf310f3e8965d58482449)). - Manager: don't stop when the Flamenco Setup Assistant cannot start a webbrowser @@ -15,7 +22,11 @@ bugs in actually-released versions. putting all the files in the root of the tarball). - Two-way variable replacement now also changes the path separators to the target platform. - Allow setting priority when submitting a job. - +- Separate "blender location" and "blender arguments" into two variables. + - The variable `blender` now should only point at the Blender executable, for + example `D:\Blender_3.2_stable\blender.exe`. + - The variable `blenderArgs` can be used to set the default Blender arguments, + for example `-b -y`. ## 3.0-beta1 - released 2022-08-03 diff --git a/internal/manager/api_impl/meta.go b/internal/manager/api_impl/meta.go index a5c5e1db..b82fa485 100644 --- a/internal/manager/api_impl/meta.go +++ b/internal/manager/api_impl/meta.go @@ -278,16 +278,18 @@ func (f *Flamenco) SaveSetupAssistantConfig(e echo.Context) error { if commandNeedsQuoting(executable) { executable = strconv.Quote(executable) } - blenderCommand := fmt.Sprintf("%s %s", executable, config.DefaultBlenderArguments) - // Use the same command for each platform for now, but put them each in their // own definition so that they're easier to edit later. conf.Variables["blender"] = config.Variable{ - IsTwoWay: false, Values: config.VariableValues{ - {Platform: "linux", Value: blenderCommand}, - {Platform: "windows", Value: blenderCommand}, - {Platform: "darwin", Value: blenderCommand}, + {Platform: "linux", Value: executable}, + {Platform: "windows", Value: executable}, + {Platform: "darwin", Value: executable}, + }, + } + conf.Variables["blenderArgs"] = config.Variable{ + Values: config.VariableValues{ + {Platform: config.VariablePlatformAll, Value: config.DefaultBlenderArguments}, }, } @@ -314,5 +316,5 @@ func flamencoManagerDir() (string, error) { } func commandNeedsQuoting(cmd string) bool { - return strings.ContainsAny(cmd, " \n\t;()") + return strings.ContainsAny(cmd, "\n\t;()") } diff --git a/internal/manager/api_impl/meta_test.go b/internal/manager/api_impl/meta_test.go index d0ead7b4..f6e5e195 100644 --- a/internal/manager/api_impl/meta_test.go +++ b/internal/manager/api_impl/meta_test.go @@ -126,6 +126,12 @@ func TestSaveSetupAssistantConfig(t *testing.T) { mf, finish := metaTestFixtures(t) defer finish() + defaultBlenderArgsVar := config.Variable{ + Values: config.VariableValues{ + {Platform: config.VariablePlatformAll, Value: config.DefaultBlenderArguments}, + }, + } + doTest := func(body api.SetupAssistantConfig) config.Conf { // Always start the test with a clean configuration. originalConfig := config.DefaultConfig(func(c *config.Conf) { @@ -165,12 +171,13 @@ func TestSaveSetupAssistantConfig(t *testing.T) { assert.Equal(t, mf.tempdir, savedConfig.SharedStoragePath) expectBlenderVar := config.Variable{ Values: config.VariableValues{ - {Platform: "linux", Value: "blender " + config.DefaultBlenderArguments}, - {Platform: "windows", Value: "blender " + config.DefaultBlenderArguments}, - {Platform: "darwin", Value: "blender " + config.DefaultBlenderArguments}, + {Platform: "linux", Value: "blender"}, + {Platform: "windows", Value: "blender"}, + {Platform: "darwin", Value: "blender"}, }, } assert.Equal(t, expectBlenderVar, savedConfig.Variables["blender"]) + assert.Equal(t, defaultBlenderArgsVar, savedConfig.Variables["blenderArgs"]) } // Test situation where the given command could be found on $PATH. @@ -187,12 +194,13 @@ func TestSaveSetupAssistantConfig(t *testing.T) { assert.Equal(t, mf.tempdir, savedConfig.SharedStoragePath) expectBlenderVar := config.Variable{ Values: config.VariableValues{ - {Platform: "linux", Value: "kitty " + config.DefaultBlenderArguments}, - {Platform: "windows", Value: "kitty " + config.DefaultBlenderArguments}, - {Platform: "darwin", Value: "kitty " + config.DefaultBlenderArguments}, + {Platform: "linux", Value: "kitty"}, + {Platform: "windows", Value: "kitty"}, + {Platform: "darwin", Value: "kitty"}, }, } assert.Equal(t, expectBlenderVar, savedConfig.Variables["blender"]) + assert.Equal(t, defaultBlenderArgsVar, savedConfig.Variables["blenderArgs"]) } // Test a custom command given with the full path. @@ -209,12 +217,13 @@ func TestSaveSetupAssistantConfig(t *testing.T) { assert.Equal(t, mf.tempdir, savedConfig.SharedStoragePath) expectBlenderVar := config.Variable{ Values: config.VariableValues{ - {Platform: "linux", Value: "/bin/cat " + config.DefaultBlenderArguments}, - {Platform: "windows", Value: "/bin/cat " + config.DefaultBlenderArguments}, - {Platform: "darwin", Value: "/bin/cat " + config.DefaultBlenderArguments}, + {Platform: "linux", Value: "/bin/cat"}, + {Platform: "windows", Value: "/bin/cat"}, + {Platform: "darwin", Value: "/bin/cat"}, }, } assert.Equal(t, expectBlenderVar, savedConfig.Variables["blender"]) + assert.Equal(t, defaultBlenderArgsVar, savedConfig.Variables["blenderArgs"]) } } diff --git a/internal/manager/config/defaults.go b/internal/manager/config/defaults.go index 55f53770..2468467f 100644 --- a/internal/manager/config/defaults.go +++ b/internal/manager/config/defaults.go @@ -62,9 +62,14 @@ var defaultConfig = Conf{ // The default commands assume that the executables are available on $PATH. "blender": { Values: VariableValues{ - VariableValue{Platform: "linux", Value: "blender " + DefaultBlenderArguments}, - VariableValue{Platform: "windows", Value: "blender.exe " + DefaultBlenderArguments}, - VariableValue{Platform: "darwin", Value: "blender " + DefaultBlenderArguments}, + VariableValue{Platform: VariablePlatformLinux, Value: "blender"}, + VariableValue{Platform: VariablePlatformWindows, Value: "blender.exe"}, + VariableValue{Platform: VariablePlatformDarwin, Value: "blender"}, + }, + }, + "blenderArgs": { + Values: VariableValues{ + VariableValue{Platform: VariablePlatformAll, Value: DefaultBlenderArguments}, }, }, // TODO: determine useful defaults for these. diff --git a/internal/manager/config/settings_test.go b/internal/manager/config/settings_test.go index bf06a5aa..dcc8980e 100644 --- a/internal/manager/config/settings_test.go +++ b/internal/manager/config/settings_test.go @@ -20,9 +20,12 @@ func TestDefaultSettings(t *testing.T) { assert.Equal(t, defaultConfig.DatabaseDSN, config.DatabaseDSN) assert.Equal(t, false, config.Variables["blender"].IsTwoWay) - assert.Equal(t, "blender "+DefaultBlenderArguments, config.Variables["blender"].Values[0].Value) + assert.Equal(t, "blender", config.Variables["blender"].Values[0].Value) assert.Equal(t, VariablePlatformLinux, config.Variables["blender"].Values[0].Platform) + assert.Equal(t, DefaultBlenderArguments, config.Variables["blenderArgs"].Values[0].Value) + assert.Equal(t, VariablePlatformAll, config.Variables["blenderArgs"].Values[0].Platform) + assert.Greater(t, config.BlocklistThreshold, 0) } diff --git a/internal/manager/job_compilers/job_compilers_test.go b/internal/manager/job_compilers/job_compilers_test.go index 119bf9de..b19a5762 100644 --- a/internal/manager/job_compilers/job_compilers_test.go +++ b/internal/manager/job_compilers/job_compilers_test.go @@ -103,6 +103,7 @@ func TestSimpleBlenderRenderHappy(t *testing.T) { assert.Equal(t, "blender-render", t0.Commands[0].Name) assert.EqualValues(t, AuthoredCommandParameters{ "exe": "{blender}", + "exeArgs": "{blenderArgs}", "blendfile": settings["blendfile"].(string), "args": expectCliArgs, "argsBefore": make([]interface{}, 0), @@ -187,6 +188,7 @@ func TestSimpleBlenderRenderWindowsPaths(t *testing.T) { assert.Equal(t, "blender-render", t0.Commands[0].Name) assert.EqualValues(t, AuthoredCommandParameters{ "exe": "{blender}", + "exeArgs": "{blenderArgs}", "blendfile": "R:\\sf\\jobs\\scene123.blend", // The blendfile parameter is just copied as-is, so keeps using backslash notation. "args": expectCliArgs, "argsBefore": make([]interface{}, 0), @@ -240,6 +242,7 @@ func TestSimpleBlenderRenderOutputPathFieldReplacement(t *testing.T) { } assert.EqualValues(t, AuthoredCommandParameters{ "exe": "{blender}", + "exeArgs": "{blenderArgs}", "blendfile": sj.Settings.AdditionalProperties["blendfile"].(string), "args": expectCliArgs, "argsBefore": make([]interface{}, 0), diff --git a/internal/manager/job_compilers/scripts/simple_blender_render.js b/internal/manager/job_compilers/scripts/simple_blender_render.js index 08259437..8a8365eb 100644 --- a/internal/manager/job_compilers/scripts/simple_blender_render.js +++ b/internal/manager/job_compilers/scripts/simple_blender_render.js @@ -121,6 +121,7 @@ function authorRenderTasks(settings, renderDir, renderOutput) { const task = author.Task(`render-${chunk}`, "blender"); const command = author.Command("blender-render", { exe: settings.blender_cmd, + exeArgs: "{blenderArgs}", argsBefore: [], blendfile: settings.blendfile, args: [ diff --git a/internal/worker/command_blender.go b/internal/worker/command_blender.go index 4711e37b..5b2cab2b 100644 --- a/internal/worker/command_blender.go +++ b/internal/worker/command_blender.go @@ -22,7 +22,8 @@ import ( var regexpFileSaved = regexp.MustCompile("Saved: '(.*)'") type BlenderParameters struct { - exe string // Expansion of `{blender}`: executable path + its CLI parameters defined by the Manager. + exe string // Expansion of `{blender}`: the executable path defined by the Manager. + exeArgs string // Expansion of `{blenderargs}`: its CLI parameters defined by the Manager. argsBefore []string // Additional CLI arguments defined by the job compiler script, to go before the blend file name. blendfile string // Path of the file to open. args []string // Additional CLI arguments defined by the job compiler script, to go after the blend file name. @@ -133,6 +134,10 @@ func cmdBlenderRenderParams(logger zerolog.Logger, cmd api.Command) (BlenderPara logger.Warn().Interface("command", cmd).Msg("missing 'exe' parameter") return parameters, NewParameterMissingError("exe", cmd) } + + // Ignore the `ok` return value, as a missing exeArgs key is fine: + parameters.exeArgs, _ = cmdParameter[string](cmd, "exeArgs") + if parameters.argsBefore, ok = cmdParameterAsStrings(cmd, "argsBefore"); !ok { logger.Warn().Interface("command", cmd).Msg("invalid 'argsBefore' parameter") return parameters, NewParameterInvalidError("argsBefore", cmd, "cannot convert to list of strings") @@ -146,17 +151,16 @@ func cmdBlenderRenderParams(logger zerolog.Logger, cmd api.Command) (BlenderPara return parameters, NewParameterInvalidError("args", cmd, "cannot convert to list of strings") } - // Move any CLI args from 'exe' to 'argsBefore'. - exeArgs, err := shlex.Split(parameters.exe) + // Split the exeArgs string into separate parts, and prepend them to `argsBefore`. + exeArgs, err := shlex.Split(parameters.exeArgs) if err != nil { - logger.Warn().Err(err).Interface("command", cmd).Msg("error parsing 'exe' parameter with shlex") - return parameters, NewParameterInvalidError("exe", cmd, err.Error()) + logger.Warn().Err(err).Interface("command", cmd).Msg("error parsing 'exeArgs' parameter with shlex") + return parameters, NewParameterInvalidError("exeArgs", cmd, err.Error()) } - if len(exeArgs) > 1 { + if len(exeArgs) > 0 { allArgsBefore := []string{} - allArgsBefore = append(allArgsBefore, exeArgs[1:]...) + allArgsBefore = append(allArgsBefore, exeArgs...) allArgsBefore = append(allArgsBefore, parameters.argsBefore...) - parameters.exe = exeArgs[0] parameters.argsBefore = allArgsBefore } diff --git a/internal/worker/command_blender_test.go b/internal/worker/command_blender_test.go index 66e6dbbd..d214ca3f 100644 --- a/internal/worker/command_blender_test.go +++ b/internal/worker/command_blender_test.go @@ -44,11 +44,21 @@ func TestCmdBlenderCliArgsInExeParameter(t *testing.T) { ce, mocks := testCommandExecutor(t, mockCtrl) + // Just use backslashes even when we're testing on Linux/macOS. Backslashes + // are always causing issues, so it's better to test with those on other + // platforms as well. Even when the resulting command cannot be run, every + // platform should be able to at least make the backslashes survive any + // processing. + exe := `D:\Blender_3.2_stable\blender.exe` + taskID := "1d54c6fe-1242-4c8f-bd63-5a09e358d7b6" cmd := api.Command{ Name: "blender", Parameters: map[string]interface{}{ - "exe": "/path/to/blender --factory-startup --python-expr \"import bpy; print('hello world')\"", + "exe": exe, + // This intentionally starts with a space (should be ignored) and has + // quoted text within quoted text: + "exeArgs": ` --factory-startup --python-expr "import bpy; print(\"hello world\")"`, "argsBefore": []string{"-no-audio"}, "blendfile": "file with spaces.blend", "args": []string{"--debug"}, @@ -56,10 +66,10 @@ func TestCmdBlenderCliArgsInExeParameter(t *testing.T) { } mocks.cli.EXPECT().CommandContext(gomock.Any(), - "/path/to/blender", // from 'exe' - "--factory-startup", // from 'exe' - "--python-expr", // from 'exe' - "import bpy; print('hello world')", // from 'exe' + exe, // from 'exe' + "--factory-startup", // from 'exeArgs' + "--python-expr", // from 'exeArgs' + `import bpy; print("hello world")`, // from 'exeArgs' "-no-audio", // from 'argsBefore' "file with spaces.blend", // from 'blendfile' "--debug", // from 'args' diff --git a/internal/worker/command_ffmpeg.go b/internal/worker/command_ffmpeg.go index bc8f8632..af1ed229 100644 --- a/internal/worker/command_ffmpeg.go +++ b/internal/worker/command_ffmpeg.go @@ -26,7 +26,8 @@ import ( ) type CreateVideoParams struct { - exe string // Executable path + its CLI parameters defined by the Manager. + exe string // Executable path defined by the Manager. + exeArgs string // Its CLI parameters defined by the Manager. fps float64 // Frames per second of the video file. inputGlob string // Glob of input files. outputFile string // File to save the video to. @@ -119,6 +120,10 @@ func cmdFramesToVideoParams(logger zerolog.Logger, cmd api.Command) (CreateVideo logger.Warn().Interface("command", cmd).Msg("missing 'exe' parameter") return parameters, NewParameterMissingError("exe", cmd) } + if parameters.exeArgs, ok = cmdParameter[string](cmd, "exeArgs"); !ok { + logger.Warn().Interface("command", cmd).Msg("invalid 'exeArgs' parameter") + return parameters, NewParameterInvalidError("exeArgs", cmd, "parameter must be string") + } if parameters.fps, ok = cmdParameter[float64](cmd, "fps"); !ok || parameters.fps == 0.0 { logger.Warn().Interface("command", cmd).Msg("missing 'fps' parameter") return parameters, NewParameterMissingError("fps", cmd) @@ -140,17 +145,16 @@ func cmdFramesToVideoParams(logger zerolog.Logger, cmd api.Command) (CreateVideo return parameters, NewParameterInvalidError("args", cmd, "cannot convert to list of strings") } - // Move any CLI args from 'exe' to 'argsBefore'. - exeArgs, err := shlex.Split(parameters.exe) + // Split the exeArgs string into separate parts, and prepend them to `argsBefore`. + exeArgs, err := shlex.Split(parameters.exeArgs) if err != nil { - logger.Warn().Err(err).Interface("command", cmd).Msg("error parsing 'exe' parameter with shlex") - return parameters, NewParameterInvalidError("exe", cmd, err.Error()) + logger.Warn().Err(err).Interface("command", cmd).Msg("error parsing 'exeArgs' parameter with shlex") + return parameters, NewParameterInvalidError("exeArgs", cmd, err.Error()) } - if len(exeArgs) > 1 { + if len(exeArgs) > 0 { allArgsBefore := []string{} - allArgsBefore = append(allArgsBefore, exeArgs[1:]...) + allArgsBefore = append(allArgsBefore, exeArgs...) allArgsBefore = append(allArgsBefore, parameters.argsBefore...) - parameters.exe = exeArgs[0] parameters.argsBefore = allArgsBefore } parameters.args = append(parameters.args, diff --git a/internal/worker/command_ffmpeg_test.go b/internal/worker/command_ffmpeg_test.go index 1de4dacf..ba8c2704 100644 --- a/internal/worker/command_ffmpeg_test.go +++ b/internal/worker/command_ffmpeg_test.go @@ -24,10 +24,12 @@ func TestCmdFramesToVideoSimplePosix(t *testing.T) { ce, mocks := testCommandExecutor(t, mockCtrl) taskID := "1d54c6fe-1242-4c8f-bd63-5a09e358d7b6" + exe := `F:\software\tools\ffmpeg.exe` // Backslashes are tricksy, test with them on all platforms. cmd := api.Command{ Name: "blender", Parameters: map[string]interface{}{ - "exe": "/path/to/ffmpeg -v quiet", + "exe": exe, + "exeArgs": "-v quiet", "argsBefore": []string{"-report"}, "inputGlob": "path/to/renders/*.png", "fps": 10.0, @@ -41,7 +43,7 @@ func TestCmdFramesToVideoSimplePosix(t *testing.T) { } cliArgs := []string{ - "-v", "quiet", // exe + "-v", "quiet", // exeArgs "-report", // argsBefore "-r", "10", // input frame rate "-pattern_type", "glob", "-i", "path/to/renders/*.png", // inputGlob @@ -49,7 +51,7 @@ func TestCmdFramesToVideoSimplePosix(t *testing.T) { "-r", "10", // output frame rate "path/to/renders/preview.mkv", // outputFile } - mocks.cli.EXPECT().CommandContext(gomock.Any(), "/path/to/ffmpeg", cliArgs).Return(nil) + mocks.cli.EXPECT().CommandContext(gomock.Any(), exe, cliArgs).Return(nil) err := ce.cmdFramesToVideo(context.Background(), zerolog.Nop(), taskID, cmd) assert.Equal(t, ErrNoExecCmd, err, "nil *exec.Cmd should result in ErrNoExecCmd") @@ -66,10 +68,12 @@ func TestCmdFramesToVideoSimpleWindows(t *testing.T) { ce, mocks := testCommandExecutor(t, mockCtrl) taskID := "1d54c6fe-1242-4c8f-bd63-5a09e358d7b6" + exe := `F:\software\tools\ffmpeg.exe` cmd := api.Command{ Name: "blender", Parameters: map[string]interface{}{ - "exe": "/path/to/ffmpeg -v quiet", + "exe": exe, + "exeArgs": "-v quiet", "argsBefore": []string{"-report"}, // NOTE: these files MUST exist, otherwise the glob index file creation will fail. "inputGlob": "command_ffmpeg_test_files/*.png", @@ -84,7 +88,7 @@ func TestCmdFramesToVideoSimpleWindows(t *testing.T) { } cliArgs := []string{ - "-v", "quiet", // exe + "-v", "quiet", // exeArgs "-report", // argsBefore "-f", "concat", "-safe", "0", "-i", "command_ffmpeg_test_files\\ffmpeg-file-index.txt", // input glob "-c:v", "hevc", "-crf", "31", "-vf", "pad=ceil(iw/2)*2:ceil(ih/2)*2", // args @@ -92,7 +96,7 @@ func TestCmdFramesToVideoSimpleWindows(t *testing.T) { "path/to/renders/preview.mkv", // outputFile } mocks.cli.EXPECT(). - CommandContext(gomock.Any(), "/path/to/ffmpeg", gomock.Any()). + CommandContext(gomock.Any(), exe, gomock.Any()). DoAndReturn(func(ctx context.Context, name string, arg ...string) error { assert.EqualValues(t, cliArgs, arg) return nil // this is the nil *exec.Cmd referenced below