Worker: check BLENDER_CMD environment variable (for multi-GPU Eevee rendering) #104193

Open
MKRelax wants to merge 9 commits from MKRelax/flamenco:worker-use-blender-from-env into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
4 changed files with 81 additions and 16 deletions
Showing only changes of commit d37d33a8b9 - Show all commits

View File

@ -53,8 +53,24 @@ func FileAssociation() (string, error) {
} }
// EnvironmentVariable returns the full path of a Blender executable, by checking the environment. // EnvironmentVariable returns the full path of a Blender executable, by checking the environment.
MKRelax marked this conversation as resolved Outdated

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.

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.
func EnvironmentVariable() string { func EnvironmentVariable() (string, string, error) {
return os.Getenv(BlenderPathEnvVariable)
// Check the environment for a full path
fullPath := os.Getenv(BlenderPathEnvVariable)
if fullPath == "" {
return BlenderPathEnvVariable, "", nil
}
// Make sure the path exists and is not a directory
stat, err := os.Stat(fullPath)
if err != nil {
return BlenderPathEnvVariable, fullPath, err
}
if stat.IsDir() {
return BlenderPathEnvVariable, fullPath, fmt.Errorf("Path from environment is a directory: %s", fullPath)
}
return BlenderPathEnvVariable, fullPath, nil
} }
func CheckBlender(ctx context.Context, exename string) (CheckBlenderResult, error) { func CheckBlender(ctx context.Context, exename string) (CheckBlenderResult, error) {
@ -66,10 +82,12 @@ func CheckBlender(ctx context.Context, exename string) (CheckBlenderResult, erro
} }
// Check the environment for a full path // Check the environment for a full path
envFullPath := EnvironmentVariable() _, envFullPath, err := EnvironmentVariable()
if err != nil {
return CheckBlenderResult{}, err
}
if envFullPath != "" { if envFullPath != "" {
// The full path was found in the environment, report the Blender version. return getResultWithVersion(ctx, exename, envFullPath, api.BlenderPathSourceEnvVariable)
return getResultWithVersion(ctx, exename, envFullPath, api.BlenderPathSourceInputPath)
} }
if exename == "" { if exename == "" {

View File

@ -7,6 +7,7 @@ import (
"flag" "flag"
"os" "os"
"os/exec" "os/exec"
"runtime"
"testing" "testing"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@ -45,21 +46,59 @@ func TestGetBlenderVersion(t *testing.T) {
func TestGetBlenderCommandFromEnvironment(t *testing.T) { func TestGetBlenderCommandFromEnvironment(t *testing.T) {
// If the environment variable is unset or empty, we expect an empty string // If the environment variable is unset, we expect an empty string
MKRelax marked this conversation as resolved Outdated

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 ;-)
err := os.Unsetenv(BlenderPathEnvVariable) err := os.Unsetenv(BlenderPathEnvVariable)
assert.NoError(t, err, "Could not clear blender executable from environment") assert.NoError(t, err, "Could not clear blender executable from environment")
path := EnvironmentVariable() envVar, path, err := EnvironmentVariable()
assert.NoError(t, err)
assert.Equal(t, BlenderPathEnvVariable, envVar)
assert.Equal(t, "", path) assert.Equal(t, "", path)
MKRelax marked this conversation as resolved Outdated

The if and t.Fatal() can be replaced with require.NoError(t, err, "Could not set BLENDER_CMD environment variable").

The `if` and `t.Fatal()` can be replaced with `require.NoError(t, err, "Could not set BLENDER_CMD environment variable")`.
// If the environment variable is empty, we expect an empty string
err = os.Setenv(BlenderPathEnvVariable, "") err = os.Setenv(BlenderPathEnvVariable, "")
assert.NoError(t, err, "Could not set blender executable in environment") assert.NoError(t, err, "Could not set blender executable in environment")
path = EnvironmentVariable() envVar, path, err = EnvironmentVariable()
assert.NoError(t, err)
assert.Equal(t, BlenderPathEnvVariable, envVar)
assert.Equal(t, "", path) assert.Equal(t, "", path)
// Try finding the blender path in the environment variable // Try finding a non-existing executable in the environment variable
envExe := `D:\Blender_3.2_stable\blender.exe` envExe := `D:\Blender_3.2_stable\blender.exe`
err = os.Setenv(BlenderPathEnvVariable, envExe) err = os.Setenv(BlenderPathEnvVariable, envExe)
assert.NoError(t, err, "Could not set blender executable in environment") assert.NoError(t, err, "Could not set blender executable in environment")
path = EnvironmentVariable() envVar, path, err = EnvironmentVariable()
assert.Equal(t, envExe, path) assert.Error(t, err)
assert.Equal(t, path, envExe)
assert.Equal(t, BlenderPathEnvVariable, envVar)
// Try finding a existing file in the environment variable
// We use this filename instead of a real blender executable
_, myFilename, _, _ := runtime.Caller(0)
err = os.Setenv(BlenderPathEnvVariable, myFilename)
assert.NoError(t, err, "Could not set blender executable in environment")
envVar, path, err = EnvironmentVariable()
assert.NoError(t, err)
assert.Equal(t, path, myFilename)
assert.Equal(t, BlenderPathEnvVariable, envVar)
if !*withBlender {
t.Skip("skipping test, -withBlender arg not passed")
}
existingPath, err := exec.LookPath("blender")
if err != nil {
existingPath, err = fileAssociation()
if !assert.NoError(t, err) {
t.Fatal("running with -withBlender requires having a `blender` command on $PATH or a file association to .blend files")
}
}
// Try finding an existing executable in the environment variable
err = os.Setenv(BlenderPathEnvVariable, existingPath)
assert.NoError(t, err, "Could not set blender executable in environment")
envVar, path, err = EnvironmentVariable()
assert.NoError(t, err)
assert.Equal(t, path, existingPath)
assert.Equal(t, BlenderPathEnvVariable, envVar)
} }

View File

@ -88,10 +88,14 @@ func (ce *CommandExecutor) cmdBlenderRenderCommand(
if crosspath.Dir(parameters.exe) == "." { if crosspath.Dir(parameters.exe) == "." {
// No directory path given. Check that the executable is set in an // No directory path given. Check that the executable is set in an
// environment variable or can be found on the path. // environment variable or can be found on the path.
if path := find_blender.EnvironmentVariable(); path != "" { envVar, envPath, err := find_blender.EnvironmentVariable()
msg := fmt.Sprintf("using blender from %s", find_blender.BlenderPathEnvVariable) if err != nil {
logger.Info().Str("path", path).Msg(msg) return nil, err

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}".

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}".

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.

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.

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.
parameters.exe = path }
if envPath != "" {
msg := fmt.Sprintf("using blender from %s", envVar)
logger.Info().Str("path", envPath).Msg(msg)
parameters.exe = envPath
} else if _, err := exec.LookPath(parameters.exe); err != nil { } else if _, err := exec.LookPath(parameters.exe); err != nil {
// 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

View File

@ -3,6 +3,7 @@ package worker
import ( import (
"context" "context"
"os" "os"
"runtime"
"testing" "testing"
"github.com/golang/mock/gomock" "github.com/golang/mock/gomock"
@ -87,7 +88,10 @@ func TestCmdBlenderFromEnvironment(t *testing.T) {
ce, mocks := testCommandExecutor(t, mockCtrl) ce, mocks := testCommandExecutor(t, mockCtrl)
envExe := `D:\Blender_3.2_stable\blender.exe` // We pass this file as blender executable because it must exist
// envExe := `D:\Blender_3.2_stable\blender.exe`
_, myFilename, _, _ := runtime.Caller(0)
envExe := myFilename
err := os.Setenv(find_blender.BlenderPathEnvVariable, envExe) err := os.Setenv(find_blender.BlenderPathEnvVariable, envExe)
assert.NoError(t, err, "Could not set blender executable in environment") assert.NoError(t, err, "Could not set blender executable in environment")