Fix aos cli autocomplete config check
ReadConfig does the work of finding the config path, so it shouldn't be
assumed to be in the current directory.
Signed-off-by: Milind Upadhyay <milind.upadhyay@gmail.com>
Change-Id: I20552dfc002bfd6235dd2ac981d1d8ee245bd2e9
Signed-off-by: milind-u <milind.upadhyay@gmail.com>
diff --git a/aos/aos_cli_utils.cc b/aos/aos_cli_utils.cc
index f71ed2a..970797a 100644
--- a/aos/aos_cli_utils.cc
+++ b/aos/aos_cli_utils.cc
@@ -36,16 +36,19 @@
bool expect_args) {
// Don't generate failure output if the config doesn't exist while attempting
// to autocomplete.
- if (struct stat file_stat;
- FLAGS__bash_autocomplete &&
- (!(EndsWith(FLAGS_config, ".json") || EndsWith(FLAGS_config, ".bfbs")) ||
- stat(FLAGS_config.c_str(), &file_stat) != 0 ||
- (file_stat.st_mode & S_IFMT) != S_IFREG)) {
+ if (FLAGS__bash_autocomplete &&
+ (!(EndsWith(FLAGS_config, ".json") || EndsWith(FLAGS_config, ".bfbs")))) {
std::cout << "COMPREPLY=()";
return true;
}
- config.emplace(aos::configuration::ReadConfig(FLAGS_config));
+ config = aos::configuration::MaybeReadConfig(FLAGS_config);
+ if (FLAGS__bash_autocomplete && !config.has_value()) {
+ std::cout << "COMPREPLY=()";
+ return true;
+ }
+ CHECK(config.has_value()) << "Could not read config. See above errors.";
+
event_loop.emplace(&config->message());
event_loop->SkipTimingReport();
event_loop->SkipAosLog();
diff --git a/aos/configuration.cc b/aos/configuration.cc
index bc50d30..ed99e2a 100644
--- a/aos/configuration.cc
+++ b/aos/configuration.cc
@@ -169,7 +169,7 @@
return absl::StrJoin(split, "/");
}
-FlatbufferDetachedBuffer<Configuration> ReadConfig(
+std::optional<FlatbufferDetachedBuffer<Configuration>> MaybeReadConfig(
const std::string_view path, absl::btree_set<std::string> *visited_paths,
const std::vector<std::string_view> &extra_import_paths) {
std::string binary_path = MaybeReplaceExtension(path, ".json", ".bfbs");
@@ -181,11 +181,12 @@
// instead. It is much faster to load .bfbs files than .json files.
if (!binary_path_exists && !util::PathExists(raw_path)) {
const bool path_is_absolute = raw_path.size() > 0 && raw_path[0] == '/';
- if (path_is_absolute) {
- CHECK(extra_import_paths.empty())
+ if (path_is_absolute && !extra_import_paths.empty()) {
+ LOG(ERROR)
<< "Can't specify extra import paths if attempting to read a config "
"file from an absolute path (path is "
<< raw_path << ").";
+ return std::nullopt;
}
bool found_path = false;
@@ -204,11 +205,15 @@
break;
}
}
- CHECK(found_path) << ": Failed to find file " << path << ".";
+ if (!found_path) {
+ LOG(ERROR) << ": Failed to find file " << path << ".";
+ return std::nullopt;
+ }
}
- FlatbufferDetachedBuffer<Configuration> config = ReadConfigFile(
- binary_path_exists ? binary_path : raw_path, binary_path_exists);
+ std::optional<FlatbufferDetachedBuffer<Configuration>> config =
+ ReadConfigFile(binary_path_exists ? binary_path : raw_path,
+ binary_path_exists);
// Depth first. Take the following example:
//
@@ -252,15 +257,16 @@
LOG(FATAL)
<< "Already imported " << path << " (i.e. " << absolute_path
<< "). See above for the files that have already been processed.";
+ return std::nullopt;
}
- if (config.message().has_imports()) {
+ if (config->message().has_imports()) {
// Capture the imports.
const flatbuffers::Vector<flatbuffers::Offset<flatbuffers::String>> *v =
- config.message().imports();
+ config->message().imports();
// And then wipe them. This gets GCed when we merge later.
- config.mutable_message()->clear_imports();
+ config->mutable_message()->clear_imports();
// Start with an empty configuration to merge into.
FlatbufferDetachedBuffer<Configuration> merged_config =
@@ -271,14 +277,17 @@
const std::string included_config =
path_folder + "/" + std::string(str->string_view());
+ const auto optional_config =
+ MaybeReadConfig(included_config, visited_paths, extra_import_paths);
+ if (!optional_config.has_value()) {
+ return std::nullopt;
+ }
// And them merge everything in.
- merged_config = MergeFlatBuffers(
- merged_config,
- ReadConfig(included_config, visited_paths, extra_import_paths));
+ merged_config = MergeFlatBuffers(merged_config, *optional_config);
}
// Finally, merge this file in.
- config = MergeFlatBuffers(merged_config, config);
+ config = MergeFlatBuffers(merged_config, *config);
}
return config;
}
@@ -701,7 +710,7 @@
return result;
}
-FlatbufferDetachedBuffer<Configuration> ReadConfig(
+std::optional<FlatbufferDetachedBuffer<Configuration>> MaybeReadConfig(
const std::string_view path,
const std::vector<std::string_view> &extra_import_paths) {
// Add the executable directory to the search path. That makes it so that
@@ -726,17 +735,29 @@
// We only want to read a file once. So track the visited files in a set.
absl::btree_set<std::string> visited_paths;
- FlatbufferDetachedBuffer<Configuration> read_config =
- ReadConfig(path, &visited_paths, extra_import_paths_with_exe);
+ std::optional<FlatbufferDetachedBuffer<Configuration>> read_config =
+ MaybeReadConfig(path, &visited_paths, extra_import_paths_with_exe);
+
+ if (read_config == std::nullopt) {
+ return read_config;
+ }
// If we only read one file, and it had a .bfbs extension, it has to be a
// fully formatted config. Do a quick verification and return it.
if (visited_paths.size() == 1 && EndsWith(*visited_paths.begin(), ".bfbs")) {
- ValidateConfiguration(read_config);
+ ValidateConfiguration(*read_config);
return read_config;
}
- return MergeConfiguration(read_config);
+ return MergeConfiguration(*read_config);
+}
+
+FlatbufferDetachedBuffer<Configuration> ReadConfig(
+ const std::string_view path,
+ const std::vector<std::string_view> &extra_import_paths) {
+ auto optional_config = MaybeReadConfig(path, extra_import_paths);
+ CHECK(optional_config) << "Could not read config. See above errors";
+ return std::move(*optional_config);
}
FlatbufferDetachedBuffer<Configuration> MergeWithConfig(
diff --git a/aos/configuration.h b/aos/configuration.h
index c0e2715..cb52d09 100644
--- a/aos/configuration.h
+++ b/aos/configuration.h
@@ -18,11 +18,17 @@
namespace configuration {
// Reads a json configuration. This includes all imports and merges. Note:
-// duplicate imports will result in a CHECK.
+// duplicate imports or invalid paths will result in a FATAL.
FlatbufferDetachedBuffer<Configuration> ReadConfig(
const std::string_view path,
const std::vector<std::string_view> &extra_import_paths = {});
+// Reads a json configuration. This includes all imports and merges. Returns
+// std::nullopt on duplicate imports or invalid paths.
+std::optional<FlatbufferDetachedBuffer<Configuration>> MaybeReadConfig(
+ const std::string_view path,
+ const std::vector<std::string_view> &extra_import_paths = {});
+
// Sorts and merges entries in a config.
FlatbufferDetachedBuffer<Configuration> MergeConfiguration(
const Flatbuffer<Configuration> &config);
diff --git a/aos/configuration_test.cc b/aos/configuration_test.cc
index f5081d2..b493058 100644
--- a/aos/configuration_test.cc
+++ b/aos/configuration_test.cc
@@ -95,6 +95,23 @@
"aos/testdata/config1_bad.json");
}
+// Tests that we die when we give an invalid path.
+TEST_F(ConfigurationDeathTest, NonexistentFile) {
+ EXPECT_DEATH(
+ {
+ FlatbufferDetachedBuffer<Configuration> config =
+ ReadConfig("nonexistent/config.json");
+ },
+ "above error");
+}
+
+// Tests that we return std::nullopt when we give an invalid path.
+TEST_F(ConfigurationTest, NonexistentFileOptional) {
+ std::optional<FlatbufferDetachedBuffer<Configuration>> config =
+ MaybeReadConfig("nonexistent/config.json");
+ EXPECT_FALSE(config.has_value());
+}
+
// Tests that we reject invalid channel names. This means any channels with //
// in their name, a trailing /, or regex characters.
TEST_F(ConfigurationDeathTest, InvalidChannelName) {
@@ -891,8 +908,8 @@
TEST_F(ConfigurationDeathTest, InvalidLoggerConfig) {
EXPECT_DEATH(
{
- FlatbufferDetachedBuffer<Configuration> config =
- ReadConfig(ArtifactPath("aos/testdata/invalid_logging_configuration.json"));
+ FlatbufferDetachedBuffer<Configuration> config = ReadConfig(
+ ArtifactPath("aos/testdata/invalid_logging_configuration.json"));
},
"Logging timestamps without data");
}