diff --git a/src/common/SystemUtils.cpp b/src/common/SystemUtils.cpp index 73aa4ee640..f8282eb414 100644 --- a/src/common/SystemUtils.cpp +++ b/src/common/SystemUtils.cpp @@ -14,6 +14,8 @@ #include "common/SystemUtils.h" +#include "common/Assert.h" + #if defined(DAWN_PLATFORM_WINDOWS) # include # include @@ -115,3 +117,27 @@ std::string GetExecutableDirectory() { size_t lastPathSepLoc = exePath.find_last_of(GetPathSeparator()); return lastPathSepLoc != std::string::npos ? exePath.substr(0, lastPathSepLoc + 1) : ""; } + +// ScopedEnvironmentVar + +ScopedEnvironmentVar::ScopedEnvironmentVar(const char* variableName, const char* value) + : mName(variableName), + mOriginalValue(GetEnvironmentVar(variableName)), + mIsSet(SetEnvironmentVar(variableName, value)) { +} + +ScopedEnvironmentVar::~ScopedEnvironmentVar() { + if (mIsSet) { + bool success = SetEnvironmentVar(mName.c_str(), mOriginalValue.c_str()); + // If we set the environment variable in the constructor, we should never fail restoring it. + ASSERT(success); + } +} + +bool ScopedEnvironmentVar::Set(const char* variableName, const char* value) { + ASSERT(!mIsSet); + mName = variableName; + mOriginalValue = GetEnvironmentVar(variableName); + mIsSet = SetEnvironmentVar(variableName, value); + return mIsSet; +} diff --git a/src/common/SystemUtils.h b/src/common/SystemUtils.h index 2edf1e3a25..ed18c31e66 100644 --- a/src/common/SystemUtils.h +++ b/src/common/SystemUtils.h @@ -24,4 +24,21 @@ std::string GetEnvironmentVar(const char* variableName); bool SetEnvironmentVar(const char* variableName, const char* value); std::string GetExecutableDirectory(); +class ScopedEnvironmentVar { + public: + ScopedEnvironmentVar() = default; + ScopedEnvironmentVar(const char* variableName, const char* value); + ~ScopedEnvironmentVar(); + + ScopedEnvironmentVar(const ScopedEnvironmentVar& rhs) = delete; + ScopedEnvironmentVar& operator=(const ScopedEnvironmentVar& rhs) = delete; + + bool Set(const char* variableName, const char* value); + + private: + std::string mName; + std::string mOriginalValue; + bool mIsSet = false; +}; + #endif // COMMON_SYSTEMUTILS_H_ diff --git a/src/dawn_native/vulkan/BackendVk.cpp b/src/dawn_native/vulkan/BackendVk.cpp index eef62e8d39..fe9eba6dbc 100644 --- a/src/dawn_native/vulkan/BackendVk.cpp +++ b/src/dawn_native/vulkan/BackendVk.cpp @@ -109,14 +109,16 @@ namespace dawn_native { namespace vulkan { MaybeError Backend::Initialize(bool useSwiftshader) { DAWN_TRY(LoadVulkan(useSwiftshader)); - // TODO(crbug.com/dawn/406): In order to not modify the environment variables of - // the rest of an application embedding Dawn, we should set these only - // in the scope of this function. See ANGLE's ScopedVkLoaderEnvironment + // These environment variables need only be set while loading procs and gathering device + // info. + ScopedEnvironmentVar vkICDFilenames; + ScopedEnvironmentVar vkLayerPath; + if (useSwiftshader) { #if defined(DAWN_SWIFTSHADER_VK_ICD_JSON) std::string fullSwiftshaderICDPath = GetExecutableDirectory() + DAWN_SWIFTSHADER_VK_ICD_JSON; - if (!SetEnvironmentVar("VK_ICD_FILENAMES", fullSwiftshaderICDPath.c_str())) { + if (!vkICDFilenames.Set("VK_ICD_FILENAMES", fullSwiftshaderICDPath.c_str())) { return DAWN_INTERNAL_ERROR("Couldn't set VK_ICD_FILENAMES"); } #else @@ -128,7 +130,7 @@ namespace dawn_native { namespace vulkan { if (GetInstance()->IsBackendValidationEnabled()) { #if defined(DAWN_ENABLE_VULKAN_VALIDATION_LAYERS) std::string vkDataDir = GetExecutableDirectory() + DAWN_VK_DATA_DIR; - if (!SetEnvironmentVar("VK_LAYER_PATH", vkDataDir.c_str())) { + if (!vkLayerPath.Set("VK_LAYER_PATH", vkDataDir.c_str())) { return DAWN_INTERNAL_ERROR("Couldn't set VK_LAYER_PATH"); } #else diff --git a/src/tests/unittests/SystemUtilsTests.cpp b/src/tests/unittests/SystemUtilsTests.cpp index 540f4ef931..3730f45f13 100644 --- a/src/tests/unittests/SystemUtilsTests.cpp +++ b/src/tests/unittests/SystemUtilsTests.cpp @@ -39,3 +39,48 @@ TEST(SystemUtilsTests, GetExecutableDirectory) { // Test last charecter in path ASSERT_EQ(GetExecutableDirectory().back(), *GetPathSeparator()); } + +// Tests for ScopedEnvironmentVar +TEST(SystemUtilsTests, ScopedEnvironmentVar) { + SetEnvironmentVar("ScopedEnvironmentVarForTest", "original"); + + // Test empty environment variable doesn't crash + { ScopedEnvironmentVar var; } + + // Test setting empty environment variable + { + ScopedEnvironmentVar var; + var.Set("ScopedEnvironmentVarForTest", "NewEnvironmentVarValue"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "NewEnvironmentVarValue"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test that the environment variable can be set, and it is unset at the end of the scope. + { + ScopedEnvironmentVar var("ScopedEnvironmentVarForTest", "NewEnvironmentVarValue"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "NewEnvironmentVarValue"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test nested scopes + { + ScopedEnvironmentVar outer("ScopedEnvironmentVarForTest", "outer"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer"); + { + ScopedEnvironmentVar inner("ScopedEnvironmentVarForTest", "inner"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "inner"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test redundantly setting scoped variables + { + ScopedEnvironmentVar var1("ScopedEnvironmentVarForTest", "var1"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var1"); + + ScopedEnvironmentVar var2("ScopedEnvironmentVarForTest", "var2"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var2"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); +}