ROCm / MIOpen

AMD's Machine Intelligence Library
https://rocm.docs.amd.com/projects/MIOpen/en/latest/
Other
1.08k stars 228 forks source link

Remove `setEnvironmentVariable` from the tests and use `env::update(VAR, value)` instead. #3037

Open CAHEK7 opened 5 months ago

CAHEK7 commented 5 months ago

Based on https://github.com/ROCm/MIOpen/pull/2837#pullrequestreview-2107431371 setEnvironmentVariable should not be used in the tests, since it's an overkill and we don't need to update OS environment to get our internal state updated. setEnvironmentVariable was misused on early stage of gtest transition, it was before we've got special functions for proper internal state updating. Now env::update(VAR, value) does exactly what we need.

apwojcik commented 5 months ago

My first attempt in #2837 was with env::update(), but that broke one definition rule, and I got clang-tidy errors. To make it work, we need to define all environment variables in a single source file and create an interface to import names into units where we need to work with values (DEFINE and DECLARE macros).

apwojcik commented 5 months ago

Furthermore, the call to the setEnvironemntVariable function does not alter the OS environment variables. Environment variables set with it will only exist for the lifetime of a process. After the program terminates, all variables are removed from the caller's environment. That is because the setenv()/putenv() function on Linux and the SetEnvironmentVariable() function on Windows work only in the local environment. To update the environment variables on Windows globally (OS level), one must update the Windows registry. On Linux, on the other hand, one needs to run a new shell instance with an updated variables list.

CAHEK7 commented 5 months ago

In any case, it goes to OS for no reasons - with all those string names, reparsing and stuff, while we have to update just one global variable.