Skip to content

Commit 1382907

Browse files
pbalcerpbalcer
authored
Merge pull request #438 from PatKamin/logger-env-var-tests
[ur] Improve logger env var handling
2 parents ed77900 + 93754d2 commit 1382907

22 files changed

+604
-64
lines changed

‎cmake/match.cmake‎

Lines changed: 48 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,39 +8,74 @@
88

99
find_package(Python3 COMPONENTS Interpreter)
1010

11-
if(NOT DEFINED TEST_FILE)
12-
message(FATAL_ERROR "TEST_FILE needs to be defined")
11+
# Process variables passed to the script
12+
if(NOT DEFINED MODE)
13+
message(FATAL_ERROR "MODE needs to be defined. Possible values: 'stdout', 'stderr', 'file'")
14+
elseif(${MODE} STREQUAL "stdout" OR ${MODE} STREQUAL "stderr")
15+
if(NOT DEFINED TEST_FILE)
16+
message(FATAL_ERROR "TEST_FILE needs to be defined with a path to a binary to run")
17+
else()
18+
set(OUT_FILE "_matchtmpfile")
19+
endif()
20+
elseif(${MODE} STREQUAL "file")
21+
if(NOT DEFINED OUT_FILE)
22+
message(FATAL_ERROR "OUT_FILE needs to be defined with an output file to be verified")
23+
endif()
24+
else()
25+
message(FATAL_ERROR "${MODE} mode not recognised. Possible values: 'stdout', 'stderr', 'file'")
1326
endif()
1427
if(NOT DEFINED MATCH_FILE)
1528
message(FATAL_ERROR "MATCH_FILE needs to be defined")
1629
endif()
17-
18-
set(TEST_OUT "_matchtmpfile")
19-
2030
if(NOT DEFINED TEST_ARGS) # easier than ifdefing the rest of the code
2131
set(TEST_ARGS "")
2232
endif()
2333

2434
string(REPLACE "\"" "" TEST_ARGS "${TEST_ARGS}")
2535
separate_arguments(TEST_ARGS)
2636

27-
execute_process(
28-
COMMAND ${TEST_FILE} ${TEST_ARGS}
29-
OUTPUT_FILE ${TEST_OUT}
30-
RESULT_VARIABLE TEST_RESULT
31-
)
37+
if(EXISTS ${OUT_FILE})
38+
file(REMOVE ${OUT_FILE})
39+
endif()
40+
41+
# Run the test binary. Capture the output to the temporary file.
42+
if(${MODE} STREQUAL "stdout")
43+
execute_process(
44+
COMMAND ${TEST_FILE} ${TEST_ARGS}
45+
OUTPUT_FILE ${OUT_FILE}
46+
RESULT_VARIABLE TEST_RESULT
47+
)
48+
elseif(${MODE} STREQUAL "stderr")
49+
execute_process(
50+
COMMAND ${TEST_FILE} ${TEST_ARGS}
51+
ERROR_FILE ${OUT_FILE}
52+
RESULT_VARIABLE TEST_RESULT
53+
)
54+
elseif(${MODE} STREQUAL "file")
55+
execute_process(
56+
COMMAND ${TEST_FILE}
57+
RESULT_VARIABLE TEST_RESULT
58+
)
59+
endif()
3260

3361
if(TEST_RESULT)
3462
message(FATAL_ERROR "Failed: Test ${TEST_FILE} ${TEST_ARGS} returned non-zero (${TEST_RESULT}).")
3563
endif()
3664

65+
# Compare the output file contents with a match file contents
3766
execute_process(
38-
COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_LIST_DIR}/match.py ${TEST_OUT} ${MATCH_FILE}
67+
COMMAND ${Python3_EXECUTABLE} ${CMAKE_CURRENT_LIST_DIR}/match.py ${OUT_FILE} ${MATCH_FILE}
3968
RESULT_VARIABLE TEST_RESULT
4069
)
4170

4271
if(TEST_RESULT)
43-
message(FATAL_ERROR "Failed: The output of ${TEST_FILE} (stored in ${TEST_OUT}) does not match ${MATCH_FILE} (${TEST_RESULT})")
72+
file(READ ${OUT_FILE} OUTPUT)
73+
file(READ ${MATCH_FILE} MATCH_STRING)
74+
message(FATAL_ERROR "Failed: The output of ${OUT_FILE}:\n ${OUTPUT}\n does not match ${MATCH_FILE}:\n ${MATCH_STRING} (${TEST_RESULT})")
4475
elseif()
45-
message("Passed: The output of ${TEST_FILE} matches ${MATCH_FILE}")
76+
message("Passed: The output ${OUT_FILE} matches ${MATCH_FILE}")
77+
endif()
78+
79+
if(EXISTS ${OUT_FILE})
80+
file(REMOVE ${OUT_FILE})
4681
endif()

