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