Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Environment Variable Support #83

Merged
merged 9 commits into from
May 20, 2024

Conversation

fosterbrereton
Copy link
Contributor

@fosterbrereton fosterbrereton commented May 17, 2024

With this PR, ORC now uses environment variables to override its configuration values. The environment variables, if set, will usurp any defaults or values found in the orc-config file. Environment variables can be used to override any ORC configuration setting except those that take lists (symbol_ignore, violation_report, and violation_ignore).

For any given orc-config setting, the equivalent environment variable is ORC_ appended to the name of the setting in all caps. So output_file can be overridden by ORC_OUTPUT_FILE, parallel_processing by ORC_PARALLEL_PROCESSING, etc.

This PR also comes with additional tests that leverage this new capability.

Comment on lines +108 to +118
template <typename T>
T derive_configuration(const char* key,
const toml::parse_result& settings,
T&& fallback) {
T result = settings[key].value_or(fallback);
std::string envar = toupper(std::string("ORC_") + key);
if (const char* enval = std::getenv(envar.c_str())) {
result = parse_enval<T>(enval);
}
return result;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This routine is the heart of the PR. Instead of taking values from the settings file directly, derive_configuration will also look at the associated environment variable and use that if found.

@@ -32,7 +32,12 @@ jobs:
id: build-orc-orc_dogfood
continue-on-error: true
run: |
xcodebuild -project ./build/orc.xcodeproj -scheme orc_dogfood -configuration Debug
ORC_OUTPUT_FILE=./output.json ORC_OUTPUT_FILE_MODE=json xcodebuild -json -project ./build/orc.xcodeproj -scheme orc_dogfood -configuration Debug -hideShellScriptEnvironment
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new test that leverages the environment variables to cause a JSON file to get dumped when running orc_dogfood (which is a CMake target that uses ORC to build ORC). The JSON is then traversed with a couple jq queries to make sure some boilerplate keys are in there and filled in with something meaningful.

@fosterbrereton fosterbrereton requested a review from baheath May 20, 2024 16:06
Copy link
Contributor

@leethomason leethomason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor question on the string move - I didn't follow it through to see if that saved something, or the move semantics aren't needed.

T parse_enval(std::string&&) = delete;

template <>
std::string parse_enval(std::string&& x) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why move semantics instead of const std::string&?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. the value coming from getenv is a const char*, which is then converted to a std::string for this routine. For the std::string specialization of parse_enval, we can move the incoming string to the result with no additional changes. If the incoming string were const std::string&, it would cost us a copy to make a new one to return by value.

@fosterbrereton fosterbrereton merged commit 29e5cde into main May 20, 2024
3 checks passed
@fosterbrereton fosterbrereton deleted the fosterbrereton/env-var-config-support branch May 20, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants