Worker: check BLENDER_CMD environment variable (for multi-GPU Eevee rendering) #104193
@ -50,6 +50,11 @@ func FileAssociation() (string, error) {
|
|||||||
return fileAssociation()
|
return fileAssociation()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// EnvironmentVariable returns the content of the BLENDER_CMD environment variable.
|
||||||
|
func EnvironmentVariable() string {
|
||||||
|
return os.Getenv("BLENDER_CMD")
|
||||||
MKRelax marked this conversation as resolved
Outdated
|
|||||||
|
}
|
||||||
|
|
||||||
func CheckBlender(ctx context.Context, exename string) (CheckBlenderResult, error) {
|
func CheckBlender(ctx context.Context, exename string) (CheckBlenderResult, error) {
|
||||||
if exename == "" {
|
if exename == "" {
|
||||||
// exename is not given, see if we can use .blend file association.
|
// exename is not given, see if we can use .blend file association.
|
||||||
|
@ -5,6 +5,7 @@ package find_blender
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"flag"
|
"flag"
|
||||||
|
"os"
|
||||||
"os/exec"
|
"os/exec"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
@ -41,3 +42,20 @@ func TestGetBlenderVersion(t *testing.T) {
|
|||||||
assert.ErrorIs(t, err, exec.ErrNotFound)
|
assert.ErrorIs(t, err, exec.ErrNotFound)
|
||||||
assert.Empty(t, version)
|
assert.Empty(t, version)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestGetBlenderCommandFromEnvironment(t *testing.T) {
|
||||||
|
|
||||||
|
// Without environment variable, we expect an empty string
|
||||||
|
path := EnvironmentVariable()
|
||||||
MKRelax marked this conversation as resolved
Outdated
Sybren A. Stüvel
commented
This should check that the environment variable isn't already set while running the test ;-) This should check that the environment variable isn't already set while running the test ;-)
|
|||||||
|
assert.Equal(t, "", path)
|
||||||
|
|
||||||
|
// Try finding the blender path in the BLENDER_CMD environment variable
|
||||||
|
err := os.Setenv("BLENDER_CMD", "/path/specified/in/env/to/blender")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal("Could not set BLENDER_CMD environment variable")
|
||||||
MKRelax marked this conversation as resolved
Outdated
Sybren A. Stüvel
commented
The The `if` and `t.Fatal()` can be replaced with `require.NoError(t, err, "Could not set BLENDER_CMD environment variable")`.
|
|||||||
|
}
|
||||||
|
|
||||||
|
path = EnvironmentVariable()
|
||||||
|
assert.Equal(t, "/path/specified/in/env/to/blender", path)
|
||||||
|
|
||||||
|
}
|
||||||
|
@ -85,9 +85,12 @@ func (ce *CommandExecutor) cmdBlenderRenderCommand(
|
|||||||
}
|
}
|
||||||
|
|
||||||
if crosspath.Dir(parameters.exe) == "." {
|
if crosspath.Dir(parameters.exe) == "." {
|
||||||
// No directory path given. Check that the executable can be found on the
|
// No directory path given. Check that the executable can be found in the
|
||||||
// path.
|
// environment variable BLENDER_CMD or on the path.
|
||||||
if _, err := exec.LookPath(parameters.exe); err != nil {
|
if path := find_blender.EnvironmentVariable(); path != "" {
|
||||||
|
logger.Info().Str("path", path).Msg("found Blender in environment")
|
||||||
|
parameters.exe = path
|
||||||
|
} else if _, err := exec.LookPath(parameters.exe); err != nil {
|
||||||
Sybren A. Stüvel
commented
I wouldn't say "found Blender" here, because the path wasn't checked. Blender may not even exist there. Maybe I wouldn't say "found Blender" here, because the path wasn't checked. Blender may not even exist there.
Maybe `find_blender.EnvironmentVariable()` can return two strings, `(name, path)` where `name` is the name of the environment variable, and `path` is the actual value. That way you can log here "using Blender from {environment variable name}".
MKRelax
commented
I did change the log message to "using Blender from FLAMENCO_BLENDER_PATH". However, since the name of the variable is (now) a constant I think there is no need to return it from I did change the log message to "using Blender from FLAMENCO_BLENDER_PATH".
However, since the name of the variable is (now) a constant I think there is no need to return it from `find_blender.EnvironmentVariable()`. Most code would not be interested in it anyway and probably ignore it using a `_` variable. So the log message simply uses the constant.
Sybren A. Stüvel
commented
I think it's still good to return the variable name. That way you get less coupling between the different parts of the code, making things more maintainable. I think it's still good to return the variable name. That way you get less coupling between the different parts of the code, making things more maintainable.
|
|||||||
// Attempt a platform-specific way to find which Blender executable to
|
// Attempt a platform-specific way to find which Blender executable to
|
||||||
// use. If Blender cannot not be found, just use the configured command
|
// use. If Blender cannot not be found, just use the configured command
|
||||||
// and let the OS produce the errors.
|
// and let the OS produce the errors.
|
||||||
|
@ -2,6 +2,7 @@ package worker
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"os"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/golang/mock/gomock"
|
"github.com/golang/mock/gomock"
|
||||||
@ -79,6 +80,41 @@ func TestCmdBlenderCliArgsInExeParameter(t *testing.T) {
|
|||||||
assert.Equal(t, ErrNoExecCmd, err, "nil *exec.Cmd should result in ErrNoExecCmd")
|
assert.Equal(t, ErrNoExecCmd, err, "nil *exec.Cmd should result in ErrNoExecCmd")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestCmdBlenderFromEnvironment(t *testing.T) {
|
||||||
|
mockCtrl := gomock.NewController(t)
|
||||||
|
defer mockCtrl.Finish()
|
||||||
|
|
||||||
|
ce, mocks := testCommandExecutor(t, mockCtrl)
|
||||||
|
|
||||||
|
envExe := `D:\Blender_3.2_stable\blender.exe`
|
||||||
|
|
||||||
|
// This should use blender.exe from the BLENDER_CMD environment variable
|
||||||
|
err := os.Setenv("BLENDER_CMD", envExe)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatal("Could not set BLENDER_CMD environment variable")
|
||||||
|
}
|
||||||
|
|
||||||
|
taskID := "c5dfdfab-4492-4ab1-9b38-8ca4cbd84a17"
|
||||||
|
cmd := api.Command{
|
||||||
|
Name: "blender",
|
||||||
|
Parameters: map[string]interface{}{
|
||||||
|
// Passing "blender", the environment path should override this
|
||||||
|
"exe": "blender",
|
||||||
|
"argsBefore": []string{},
|
||||||
|
"blendfile": "file.blend",
|
||||||
|
"args": []string{},
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
mocks.cli.EXPECT().CommandContext(gomock.Any(),
|
||||||
|
envExe, // from environment variable
|
||||||
|
"file.blend", // from 'blendfile'
|
||||||
|
).Return(nil)
|
||||||
|
|
||||||
|
err = ce.cmdBlenderRender(context.Background(), zerolog.Nop(), taskID, cmd)
|
||||||
|
assert.Equal(t, ErrNoExecCmd, err, "nil *exec.Cmd should result in ErrNoExecCmd")
|
||||||
|
}
|
||||||
|
|
||||||
func TestProcessLineBlender(t *testing.T) {
|
func TestProcessLineBlender(t *testing.T) {
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
mockCtrl := gomock.NewController(t)
|
mockCtrl := gomock.NewController(t)
|
||||||
|
I think the name of this variable should be in a constant. Can be in the same file. That way the string literal doesn't get repeated everywhere.