‎source/common/logger/ur_level.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ inline auto str_to_level(std::string name) {
3535
const lvl_name lvl_names[] = {{"debug", Level::DEBUG},
3636
{"info", Level::INFO},
3737
{"warning", Level::WARN},
38-
{"error", Level::WARN}};
38+
{"error", Level::ERR}};
3939

4040
for (auto const &item : lvl_names) {
4141
if (item.name.compare(name) == 0) {

‎source/common/logger/ur_logger.hpp‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -78,39 +78,50 @@ inline Logger create_logger(std::string logger_name, bool skip_prefix) {
7878
std::unique_ptr<logger::Sink> sink;
7979

8080
env_var_name << "UR_LOG_" << logger_name;
81-
auto map = getenv_to_map(env_var_name.str().c_str());
82-
if (!map.has_value()) {
83-
return Logger(
84-
std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
85-
}
86-
8781
try {
82+
auto map = getenv_to_map(env_var_name.str().c_str());
83+
if (!map.has_value()) {
84+
return Logger(
85+
std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
86+
}
87+
8888
auto kv = map->find("level");
8989
if (kv != map->end()) {
9090
auto value = kv->second.front();
9191
level = str_to_level(value);
92+
map->erase(kv);
9293
}
9394

9495
kv = map->find("flush");
9596
if (kv != map->end()) {
9697
auto value = kv->second.front();
9798
flush_level = str_to_level(value);
99+
map->erase(kv);
98100
}
99101

100102
std::vector<std::string> values = {default_output};
101103
kv = map->find("output");
102104
if (kv != map->end()) {
103105
values = kv->second;
106+
map->erase(kv);
107+
}
108+
109+
if (!map->empty()) {
110+
std::cerr << "Wrong logger environment variable parameter: '"
111+
<< map->begin()->first
112+
<< "'. Default logger options are set.";
113+
return Logger(
114+
std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
104115
}
105116

106117
sink =
107118
values.size() == 2
108119
? sink_from_str(logger_name, values[0], values[1], skip_prefix)
109120
: sink_from_str(logger_name, values[0], "", skip_prefix);
110121
} catch (const std::invalid_argument &e) {
111-
std::cerr
112-
<< "Error when creating a logger instance from environment variable"
113-
<< e.what();
122+
std::cerr << "Error when creating a logger instance from the '"
123+
<< env_var_name.str() << "' environment variable:\n"
124+
<< e.what() << std::endl;
114125
return Logger(
115126
std::make_unique<logger::StderrSink>(logger_name, skip_prefix));
116127
}

‎source/common/logger/ur_sinks.hpp‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include <fstream>
88
#include <iostream>
9+
#include <sstream>
910

1011
#if __has_include(<filesystem>)
1112
#include <filesystem>
@@ -142,12 +143,12 @@ class FileSink : public Sink {
142143
FileSink(std::string logger_name, filesystem::path file_path,
143144
bool skip_prefix = false)
144145
: Sink(logger_name, skip_prefix) {
145-
ofstream = std::ofstream(file_path, std::ofstream::out);
146+
ofstream = std::ofstream(file_path);
146147
if (!ofstream.good()) {
147-
throw std::invalid_argument(
148-
std::string("Failure while opening log file: ") +
149-
file_path.string() +
150-
std::string(" Check if given path exists."));
148+
std::stringstream ss;
149+
ss << "Failure while opening log file " << file_path.string()
150+
<< ". Check if given path exists.";
151+
throw std::invalid_argument(ss.str());
151152
}
152153
this->ostream = &ofstream;
153154
}
@@ -164,20 +165,20 @@ class FileSink : public Sink {
164165

165166
inline std::unique_ptr<Sink> sink_from_str(std::string logger_name,
166167
std::string name,
167-
std::string file_path = "",
168+
std::filesystem::path file_path = "",
168169
bool skip_prefix = false) {
169-
if (name == "stdout") {
170+
if (name == "stdout" && file_path.empty()) {
170171
return std::make_unique<logger::StdoutSink>(logger_name, skip_prefix);
171-
} else if (name == "stderr") {
172+
} else if (name == "stderr" && file_path.empty()) {
172173
return std::make_unique<logger::StderrSink>(logger_name, skip_prefix);
173174
} else if (name == "file" && !file_path.empty()) {
174-
return std::make_unique<logger::FileSink>(
175-
logger_name, file_path.c_str(), skip_prefix);
175+
return std::make_unique<logger::FileSink>(logger_name, file_path,
176+
skip_prefix);
176177
}
177178

178179
throw std::invalid_argument(
179180
std::string("Parsing error: no valid sink for string '") + name +
180-
std::string("' with path '") + file_path + std::string("'.") +
181+
std::string("' with path '") + file_path.string() + std::string("'.") +
181182
std::string("\nValid sink names are: stdout, stderr, file"));
182183
}
183184

‎source/common/ur_util.hpp‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,11 +107,13 @@ static void throw_wrong_format_vec(const char *env_var_name) {
107107
throw std::invalid_argument(ex_ss.str());
108108
}
109109

110-
static void throw_wrong_format_map(const char *env_var_name) {
110+
static void throw_wrong_format_map(const char *env_var_name,
111+
std::string env_var_value) {
111112
std::stringstream ex_ss;
112-
ex_ss << "Wrong format of the " << env_var_name << " environment variable!"
113-
<< " Proper format is: "
114-
"ENV_VAR=\"param_1:value_1,value_2;param_2:value_1";
113+
ex_ss << "Wrong format of the " << env_var_name
114+
<< " environment variable value: '" << env_var_value << "'\n"
115+
<< "Proper format is: "
116+
"ENV_VAR=\"param_1:value_1,value_2;param_2:value_1\"";
115117
throw std::invalid_argument(ex_ss.str());
116118
}
117119

@@ -188,31 +190,42 @@ inline std::optional<EnvVarMap> getenv_to_map(const char *env_var_name,
188190
return std::nullopt;
189191
}
190192

193+
auto is_quoted = [](std::string &str) {
194+
return (str.front() == '\'' && str.back() == '\'') ||
195+
(str.front() == '"' && str.back() == '"');
196+
};
197+
auto has_colon = [](std::string &str) {
198+
return str.find(':') != std::string::npos;
199+
};
200+
191201
std::stringstream ss(*env_var);
192202
std::string key_value;
193203
while (std::getline(ss, key_value, main_delim)) {
194204
std::string key;
195205
std::string values;
196206
std::stringstream kv_ss(key_value);
197207

198-
if (reject_empty && key_value.find(':') == std::string::npos) {
199-
throw_wrong_format_map(env_var_name);
208+
if (reject_empty && !has_colon(key_value)) {
209+
throw_wrong_format_map(env_var_name, *env_var);
200210
}
201211

202212
std::getline(kv_ss, key, key_value_delim);
203213
std::getline(kv_ss, values);
204214
if (key.empty() || (reject_empty && values.empty()) ||
205-
values.find(':') != std::string::npos ||
206215
map.find(key) != map.end()) {
207-
throw_wrong_format_map(env_var_name);
216+
throw_wrong_format_map(env_var_name, *env_var);
208217
}
209218

210219
std::vector<std::string> values_vec;
211220
std::stringstream values_ss(values);
212221
std::string value;
213222
while (std::getline(values_ss, value, values_delim)) {
214-
if (value.empty()) {
215-
throw_wrong_format_map(env_var_name);
223+
if (value.empty() || (has_colon(value) && !is_quoted(value))) {
224+
throw_wrong_format_map(env_var_name, *env_var);
225+
}
226+
if (is_quoted(value)) {
227+
value.erase(value.cbegin());
228+
value.pop_back();
216229
}
217230
values_vec.push_back(value);
218231
}

‎test/layers/tracing/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ set(TEST_NAME example-collected-hello-world)
55

66
add_test(NAME ${TEST_NAME}
77
COMMAND ${CMAKE_COMMAND}
8+
-D MODE=stdout
89
-D TEST_FILE=$<TARGET_FILE:hello_world>
910
-D MATCH_FILE=${CMAKE_CURRENT_SOURCE_DIR}/hello_world.out.match
1011
-P ${PROJECT_SOURCE_DIR}/cmake/match.cmake

‎test/tools/urtrace/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ function(add_trace_test name CLI_ARGS)
99
COMMAND ${CMAKE_COMMAND}
1010
-D TEST_FILE=${Python3_EXECUTABLE}
1111
-D TEST_ARGS="${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/urtrace --stdout ${CLI_ARGS} $<TARGET_FILE:hello_world>"
12+
-D MODE=stdout
1213
-D MATCH_FILE=${CMAKE_CURRENT_SOURCE_DIR}/${name}.match
1314
-P ${PROJECT_SOURCE_DIR}/cmake/match.cmake
1415
DEPENDS ur_trace_cli hello_world

0 commit comments

Comments
 (0)