MIOpen
MIOpen copied to clipboard
fix handling environment variables on Windows
The PR fixes handling environment variables on Windows and adds some improvements to the interface. Below are the updated functions
-
env::value(VAR)
reads a value from VAR or returns the default value if VAR is not defined -
env::value_or(VAR, alt)
reads a value from VAR or returns thealt
value if VAR is not defined -
env::enabled(VAR)
returnstrue
if VAR is defined and holds atrue
value -
env::disabled(VAR)
returnstrue
if VAR is defined and holds afalse
value -
env::name(VAR)
returns a string with a VAR name -
env::clear(VAR)
undefines (unsets) a VAR for the local process (does not affect global system settings) -
env::update(VAR, value)
updates the value of a VAR for the local process. If VAR is not yet defined, it creates it -
if(VAR)
... orif(!VAR)
will tell you if VAR is defined or not defined, respectively
The difference between the previous function's interface and the new one is that the new one does automatic type casting (see examples). That includes reading values (env::value()
and env::value_or()
) and updating variables (env::update()
).
Below is the updated declaration macro
-
MIOPEN_DECLARE_ENV_VAR_*(name, [default_value], [create_if_missing])
compared to the previous definition, the declaration macros has two additional and optional arguments - the default value (if not specified, the default for the data type will be used), and flag which will define a variable if it is missing (not already defined). The default for the flag is not to create the variable.
Examples (see the updated code for more examples)
// declare SOME_VARIABLE with default value 0ULL
MIOPEN_DECLARE_ENV_VAR_UINT64(SOME_VARIABLE)
// declare OTHER_VARIABLE with default value "lorem ipsum"
MIOPEN_DECLARE_ENV_VAR_STR(OTHER_VARIABLE, "lorem ipsum")
// declare FLAG_VARIABLE with default value false, and create it if missing
// Notice: the default value needs to be provided if we want to use `create_if_missing` argument
MIOPEN_DECLARE_ENV_VAR_BOOL(FLAG_VARIABLE, false, true)
// example POD for automatic casting
enum some_type {
...
};
// no need for explicit casting from `unsigned long long` to `some_type`
// the compiler will do that automatically
some_type v = env::value(SOME_VARIABLE);
// read the value of OTHER_VARIABLE, and if not defined, ignore the default value
// with declaration macro and use "dolor sit amet instead
auto str = env::value_or(OTHER_VARIABLE, "dolor sit amet");
MIOPEN_DECLARE_ENV_VAR_UIN64(CHOOSE_ALGORITHM, 1)
...
struct Algorithm {
int value;
...
// override the default value for an algorithm with `value` if CHOOSE_ALGORITHM does not exists
// notice: no explicit cast is needed between `unsigned long long` and ` algorithmType` data type
algorithmType getAlgorithm() const { return env::value_or(CHOOSE_ALGORITHM, value); }
// use the default value for an algorithm if CHOOSE_ALGORITHM does not exists
algorithmType getDefaultAlgorithm() const { return env::value(CHOOSE_ALGORITHM); }
};
enum Flags {
A = 0,
B = 1,
...
K
};
// No need for casting to `unsigned long long`
MIOPEN_DECLARE_ENV_VAR_UINT64(ANOTHER_FLAG, Flags::D)
...
env::update(ANOTHER_FLAG, Flags::K);
// Read default PATH from env variable or use default like this
MIOPEN_DECLARE_ENV_VAR_STR(USER_PATH, fs::temp_directory_path().string())
@apwojcik HIP tidy is complaining about ctuOneDefinitionRuleViolation
, could you help to fix that?
@apwojcik Can we have a smaller PR that contains only functional changes and another NFC PR that contains only renames and namespace changes?
@apwojcik could you help to follow up on this PR? Thanks!
@apwojcik HIP tidy is complaining about
ctuOneDefinitionRuleViolation
, could you help to fix that?
This problem still exists, please check the latest CI status. Thanks!
@apwojcik are we still working on this change requested?
@apwojcik are we still working on this change requested?
Yes, but currently, I have other priorities. I will be back to this PR.
@CAHEK7 @atamazov @pfultz2 last call of review. Thanks!
@junliume, please ignore the Windows build failure; the #3043 will fix that.
I don't think that
setEnvironmentVariable
is required at all, it was misused in the tests and must be removed, but now it is even widely used.env::update(VAR, value)
must be used insted.But this PR is too huge and fragile, lets merge it and deal with
setEnvironmentVariable
later.
The setEnvironmentVariable
function is needed. It is used under the hood because it is a cross-platform implementation of setting/updating environment variables.
My first attempt 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).
I don't think that
setEnvironmentVariable
is required at all, it was misused in the tests and must be removed, but now it is even widely used.env::update(VAR, value)
must be used insted. But this PR is too huge and fragile, lets merge it and deal withsetEnvironmentVariable
later.The
setEnvironmentVariable
function is needed. It is used under the hood because it is a cross-platform implementation of setting/updating environment variables. My first attempt was withenv::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).
We were able to handle ODR violation before and it behaved as expected. We don't need OS for updating our internal state, that's important thing.
Initially I've thought about DEFINE
and DECLARE
macro, but we've found a solution without it.