From 6d1ff41d37b83108fdce899d118c71bff975c931 Mon Sep 17 00:00:00 2001 From: TheDoctor314 <64731940+TheDoctor314@users.noreply.github.com> Date: Wed, 20 Oct 2021 16:01:15 +0530 Subject: [PATCH] Handle relative includes (#2535) * Handle relative includes We change to the directory of the given config file before parsing. This allows us to handle relative includes. TODO: Maybe improve the name of the change_dir() function. * Fix unused result warning * Add `relative_to` parameter to expand() If the path is relative, we resolve it by prepending dirname(config) to the path. Add dirname() - Returns the parent directory of the file or an empty string. * Resolve relative paths Handle paths relative to the current file being parsed. * Remove unneeded change_dir() * Fix expand() `is_absolute` is calculated after we expand the path. `relative_to` must be a directory. Add test for expand() with relative paths * Recalculate `is_absolute` after expanding `path` * Add more file_util::expand tests * Add changelog Co-authored-by: patrick96 --- CHANGELOG.md | 3 +++ include/utils/file.hpp | 3 ++- src/components/config_parser.cpp | 6 +++-- src/utils/file.cpp | 24 ++++++++++++++--- tests/unit_tests/utils/file.cpp | 44 +++++++++++++++++++++++++++----- 5 files changed, 66 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6fcd6b2..33264c74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -164,6 +164,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Polybar can now be run without passing the bar name as argument given that the configuration file only defines one bar ([`#2525`](https://github.com/polybar/polybar/issues/2525)) +- `include-directory` and `include-file` now support relative paths. The paths + are relative to the folder of the file where those directives appear. + ([`#2523`](https://github.com/polybar/polybar/issues/2523)) ### Fixed - `custom/script`: Concurrency issues with fast-updating tailed scripts. diff --git a/include/utils/file.hpp b/include/utils/file.hpp index a76e73db..213bf6cd 100644 --- a/include/utils/file.hpp +++ b/include/utils/file.hpp @@ -88,9 +88,10 @@ namespace file_util { void write_contents(const string& filename, const string& contents); bool is_fifo(const string& filename); vector glob(string pattern); - const string expand(const string& path); + string expand(const string& path, const string& relative_to = {}); string get_config_path(); vector list_files(const string& dirname); + string dirname(const string& path); template decltype(auto) make_file_descriptor(Args&&... args) { diff --git a/src/components/config_parser.cpp b/src/components/config_parser.cpp index 645ceda3..034a0f2a 100644 --- a/src/components/config_parser.cpp +++ b/src/components/config_parser.cpp @@ -161,6 +161,8 @@ void config_parser::parse_file(const string& file, file_list path) { throw application_error("Failed to open config file " + file + ": " + strerror(errno)); } + auto dirname = file_util::dirname(file); + while (std::getline(in, line_str)) { line_no++; line_t line; @@ -174,9 +176,9 @@ void config_parser::parse_file(const string& file, file_list path) { } if (!line.is_header && line.key == "include-file") { - parse_file(file_util::expand(line.value), path); + parse_file(file_util::expand(line.value, dirname), path); } else if (!line.is_header && line.key == "include-directory") { - const string expanded_path = file_util::expand(line.value); + const string expanded_path = file_util::expand(line.value, dirname); vector file_list = file_util::list_files(expanded_path); sort(file_list.begin(), file_list.end()); for (const auto& filename : file_list) { diff --git a/src/utils/file.cpp b/src/utils/file.cpp index 87f65e7c..bcf6ba26 100644 --- a/src/utils/file.cpp +++ b/src/utils/file.cpp @@ -237,15 +237,16 @@ namespace file_util { /** * Path expansion + * + * `relative_to` must be a directory */ - const string expand(const string& path) { - string ret; + string expand(const string& path, const string& relative_to) { /* * This doesn't capture all cases for absolute paths but the other cases * (tilde and env variable) have the initial '/' character in their * expansion already and will thus not require adding '/' to the beginning. */ - bool is_absolute = path.size() > 0 && path.at(0) == '/'; + bool is_absolute = !path.empty() && (path.at(0) == '/'); vector p_exploded = string_util::split(path, '/'); for (auto& section : p_exploded) { switch (section[0]) { @@ -257,11 +258,17 @@ namespace file_util { break; } } - ret = string_util::join(p_exploded, "/"); + string ret = string_util::join(p_exploded, "/"); // Don't add an initial slash for relative paths if (ret[0] != '/' && is_absolute) { ret.insert(0, 1, '/'); } + + is_absolute = !ret.empty() && (ret.at(0) == '/'); + + if (!is_absolute && !relative_to.empty()) { + return relative_to + "/" + ret; + } return ret; } @@ -333,6 +340,15 @@ namespace file_util { } throw system_error("Failed to open directory stream for " + dirname); } + + string dirname(const string& path) { + const auto pos = path.find_last_of('/'); + if (pos != string::npos) { + return path.substr(0, pos + 1); + } + + return string{}; + } } // namespace file_util POLYBAR_NS_END diff --git a/tests/unit_tests/utils/file.cpp b/tests/unit_tests/utils/file.cpp index 1c303dc3..6e424565 100644 --- a/tests/unit_tests/utils/file.cpp +++ b/tests/unit_tests/utils/file.cpp @@ -1,16 +1,46 @@ +#include "utils/file.hpp" + #include #include #include "common/test.hpp" #include "utils/command.hpp" -#include "utils/file.hpp" +#include "utils/env.hpp" using namespace polybar; -TEST(File, expand) { - auto cmd = command_util::make_command("echo $HOME"); - cmd->exec(); - cmd->tail([](string home) { - EXPECT_EQ(home + "/test", file_util::expand("~/test")); - }); +using expand_test_t = pair; +class ExpandTest : public testing::TestWithParam {}; + +vector expand_absolute_test_list = { + {"~/foo", env_util::get("HOME") + "/foo"}, + {"$HOME/foo", env_util::get("HOME") + "/foo"}, + {"/scratch/polybar", "/scratch/polybar"}, +}; + +INSTANTIATE_TEST_SUITE_P(Inst, ExpandTest, ::testing::ValuesIn(expand_absolute_test_list)); + +TEST_P(ExpandTest, absolute) { + EXPECT_EQ(file_util::expand(GetParam().first), GetParam().second); +} + +TEST_P(ExpandTest, relativeToAbsolute) { + EXPECT_EQ(file_util::expand(GetParam().first, "/scratch"), GetParam().second); +} + +using expand_relative_test_t = std::tuple; +class ExpandRelativeTest : public testing::TestWithParam {}; + +vector expand_relative_test_list = { + {"../test", "/scratch", "/scratch/../test"}, + {"modules/battery", "/scratch/polybar", "/scratch/polybar/modules/battery"}, + {"/tmp/foo", "/scratch", "/tmp/foo"}, +}; + +INSTANTIATE_TEST_SUITE_P(Inst, ExpandRelativeTest, ::testing::ValuesIn(expand_relative_test_list)); + +TEST_P(ExpandRelativeTest, correctness) { + string path, relative_to, expected; + std::tie(path, relative_to, expected) = GetParam(); + EXPECT_EQ(file_util::expand(path, relative_to), expected); }