-
Notifications
You must be signed in to change notification settings - Fork 216
volk: prefer XDG config dir for volk_config, add tests & docs (#792) #810
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
base: main
Are you sure you want to change the base?
Changes from all commits
8719f2c
d1b742c
f4e66d5
b1d56d1
3ef4774
2850ec0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,10 +12,13 @@ | |
| #include <stdlib.h> | ||
| #include <string.h> | ||
| #if defined(_MSC_VER) | ||
| #include <direct.h> | ||
| #include <io.h> | ||
| #define access _access | ||
| #define F_OK 0 | ||
| #else | ||
| #include <sys/stat.h> | ||
| #include <sys/types.h> | ||
| #include <unistd.h> | ||
| #endif | ||
| #include <volk/volk_prefs.h> | ||
|
|
@@ -24,52 +27,105 @@ void volk_get_config_path(char* path, bool read) | |
| { | ||
| if (!path) | ||
| return; | ||
| const char* suffix = "/.volk/volk_config"; | ||
| const char* suffix2 = "/volk/volk_config"; // non-hidden | ||
| char* home = NULL; | ||
| const char* legacy_suffix = "/.volk/volk_config"; | ||
| const char* nonhidden_suffix = "/volk/volk_config"; // non-hidden | ||
| char tmp[512] = { 0 }; | ||
|
|
||
| // allows config redirection via env variable | ||
| home = getenv("VOLK_CONFIGPATH"); | ||
| if (home != NULL) { | ||
| strncpy(path, home, 512); | ||
| strcat(path, suffix2); | ||
| if (!read || access(path, F_OK) != -1) { | ||
| /* 1) explicit override via VOLK_CONFIGPATH */ | ||
| const char* env_override = getenv("VOLK_CONFIGPATH"); | ||
| if (env_override) { | ||
| snprintf(tmp, sizeof(tmp), "%s%s", env_override, nonhidden_suffix); | ||
| if (!read || access(tmp, F_OK) == 0) { | ||
| strncpy(path, tmp, 512); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // check for user-local config file | ||
| home = getenv("HOME"); | ||
| if (home != NULL) { | ||
| strncpy(path, home, 512); | ||
| strcat(path, suffix); | ||
| if (!read || (access(path, F_OK) != -1)) { | ||
| /* 2) XDG_CONFIG_HOME/volk if XDG set */ | ||
| const char* xdg = getenv("XDG_CONFIG_HOME"); | ||
| if (xdg) { | ||
| snprintf(tmp, sizeof(tmp), "%s/volk/volk_config", xdg); | ||
| if (!read) { | ||
| char parent[512]; | ||
| char dir[512]; | ||
| /* ensure XDG_CONFIG_HOME exists, then create XDG/volk */ | ||
| snprintf(parent, sizeof(parent), "%s", xdg); | ||
| snprintf(dir, sizeof(dir), "%s/volk", xdg); | ||
| #if defined(_MSC_VER) | ||
| _mkdir(parent); | ||
| _mkdir(dir); | ||
| #else | ||
| struct stat st = { 0 }; | ||
| if (stat(parent, &st) == -1) { | ||
| mkdir(parent, 0755); | ||
| } | ||
| if (stat(dir, &st) == -1) { | ||
| mkdir(dir, 0755); | ||
| } | ||
| #endif | ||
|
Comment on lines
+54
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old function behavior was to just check for possible folders and return the path. Why is it necessary to create folders now?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original function returned a path string, but didn't guarantee that path could actually be used for writing. If a user called volk_get_config_path(path, false) (write-mode) and tried to write the config file, it would fail with a file I/O error if the parent directories didn't exist. I agree this might be outside the function's scope. Would you prefer:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Go with option B. This function should just discover where to look for files =D |
||
| } | ||
| if (!read || access(tmp, F_OK) == 0) { | ||
| strncpy(path, tmp, 512); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // check for config file in APPDATA (Windows) | ||
| home = getenv("APPDATA"); | ||
| if (home != NULL) { | ||
| strncpy(path, home, 512); | ||
| strcat(path, suffix); | ||
| if (!read || (access(path, F_OK) != -1)) { | ||
| /* 3) $HOME/.config/volk */ | ||
| const char* home = getenv("HOME"); | ||
| if (home) { | ||
| snprintf(tmp, sizeof(tmp), "%s/.config/volk/volk_config", home); | ||
| if (!read) { | ||
| char parent[512]; | ||
| char dir[512]; | ||
| /* ensure HOME/.config exists, then create HOME/.config/volk */ | ||
| snprintf(parent, sizeof(parent), "%s/.config", home); | ||
| snprintf(dir, sizeof(dir), "%s/.config/volk", home); | ||
| #if defined(_MSC_VER) | ||
| _mkdir(parent); | ||
| _mkdir(dir); | ||
| #else | ||
| struct stat st = { 0 }; | ||
| if (stat(parent, &st) == -1) { | ||
| mkdir(parent, 0755); | ||
| } | ||
| if (stat(dir, &st) == -1) { | ||
| mkdir(dir, 0755); | ||
| } | ||
| #endif | ||
| } | ||
| if (!read || access(tmp, F_OK) == 0) { | ||
| strncpy(path, tmp, 512); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // check for system-wide config file | ||
| if (access("/etc/volk/volk_config", F_OK) != -1) { | ||
| strncpy(path, "/etc", 512); | ||
| strcat(path, suffix2); | ||
| if (!read || (access(path, F_OK) != -1)) { | ||
| /* 4) legacy $HOME/.volk */ | ||
| if (home) { | ||
| snprintf(tmp, sizeof(tmp), "%s%s", home, legacy_suffix); | ||
| if (!read || access(tmp, F_OK) == 0) { | ||
| strncpy(path, tmp, 512); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // If still no path was found set path[0] to '0' and fall through | ||
| path[0] = 0; | ||
| return; | ||
| /* 5) Windows APPDATA fallback */ | ||
| const char* appdata = getenv("APPDATA"); | ||
| if (appdata) { | ||
| snprintf(tmp, sizeof(tmp), "%s%s", appdata, legacy_suffix); | ||
| if (!read || access(tmp, F_OK) == 0) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old behavior was |
||
| strncpy(path, tmp, 512); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| /* 6) System-wide */ | ||
| if (access("/etc/volk/volk_config", F_OK) == 0) { | ||
| strncpy(path, "/etc/volk/volk_config", 512); | ||
| return; | ||
| } | ||
|
|
||
| /* Nothing found - return empty path */ | ||
| path[0] = '\0'; | ||
| } | ||
|
|
||
| size_t volk_load_preferences(volk_arch_pref_t** prefs_res) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does anything in this file need to change? The intended behavior before was to skip over these tests of the required dependencies are missing.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to go with fetchContent after the check. One more thing though: We have similar stuff in other parts of our CMake structure. That'd need to be combined. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| // Tests for volk config path resolution (XDG preference, legacy fallback) | ||
| #include <gtest/gtest.h> | ||
| #include <volk/volk_prefs.h> | ||
|
|
||
| #include <filesystem> | ||
| #include <fstream> | ||
| #include <string> | ||
| #include <string_view> | ||
| #include <cstdlib> | ||
| #include <random> | ||
| #include <sstream> | ||
|
|
||
| #if defined(_WIN32) | ||
| static void set_env(std::string_view name, const char* value) | ||
| { | ||
| std::string n(name); | ||
| if (value) | ||
| _putenv_s(n.c_str(), value); | ||
| else | ||
| _putenv_s(n.c_str(), ""); | ||
| } | ||
| #else | ||
| static void set_env(std::string_view name, const char* value) | ||
| { | ||
| std::string n(name); | ||
| if (value) | ||
| setenv(n.c_str(), value, 1); | ||
| else | ||
| unsetenv(n.c_str()); | ||
| } | ||
| #endif | ||
|
|
||
| static std::string volk_config_path_read() | ||
| { | ||
| char buf[512] = {0}; | ||
| volk_get_config_path(buf, true); | ||
| return std::string(buf); | ||
| } | ||
|
|
||
| static std::string volk_config_path_write() | ||
| { | ||
| char buf[512] = {0}; | ||
| volk_get_config_path(buf, false); | ||
| return std::string(buf); | ||
| } | ||
|
|
||
| // Helper to generate unique temp directory names | ||
| static std::filesystem::path unique_temp_path(const char* prefix) | ||
| { | ||
| std::random_device rd; | ||
| std::mt19937 gen(rd()); | ||
| std::uniform_int_distribution<> distrib(100000, 999999); | ||
| std::ostringstream oss; | ||
| oss << prefix << "_" << distrib(gen); | ||
| return std::filesystem::temp_directory_path() / oss.str(); | ||
| } | ||
|
|
||
| TEST(VolkPaths, OverrideEnv) | ||
| { | ||
| namespace fs = std::filesystem; | ||
| fs::path tmp = unique_temp_path("volk_test_override"); | ||
| fs::create_directories(tmp / "volk"); | ||
| std::ofstream(tmp / "volk" / "volk_config") << "test"; | ||
|
|
||
| set_env("VOLK_CONFIGPATH", tmp.string().c_str()); | ||
|
|
||
| std::string expect = (tmp / "volk" / "volk_config").generic_string(); | ||
| std::string got = volk_config_path_read(); | ||
| EXPECT_EQ(expect, got); | ||
|
|
||
| // cleanup | ||
| set_env("VOLK_CONFIGPATH", nullptr); | ||
| fs::remove_all(tmp); | ||
| } | ||
|
|
||
| TEST(VolkPaths, XDGPreference) | ||
| { | ||
| namespace fs = std::filesystem; | ||
| fs::path tmp = unique_temp_path("volk_test_xdg"); | ||
| fs::create_directories(tmp / "volk"); | ||
| std::ofstream(tmp / "volk" / "volk_config") << "test"; | ||
|
|
||
| set_env("VOLK_CONFIGPATH", nullptr); | ||
| set_env("XDG_CONFIG_HOME", tmp.string().c_str()); | ||
|
|
||
| std::string expect = (tmp / "volk" / "volk_config").generic_string(); | ||
| std::string got = volk_config_path_read(); | ||
| EXPECT_EQ(expect, got); | ||
|
|
||
| set_env("XDG_CONFIG_HOME", nullptr); | ||
| fs::remove_all(tmp); | ||
| } | ||
|
|
||
| TEST(VolkPaths, HomeDotConfigFallback) | ||
| { | ||
| namespace fs = std::filesystem; | ||
| fs::path tmp = unique_temp_path("volk_test_homecfg"); | ||
| fs::create_directories(tmp / ".config" / "volk"); | ||
| std::ofstream(tmp / ".config" / "volk" / "volk_config") << "test"; | ||
|
|
||
| set_env("VOLK_CONFIGPATH", nullptr); | ||
| set_env("XDG_CONFIG_HOME", nullptr); | ||
| set_env("HOME", tmp.string().c_str()); | ||
|
|
||
| std::string expect = (tmp / ".config" / "volk" / "volk_config").generic_string(); | ||
| std::string got = volk_config_path_read(); | ||
| EXPECT_EQ(expect, got); | ||
|
|
||
| set_env("HOME", nullptr); | ||
| fs::remove_all(tmp); | ||
| } | ||
|
|
||
| TEST(VolkPaths, LegacyFallback) | ||
| { | ||
| namespace fs = std::filesystem; | ||
| fs::path tmp = unique_temp_path("volk_test_legacy"); | ||
| fs::create_directories(tmp / ".volk"); | ||
| std::ofstream(tmp / ".volk" / "volk_config") << "test"; | ||
|
|
||
| set_env("VOLK_CONFIGPATH", nullptr); | ||
| set_env("XDG_CONFIG_HOME", nullptr); | ||
| set_env("HOME", tmp.string().c_str()); | ||
|
|
||
| // Ensure .config/volk does not exist to force legacy path selection | ||
| fs::remove_all(tmp / ".config"); | ||
|
|
||
| std::string expect = (tmp / ".volk" / "volk_config").generic_string(); | ||
| std::string got = volk_config_path_read(); | ||
| EXPECT_EQ(expect, got); | ||
|
|
||
| set_env("HOME", nullptr); | ||
| fs::remove_all(tmp); | ||
| } | ||
|
|
||
| TEST(VolkPaths, WriteCreatesXDGDir) | ||
| { | ||
| namespace fs = std::filesystem; | ||
| fs::path tmp = unique_temp_path("volk_test_write"); | ||
|
|
||
| set_env("VOLK_CONFIGPATH", nullptr); | ||
| set_env("XDG_CONFIG_HOME", tmp.string().c_str()); | ||
|
|
||
| // Ensure directory does not exist yet | ||
| fs::remove_all(tmp); | ||
| EXPECT_FALSE(fs::exists(tmp / "volk")); | ||
|
|
||
| // Call write-mode; this should create the XDG/volk directory | ||
| volk_config_path_write(); | ||
| EXPECT_TRUE(fs::exists(tmp / "volk")); | ||
|
|
||
| set_env("XDG_CONFIG_HOME", nullptr); | ||
| fs::remove_all(tmp); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.