volk: prefer XDG config dir for volk_config, add tests & docs (#792) #810

DhanashreePetare · 2026-01-11T19:01:43Z

Summary:

Change volk to prefer XDG Base Directory for the user config file location (use $XDG_CONFIG_HOME/volk or $HOME/.config/volk) while preserving legacy fallback to $HOME/.volk and the system /etc/volk/volk_config. Add unit tests and documentation.

Motivation:

Follow the XDG Base Directory Specification for user configuration files so distro and user-managed config directories are respected and consistent with other Linux applications. This addresses issue Update Linux config file folder location to follow XDG base directory specification #792.

What I changed:

Code: Prefer XDG config dirs in volk_prefs.c — lookup order: VOLK_CONFIGPATH override → $XDG_CONFIG_HOME/volk → $HOME/.config/volk → legacy $HOME/.volk → /etc/volk/volk_config. When writing, create the chosen XDG/home .config/volk directory if missing.
CMake: Adjust default VOLK_CONFIGPATH used for profiling to prefer XDG config dir in CMakeLists.txt.
Tests: Add test_volk_paths.cc to exercise override, XDG presence, write-mode directory creation, and legacy fallback. Update CMakeLists.txt to FetchContent fmt and googletest if not available.
Docs: Append note in README.md explaining XDG behavior and backward compatibility.
Repo: Changes are on branch feature/xdg-config-path.

Testing done / expected:

Added unit tests covering new lookup and write behavior; CI will run the full build+test matrix across supported compilers/OS. Local build on Windows required toolchain upgrades; recommend CI verification or running in a modern toolchain/WSL matching CI.

Backward compatibility:

Legacy behavior preserved: VOLK_CONFIGPATH still overrides; if XDG and $HOME/.config/volk are not available, the code falls back to $HOME/.volk and /etc/volk/volk_config.

Notes for reviewers:

Focus review on volk_prefs.c (path selection + directory-creation logic), test_volk_paths.cc (coverage), and the small CMakeLists.txt change for profiling default. CI will validate formatting and cross-platform builds.
Commits include DCO sign-off and were formatted with project clang-format.

Closes: #792

marcusmueller · 2026-01-12T13:31:37Z

tests/CMakeLists.txt

+include(FetchContent)
+
+find_package(fmt QUIET)
+    find_package(fmt QUIET)
+    find_package(GTest QUIET)
+    GIT_REPOSITORY https://github.com/fmtlib/fmt.git
+    GIT_TAG 10.2.1
+    GIT_SHALLOW TRUE)
+  FetchContent_MakeAvailable(fmt)
+endif()
+
+if(NOT GTest_FOUND)
+  message(STATUS "GTest not found: fetching googletest via FetchContent")
+  FetchContent_Declare(
+    googletest
+    GIT_REPOSITORY https://github.com/google/googletest.git
+    GIT_TAG release-1.12.1
+    GIT_SHALLOW TRUE)
+  FetchContent_MakeAvailable(googletest)
+  find_package(GTest REQUIRED)
+endif()


voice from the off: generally, I'd avoid random FetchContents in CMake appearing, because it makes packaging much harder.

However, here, FetchContent is only used when no system lib has been found, AND it fixes the version fetched, so I feel that's OK.

marcusmueller

Please fix the syntax errors and make sure things build and test successfully on your machine :)

marcusmueller · 2026-01-12T13:32:56Z

tests/CMakeLists.txt

+find_package(fmt QUIET)
+    find_package(fmt QUIET)
+    find_package(GTest QUIET)
+    GIT_REPOSITORY https://github.com/fmtlib/fmt.git
+    GIT_TAG 10.2.1
+    GIT_SHALLOW TRUE)
+  FetchContent_MakeAvailable(fmt)
+endif()


syntax error!

CMake Error at tests/CMakeLists.txt:18: Parse error. Expected "(", got unquoted argument with text "https://github.com/fmtlib/fmt.git".

Have you tried to build this on your machine?

jdemel

Thank you for your PR. Overall, it looks good. I feel like my comments should only be minor changes. The CI needs to pass again though.

jdemel · 2026-01-15T20:12:03Z

CMakeLists.txt

+        elseif(DEFINED ENV{HOME})
+            set(VOLK_CONFIGPATH "$ENV{HOME}/.config/volk")


I feel like this is a good change but: Unfortunately this breaks the previous behavior. Right now, I'm undecided if this is an acceptable thing to do. Adding a new option with XDG_CONFIG_HOME is fine though.

