MIOpen icon indicating copy to clipboard operation
MIOpen copied to clipboard

fix handling environment variables on Windows

Open apwojcik opened this issue 11 months ago • 4 comments

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 the alt value if VAR is not defined
  • env::enabled(VAR) returns true if VAR is defined and holds a true value
  • env::disabled(VAR) returns true if VAR is defined and holds a false 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) ... or if(!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 avatar Mar 23 '24 02:03 apwojcik

@apwojcik HIP tidy is complaining about ctuOneDefinitionRuleViolation, could you help to fix that?

junliume avatar Apr 01 '24 16:04 junliume

@apwojcik Can we have a smaller PR that contains only functional changes and another NFC PR that contains only renames and namespace changes?

atamazov avatar Apr 05 '24 20:04 atamazov

@apwojcik could you help to follow up on this PR? Thanks!

junliume avatar Apr 08 '24 05:04 junliume

@apwojcik HIP tidy is complaining about ctuOneDefinitionRuleViolation, could you help to fix that?

This problem still exists, please check the latest CI status. Thanks!

junliume avatar Apr 17 '24 00:04 junliume

@apwojcik are we still working on this change requested?

junliume avatar May 17 '24 02:05 junliume

@apwojcik are we still working on this change requested?

Yes, but currently, I have other priorities. I will be back to this PR.

apwojcik avatar May 17 '24 12:05 apwojcik

@CAHEK7 @atamazov @pfultz2 last call of review. Thanks!

junliume avatar Jun 10 '24 05:06 junliume

@junliume, please ignore the Windows build failure; the #3043 will fix that.

apwojcik avatar Jun 11 '24 10:06 apwojcik

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).

apwojcik avatar Jun 12 '24 08:06 apwojcik

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).

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.

CAHEK7 avatar Jun 12 '24 11:06 CAHEK7