From c989bce51ea2f70bb1907672629f2bc448d7e612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 17 Mar 2022 15:39:45 +0100 Subject: [PATCH] Implement `move-directory` command, and use it in Simple Blender Render --- .../job_compilers/job_compilers_test.go | 12 +- .../scripts/simple_blender_render.js | 14 + internal/worker/command_exe.go | 10 +- internal/worker/command_file_mgmt.go | 152 ++++++++++ internal/worker/command_file_mgmt_test.go | 270 ++++++++++++++++++ 5 files changed, 450 insertions(+), 8 deletions(-) create mode 100644 internal/worker/command_file_mgmt.go create mode 100644 internal/worker/command_file_mgmt_test.go diff --git a/internal/manager/job_compilers/job_compilers_test.go b/internal/manager/job_compilers/job_compilers_test.go index fb503220..7c27e40c 100644 --- a/internal/manager/job_compilers/job_compilers_test.go +++ b/internal/manager/job_compilers/job_compilers_test.go @@ -84,8 +84,8 @@ func TestSimpleBlenderRenderHappy(t *testing.T) { settings := sj.Settings.AdditionalProperties - // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding - assert.Equal(t, 5, len(aj.Tasks)) + // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding, and cleanup + assert.Len(t, aj.Tasks, 6) t0 := aj.Tasks[0] expectCliArgs := []interface{}{ // They are strings, but Goja doesn't know that and will produce an []interface{}. "--render-output", "/render/sprites/farm_output/promo/square_ellie/square_ellie.lighting_light_breakdown2__intermediate-2006-01-02_090405/######", @@ -165,8 +165,8 @@ func TestSimpleBlenderRenderWindowsPaths(t *testing.T) { settings := sj.Settings.AdditionalProperties - // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding - assert.Equal(t, 5, len(aj.Tasks)) + // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding, and cleanup + assert.Len(t, aj.Tasks, 6) t0 := aj.Tasks[0] expectCliArgs := []interface{}{ // They are strings, but Goja doesn't know that and will produce an []interface{}. // The render output is constructed by the job compiler, and thus transforms to forward slashes. @@ -221,8 +221,8 @@ func TestSimpleBlenderRenderOutputPathFieldReplacement(t *testing.T) { // The job compiler should have replaced the {timestamp} and {ext} fields. assert.Equal(t, "/root/2006-01-02_090405/jobname/######", aj.Settings["render_output_path"]) - // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding - assert.Equal(t, 5, len(aj.Tasks)) + // Tasks should have been created to render the frames: 1-3, 4-6, 7-9, 10, video-encoding, and cleanup + assert.Len(t, aj.Tasks, 6) t0 := aj.Tasks[0] expectCliArgs := []interface{}{ // They are strings, but Goja doesn't know that and will produce an []interface{}. "--render-output", "/root/2006-01-02_090405/jobname__intermediate-2006-01-02_090405/######", diff --git a/internal/manager/job_compilers/scripts/simple_blender_render.js b/internal/manager/job_compilers/scripts/simple_blender_render.js index df2f9193..e5a282c7 100644 --- a/internal/manager/job_compilers/scripts/simple_blender_render.js +++ b/internal/manager/job_compilers/scripts/simple_blender_render.js @@ -72,8 +72,10 @@ function compileJob(job) { const settings = job.settings; const renderTasks = authorRenderTasks(settings, renderDir, renderOutput); const videoTask = authorCreateVideoTask(settings, renderDir); + const cleanupTask = authorCleanupTask(finalDir, renderDir); for (const rt of renderTasks) { + cleanupTask.addDependency(rt); job.addTask(rt); } if (videoTask) { @@ -81,8 +83,10 @@ function compileJob(job) { for (const rt of renderTasks) { videoTask.addDependency(rt); } + cleanupTask.addDependency(videoTask); job.addTask(videoTask); } + job.addTask(cleanupTask); } // Do field replacement on the render output path. @@ -156,6 +160,16 @@ function authorCreateVideoTask(settings, renderDir) { return task; } +function authorCleanupTask(finalDir, renderDir) { + const task = author.Task("move-to-final", "file-management"); + const command = author.Command("move-directory", { + src: renderDir, + dest: finalDir, + }); + task.addCommand(command); + return task; +} + // Return file name extension, including period, like '.png' or '.mkv'. function guessOutputFileExtension(settings) { switch (settings.images_or_video) { diff --git a/internal/worker/command_exe.go b/internal/worker/command_exe.go index 8768e498..2b413963 100644 --- a/internal/worker/command_exe.go +++ b/internal/worker/command_exe.go @@ -69,9 +69,15 @@ func NewCommandExecutor(cli CommandLineRunner, listener CommandListener, timeSer // switch statement) makes it possible to do things like reporting the list of // supported commands. ce.registry = map[string]commandCallable{ - "echo": ce.cmdEcho, - "sleep": ce.cmdSleep, + // misc + "echo": ce.cmdEcho, + "sleep": ce.cmdSleep, + + // blender "blender-render": ce.cmdBlenderRender, + + // file-management + "move-directory": ce.cmdMoveDirectory, } return ce diff --git a/internal/worker/command_file_mgmt.go b/internal/worker/command_file_mgmt.go new file mode 100644 index 00000000..2fd75ec6 --- /dev/null +++ b/internal/worker/command_file_mgmt.go @@ -0,0 +1,152 @@ +package worker + +// SPDX-License-Identifier: GPL-3.0-or-later + +/* This file contains the commands in the "file-management" type group. */ + +import ( + "context" + "fmt" + "os" + "path/filepath" + "regexp" + "strconv" + "time" + + "github.com/rs/zerolog" + + "git.blender.org/flamenco/pkg/api" +) + +// cmdMoveDirectory executes the "move-directory" command. +// It moves directory 'src' to 'dest'; if 'dest' already exists, it's moved to 'dest-{timestamp}'. +func (ce *CommandExecutor) cmdMoveDirectory(ctx context.Context, logger zerolog.Logger, taskID string, cmd api.Command) error { + var src, dest string + var ok bool + + if src, ok = cmdParameter[string](cmd, "src"); !ok || src == "" { + logger.Warn().Interface("command", cmd).Msg("missing 'src' parameter") + return fmt.Errorf("missing 'src' parameter: %+v", cmd.Parameters) + } + if dest, ok = cmdParameter[string](cmd, "dest"); !ok || dest == "" { + logger.Warn().Interface("command", cmd).Msg("missing 'dest' parameter") + return fmt.Errorf("missing 'dest' parameter: %+v", cmd.Parameters) + } + + logger = logger.With(). + Str("src", src). + Str("dest", dest). + Logger() + if !fileExists(src) { + logger.Warn().Msg("source path does not exist, not moving anything") + + msg := fmt.Sprintf("%s: source path %q does not exist, not moving anything", cmd.Name, src) + if err := ce.listener.LogProduced(ctx, taskID, msg); err != nil { + return err + } + return fmt.Errorf(msg) + } + + if fileExists(dest) { + backup, err := timestampedPath(dest) + if err != nil { + logger.Error().Err(err).Str("path", dest).Msg("unable to determine timestamp of directory") + return err + } + + if fileExists(backup) { + logger.Debug().Str("backup", backup).Msg("backup destination also exists, finding one that does not") + backup, err = uniquePath(backup) + if err != nil { + return err + } + } + + logger.Info(). + Str("toBackup", backup). + Msg("dest directory exists, moving to backup") + if err := ce.moveAndLog(ctx, taskID, cmd.Name, dest, backup); err != nil { + return err + } + } + + // self._log.info("Moving %s to %s", src, dest) + // await self.worker.register_log( + // "%s: Moving %s to %s", self.command_name, src, dest + // ) + // src.rename(dest) + logger.Info().Msg("moving directory") + return ce.moveAndLog(ctx, taskID, cmd.Name, src, dest) +} + +// moveAndLog renames a file/directory from `src` to `dest`, and logs the moveAndLog. +// The other parameters are just for logging. +func (ce *CommandExecutor) moveAndLog(ctx context.Context, taskID, cmdName, src, dest string) error { + msg := fmt.Sprintf("%s: moving %q to %q", cmdName, src, dest) + if err := ce.listener.LogProduced(ctx, taskID, msg); err != nil { + return err + } + + if err := os.Rename(src, dest); err != nil { + msg := fmt.Sprintf("%s: could not move %q to %q: %v", cmdName, src, dest, err) + if err := ce.listener.LogProduced(ctx, taskID, msg); err != nil { + return err + } + return err + } + + return nil +} + +func fileExists(filename string) bool { + _, err := os.Stat(filename) + return !os.IsNotExist(err) +} + +// timestampedPath returns the path with its modification time appended to the name. +func timestampedPath(filepath string) (string, error) { + stat, err := os.Stat(filepath) + if err != nil { + return "", fmt.Errorf("getting mtime of %s: %w", filepath, err) + } + + // Round away the milliseconds, as those aren't all that interesting. + // Uniqueness can ensured by calling unique_path() later. + mtime := stat.ModTime().Round(time.Second) + + iso := mtime.Local().Format("2006-01-02_150405") // YYYY-MM-DD_HHMMSS + return fmt.Sprintf("%s-%s", filepath, iso), nil +} + +// uniquePath returns the path, or if it exists, the path with a unique suffix. +func uniquePath(path string) (string, error) { + matches, err := filepath.Glob(path + "-*") + if err != nil { + return "", err + } + + suffixRe, err := regexp.Compile("-([0-9]+)$") + if err != nil { + return "", fmt.Errorf("compiling regular expression: %w", err) + } + + var maxSuffix int64 + for _, path := range matches { + matches := suffixRe.FindStringSubmatch(path) + if len(matches) < 2 { + continue + } + suffix := matches[1] + value, err := strconv.ParseInt(suffix, 10, 64) + if err != nil { + // Non-numeric suffixes are fine; they just don't count for this function. + continue + } + + if value > maxSuffix { + maxSuffix = value + } + } + + return fmt.Sprintf("%s-%03d", path, maxSuffix+1), nil +} diff --git a/internal/worker/command_file_mgmt_test.go b/internal/worker/command_file_mgmt_test.go new file mode 100644 index 00000000..1b9f33e7 --- /dev/null +++ b/internal/worker/command_file_mgmt_test.go @@ -0,0 +1,270 @@ +package worker + +// SPDX-License-Identifier: GPL-3.0-or-later + +import ( + "context" + "fmt" + "io/fs" + "os" + "path/filepath" + "testing" + "time" + + "git.blender.org/flamenco/pkg/api" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" +) + +type cmdMoveDirFixture struct { + mockCtrl *gomock.Controller + ce *CommandExecutor + mocks *CommandExecutorMocks + ctx context.Context + + temppath string + cwd string +} + +const ( + taskID = "90e9d656-e201-4ef0-b6b0-c80684fafa27" + sourcePath = "render/output/here__intermediate" + destPath = "render/output/here" +) + +func TestCmdMoveDirectoryNonExistentSourceDir(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + "move-directory: source path \"render/output/here__intermediate\" does not exist, not moving anything") + err := f.run() + assert.Error(t, err) +} + +func TestCmdMoveDirectoryHappy(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + ensureDirExists(sourcePath) + fileCreateEmpty(filepath.Join(sourcePath, "testfile.txt")) + + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + "move-directory: moving \"render/output/here__intermediate\" to \"render/output/here\"") + err := f.run() + assert.NoError(t, err) + + assert.NoDirExists(t, sourcePath) + assert.DirExists(t, destPath) + assert.NoFileExists(t, filepath.Join(sourcePath, "testfile.txt")) + assert.FileExists(t, filepath.Join(destPath, "testfile.txt")) +} + +func TestCmdMoveDirectoryExistingDest(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + mtime, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05-07:00") + assert.NoError(t, err) + + ensureDirExists(sourcePath) + ensureDirExists(destPath) + fileCreateEmpty(filepath.Join(sourcePath, "sourcefile.txt")) + fileCreateEmpty(filepath.Join(destPath, "destfile.txt")) + + // Change the atime/mtime of the directory after creating the files, otherwise + // it'll reset to "now". + if err := os.Chtimes(destPath, mtime, mtime); err != nil { + t.Fatalf("changing dir time: %v", err) + } + + // This cannot be a hard-coded string, as the test would fail in other timezones. + backupDir := destPath + "-2006-01-02_" + mtime.Local().Format("150405") + + // Just a sanity check. + ts, err := timestampedPath(destPath) + assert.NoError(t, err) + if !assert.Equal(t, backupDir, ts, "the test's sanity check failed") { + t.FailNow() + } + + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + fmt.Sprintf("move-directory: moving \"render/output/here\" to %q", backupDir)) + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + "move-directory: moving \"render/output/here__intermediate\" to \"render/output/here\"") + + assert.NoError(t, f.run()) + + assert.NoDirExists(t, sourcePath) + assert.DirExists(t, destPath) + assert.DirExists(t, backupDir, "old dest dir should have been moved to new location") + assert.NoFileExists(t, filepath.Join(sourcePath, "sourcefile.txt")) + assert.FileExists(t, filepath.Join(destPath, "sourcefile.txt")) + assert.FileExists(t, filepath.Join(backupDir, "destfile.txt")) +} + +func TestCmdMoveDirectoryExistingDestAndBackup(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + mtime, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05-07:00") + assert.NoError(t, err) + + ensureDirExists(sourcePath) + ensureDirExists(destPath) + fileCreateEmpty(filepath.Join(sourcePath, "sourcefile.txt")) + fileCreateEmpty(filepath.Join(destPath, "destfile.txt")) + + // This cannot be a hard-coded string, as the test would fail in other timezones. + backupDir := destPath + "-2006-01-02_" + mtime.Local().Format("150405") + ensureDirExists(backupDir) + ensureDirExists(backupDir + "-046") + fileCreateEmpty(filepath.Join(backupDir, "backupfile.txt")) + + // uniqueDir is where 'dest' will end up, because 'backupDir' already existed beforehand. + uniqueDir := backupDir + "-047" + + // Change the atime/mtime of the directory after creating the files, otherwise + // it'll reset to "now". + if err := os.Chtimes(destPath, mtime, mtime); err != nil { + t.Fatalf("changing dir time: %v", err) + } + + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + fmt.Sprintf("move-directory: moving \"render/output/here\" to %q", uniqueDir)) + f.mocks.listener.EXPECT().LogProduced(gomock.Any(), taskID, + "move-directory: moving \"render/output/here__intermediate\" to \"render/output/here\"") + + assert.NoError(t, f.run()) + + assert.NoDirExists(t, sourcePath) + assert.DirExists(t, destPath) + assert.DirExists(t, backupDir, "the backup directory should not have been removed") + assert.DirExists(t, uniqueDir, "old dest dir should have been moved to new unique location") + + assert.NoFileExists(t, filepath.Join(sourcePath, "sourcefile.txt")) + assert.FileExists(t, filepath.Join(destPath, "sourcefile.txt")) + assert.FileExists(t, filepath.Join(backupDir, "backupfile.txt"), "the original backup directory should not have been touched") + assert.FileExists(t, filepath.Join(uniqueDir, "destfile.txt"), "the dest dir should have been moved to a unique dir") +} + +func TestTimestampedPathFile(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + mtime, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05-07:00") + assert.NoError(t, err) + + fileCreateEmpty("somefile.txt") + os.Chtimes("somefile.txt", mtime, mtime) + + newpath, err := timestampedPath("somefile.txt") + + // This cannot be a hard-coded string, as the test would fail in other timezones. + expect := fmt.Sprintf("somefile.txt-2006-01-02_%s", mtime.Local().Format("150405")) + + assert.NoError(t, err) + assert.Equal(t, expect, newpath) +} + +func TestTimestampedPathDir(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + mtime, err := time.Parse(time.RFC3339, "2006-01-02T15:04:05-07:00") + assert.NoError(t, err) + + os.Mkdir("somedir", os.ModePerm) + os.Chtimes("somedir", mtime, mtime) + + newpath, err := timestampedPath("somedir") + + // This cannot be a hard-coded string, as the test would fail in other timezones. + expect := fmt.Sprintf("somedir-2006-01-02_%s", mtime.Local().Format("150405")) + + assert.NoError(t, err) + assert.Equal(t, expect, newpath) +} + +func TestUniquePath(t *testing.T) { + f := newCmdMoveDirectoryFixture(t) + defer f.finish(t) + + fileCreateEmpty("thefile.txt") + fileCreateEmpty("thefile.txt-1") + fileCreateEmpty("thefile.txt-003") + fileCreateEmpty("thefile.txt-46") + + newpath, err := uniquePath("thefile.txt") + assert.NoError(t, err) + assert.Equal(t, "thefile.txt-047", newpath) + + // Test with existing suffix longer than 3 digits. + fileCreateEmpty("thefile.txt-10327") + newpath, err = uniquePath("thefile.txt") + assert.NoError(t, err) + assert.Equal(t, "thefile.txt-10328", newpath) +} + +func newCmdMoveDirectoryFixture(t *testing.T) cmdMoveDirFixture { + mockCtrl := gomock.NewController(t) + ce, mocks := testCommandExecutor(t, mockCtrl) + + temppath, err := os.MkdirTemp("", "test-move-directory") + if err != nil { + t.Fatalf("unable to create temp dir: %v", err) + } + + cwd, err := os.Getwd() + if err != nil { + t.Fatalf("getcw: %v", err) + } + + if err := os.Chdir(temppath); err != nil { + t.Fatalf("chdir(%s): %v", temppath, err) + } + + return cmdMoveDirFixture{ + mockCtrl: mockCtrl, + ce: ce, + mocks: mocks, + ctx: context.Background(), + temppath: temppath, + cwd: cwd, + } +} + +func (f cmdMoveDirFixture) finish(t *testing.T) { + if err := os.Chdir(f.cwd); err != nil { + t.Fatalf("chdir(%s): %v", f.cwd, err) + } + + os.RemoveAll(f.temppath) + f.mockCtrl.Finish() +} + +func (f cmdMoveDirFixture) run() error { + cmd := api.Command{ + Name: "move-directory", + Parameters: map[string]interface{}{ + "src": sourcePath, + "dest": destPath, + }, + } + return f.ce.Run(f.ctx, taskID, cmd) +} + +func ensureDirExists(dirpath string) { + if err := os.MkdirAll(dirpath, fs.ModePerm); err != nil { + panic(fmt.Sprintf("unable to create dir %s: %v", dirpath, err)) + } +} + +func fileCreateEmpty(filename string) { + file, err := os.OpenFile(filename, os.O_CREATE|os.O_APPEND|os.O_RDONLY, 0666) + + if err != nil { + panic(err.Error()) + } + file.Close() +}