jdemel · 2026-01-15T20:12:33Z

CMakeLists.txt

+        elseif(DEFINED ENV{HOME})
+            set(VOLK_CONFIGPATH "$ENV{HOME}/.config/volk")
        elseif(DEFINED ENV{APPDATA})
+            # Windows fallback


The comment is misleading because there's no other option for the Windows case.

README.md

jdemel · 2026-01-15T20:21:16Z

tests/CMakeLists.txt

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.

I made the CMakeLists.txt fetch fmt / googletest only as a fallback when the system packages are not found (it still prefers find_package(...) first). This keeps CI reproducible by pinning a known-good version while not forcing all contributors to install those libs system-wide.

I also used FetchContent_Populate + add_subdirectory for googletest to work around a CMake/FetchContent issue we hit on Windows/WSL mounts.

If you'd rather keep the prior behavior (skip tests when deps are missing) I'm happy to revert to that — or I can gate the FetchContent behind an option so packagers can opt out.

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.

jdemel · 2026-01-15T20:24:22Z

tests/test_volk_paths.cc

+#if defined(_MSC_VER)
+static void set_env(const char* name, const char* value)
+{
+    _putenv_s(name, value ? value : "");
+}
+#else
+static void set_env(const char* name, const char* value)
+{
+    if (value)
+        setenv(name, value, 1);
+    else
+        unsetenv(name);
+}
+#endif


Since these are C++ tests, you can use const std::string_view, or (the older approach) const std::string&.

jdemel · 2026-01-15T20:28:38Z

tests/test_volk_paths.cc

+int main(int argc, char** argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}


This shouldn't be a part of the file. So far, the googletest default main suffices and I can't find a reason why this should change here.

…aths tests; address reviewer suggestions (refs gnuradio#792 gnuradio#810)

…docs Signed-off-by: DhanashreePetare <[email protected]>

…viewers Signed-off-by: DhanashreePetare <[email protected]>

…aths tests; address reviewer suggestions (refs gnuradio#792 gnuradio#810) Signed-off-by: DhanashreePetare <[email protected]>

Signed-off-by: DhanashreePetare <[email protected]>

DhanashreePetare · 2026-01-17T10:19:07Z

Adds opt-in XDG config path support while preserving legacy fallbacks, ensures dirs are created on write, adds portable std::filesystem tests and fixes tests CMake; addresses reviewer feedback (use std::string_view, removed custom test main). DCO-signed and force-pushed — tested locally (WSL native build), all tests passed (253/253).

jdemel · 2026-01-18T17:45:19Z

lib/volk_prefs.c

+#if defined(_MSC_VER)
+        /* use _mkdir on MSVC */
+#else
+        /* ensure sys/stat available */
+#endif


What is this doing?

jdemel · 2026-01-18T17:49:30Z

lib/volk_prefs.c

+#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


The old function behavior was to just check for possible folders and return the path. Why is it necessary to create folders now?

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:
Option A: Keep current behavior (create dirs in write-mode) — simpler for callers
Option B: Return path only, let caller handle directory creation — better separation of concerns

Go with option B. This function should just discover where to look for files =D

jdemel · 2026-01-18T17:51:18Z

lib/volk_prefs.c

+    const char* appdata = getenv("APPDATA");
+    if (appdata) {
+        snprintf(tmp, sizeof(tmp), "%s%s", appdata, legacy_suffix);
+        if (!read || access(tmp, F_OK) == 0) {


The old behavior was access(tmp, F_OK) != -1, why does that need to change?

jdemel · 2026-01-18T17:55:51Z

lib/volk_prefs.c

+    /* System-wide */
+    if (access("/etc/volk/volk_config", F_OK) == 0) {
+        strncpy(path, "/etc/volk/volk_config", 512);
+        return;
+    }
+    /* If nothing found, follow the XDG-first fallback behavior:
+       - if XDG_CONFIG_HOME is set, return XDG_CONFIG_HOME/volk/volk_config
+       - else if HOME is set, return HOME/.config/volk/volk_config
+       - otherwise return empty string
+       This ensures read-mode returns the new XDG location as fallback. */
+    if (xdg) {
+        snprintf(tmp, sizeof(tmp), "%s/volk/volk_config", xdg);
+        strncpy(path, tmp, 512);
+        return;
+    }
+    if (home) {
+        snprintf(tmp, sizeof(tmp), "%s/.config/volk/volk_config", home);
+        strncpy(path, tmp, 512);
+        return;
+    }


The old function implementation followed a regular pattern to check for possible config folders and reacted accordingly. Could you restore the old structure and add the XDG part accordingly?
Introducing a new variable for every option is fine and probably improves readability.
Could you clean-up the implementation in this function?

jdemel · 2026-01-18T17:59:49Z

tests/CMakeLists.txt

+find_package(fmt QUIET)
+if(NOT fmt_FOUND)
+  message(STATUS "fmt not found: fetching fmt via FetchContent")
+  FetchContent_Declare(
+    fmt
+    GIT_REPOSITORY https://github.com/fmtlib/fmt.git
+    GIT_TAG 10.2.1
+    GIT_SHALLOW TRUE
+  )
+  FetchContent_MakeAvailable(fmt)
+endif()


fmt is available at this point because we either do find_package or FetchContent in the top-level CMakeLists.txt. Since it is required now, for other reasons, no need to fetch it here.

jdemel · 2026-01-18T18:01:02Z

tests/CMakeLists.txt

+  FetchContent_Declare(
+    googletest
+    GIT_REPOSITORY https://github.com/google/googletest.git
+    GIT_TAG release-1.12.1


please go with the latest release version. That'd be 1.17 at the time of writing.

…tent, update googletest to v1.17.0 Signed-off-by: DhanashreePetare <[email protected]>

DhanashreePetare · 2026-01-19T11:58:35Z

Addressed reviewer feedback: simplified volk_prefs.c fallback logic (removed duplicate XDG/HOME checks at end), removed empty comment block scaffolding, standardized access() checks to == 0 for consistency. Removed fmt FetchContent from CMakeLists.txt (already provided by top-level). Updated googletest to v1.17.0. Tested locally in WSL (native build): all 253 tests pass, build clean with Ninja/GCC.

jdemel · 2026-01-29T20:16:22Z

lib/volk_prefs.c

+#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


Go with option B. This function should just discover where to look for files =D

jdemel · 2026-01-29T20:19:24Z

tests/CMakeLists.txt

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.

marcusmueller reviewed Jan 12, 2026

View reviewed changes

marcusmueller requested changes Jan 12, 2026

View reviewed changes

jdemel reviewed Jan 15, 2026

View reviewed changes

DhanashreePetare pushed a commit to DhanashreePetare/GNUradio-volk that referenced this pull request Jan 17, 2026

volk: prefer XDG config paths, ensure directories on write; add VolkP…

895f45b

…aths tests; address reviewer suggestions (refs gnuradio#792 gnuradio#810)

DhanashreePetare added 5 commits January 17, 2026 15:35

volk: prefer XDG config dir (XDG_CONFIG_HOME/.config/volk),tests and …

8719f2c

…docs Signed-off-by: DhanashreePetare <[email protected]>

tests: rewrite test_volk_paths.cc portable; fix cmake and docs per re…

d1b742c

…viewers Signed-off-by: DhanashreePetare <[email protected]>

tests: rewrite test_volk_paths.cc portable; fix cmake and docs per re…

f4e66d5

…viewers Signed-off-by: DhanashreePetare <[email protected]>

volk: prefer XDG config paths, ensure directories on write; add VolkP…

b1d56d1

…aths tests; address reviewer suggestions (refs gnuradio#792 gnuradio#810) Signed-off-by: DhanashreePetare <[email protected]>

chore: small CMakeLists.txt comment fix

3ef4774

Signed-off-by: DhanashreePetare <[email protected]>

DhanashreePetare force-pushed the feature/xdg-config-path branch from 895f45b to 3ef4774 Compare January 17, 2026 10:05

jdemel linked an issue Jan 18, 2026 that may be closed by this pull request

Update Linux config file folder location to follow XDG base directory specification #792

Open

jdemel requested changes Jan 18, 2026

View reviewed changes

Address reviewer feedback: simplify volk_prefs.c, remove fmt FetchCon…

2850ec0

…tent, update googletest to v1.17.0 Signed-off-by: DhanashreePetare <[email protected]>

jdemel requested changes Jan 29, 2026

View reviewed changes

		elseif(DEFINED ENV{HOME})
		set(VOLK_CONFIGPATH "$ENV{HOME}/.config/volk")

volk: prefer XDG config dir for volk_config, add tests & docs (#792) #810

Are you sure you want to change the base?

volk: prefer XDG config dir for volk_config, add tests & docs (#792) #810

Uh oh!

Conversation

DhanashreePetare commented Jan 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcusmueller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdemel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DhanashreePetare commented Jan 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DhanashreePetare commented Jan 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants