Skip to content

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.

Sign up for GitHub

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

Open
DhanashreePetare wants to merge 6 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8719f2c
volk: prefer XDG config dir (XDG_CONFIG_HOME/.config/volk),tests and …
Jan 11, 2026
d1b742c
tests: rewrite test_volk_paths.cc portable; fix cmake and docs per re…
Jan 16, 2026
f4e66d5
tests: rewrite test_volk_paths.cc portable; fix cmake and docs per re…
Jan 16, 2026
b1d56d1
volk: prefer XDG config paths, ensure directories on write; add VolkP…
Jan 17, 2026
3ef4774
chore: small CMakeLists.txt comment fix
Jan 17, 2026
2850ec0
Address reviewer feedback: simplify volk_prefs.c, remove fmt FetchCon…
Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ message(STATUS " Modify using: -DENABLE_TESTING=ON/OFF")
########################################################################
option(ENABLE_PROFILING "Launch system profiler after build" OFF)
if(ENABLE_PROFILING)
option(ENABLE_XDG_CONFIG "Prefer XDG_CONFIG_HOME for config paths when enabled" OFF)
if(DEFINED VOLK_CONFIGPATH)
get_filename_component(VOLK_CONFIGPATH ${VOLK_CONFIGPATH} ABSOLUTE)
set(VOLK_CONFIGPATH "${VOLK_CONFIGPATH}/volk")
Expand All @@ -367,10 +368,22 @@ if(ENABLE_PROFILING)
STATUS "System profiling is enabled, using env path: $ENV{VOLK_CONFIGPATH}")
else()
message(STATUS "System profiling is enabled with default paths.")
if(DEFINED ENV{HOME})
set(VOLK_CONFIGPATH "$ENV{HOME}/.volk")
elseif(DEFINED ENV{APPDATA})
set(VOLK_CONFIGPATH "$ENV{APPDATA}/.volk")
if(ENABLE_XDG_CONFIG)
if(DEFINED ENV{XDG_CONFIG_HOME})
set(VOLK_CONFIGPATH "$ENV{XDG_CONFIG_HOME}/volk")
elseif(DEFINED ENV{HOME})
set(VOLK_CONFIGPATH "$ENV{HOME}/.config/volk")
elseif(DEFINED ENV{APPDATA})
# Windows APPDATA fallback
set(VOLK_CONFIGPATH "$ENV{APPDATA}/.volk")
endif()
else()
# Preserve previous behavior: prefer HOME/.volk (legacy) then APPDATA
if(DEFINED ENV{HOME})
set(VOLK_CONFIGPATH "$ENV{HOME}/.volk")
elseif(DEFINED ENV{APPDATA})
set(VOLK_CONFIGPATH "$ENV{APPDATA}/.volk")
endif()
endif()
endif()
else()
Expand Down
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,15 @@ $ sudo ldconfig
$ volk_profile
```

## Configuration location

On Linux and other UNIX-like systems VOLK follows the XDG Base Directory
Specification for user configuration files where possible. User configuration
is sought in the following order: `VOLK_CONFIGPATH` (if set), `$XDG_CONFIG_HOME/volk`,
`$HOME/.config/volk`, and finally the legacy `$HOME/.volk`. For write operations
the library will create the XDG config directory when needed. Use the
`VOLK_CONFIGPATH` environment variable to override or force a custom location.

#### Missing submodule
We use [cpu_features](https://github.com/google/cpu_features) to detect CPU features, e.g. AVX.
Some platforms require a very recent version that is not available through the appropriate package manager.
Expand Down
136 changes: 108 additions & 28 deletions lib/volk_prefs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -24,51 +27,128 @@ 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) {
/* helper to ensure directory exists for write-mode */
{
#if defined(_MSC_VER)
/* use _mkdir on MSVC */
#else
/* ensure sys/stat available */
#endif
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this doing?

}

/* 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;
}
}

/* 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
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The 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?

Copy link
Author

@DhanashreePetare DhanashreePetare Jan 19, 2026

Choose a reason for hiding this comment

The 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:
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

Copy link
Contributor

@jdemel jdemel Jan 29, 2026

Choose a reason for hiding this comment

The 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 user-local config file
home = getenv("HOME");
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 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)) {
/* 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;
}
}

// 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)) {
/* 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) {
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

strncpy(path, tmp, 512);
return;
}
}

// If still no path was found set path[0] to '0' and fall through
path[0] = 0;
/* 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;
}
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


path[0] = '\0';
return;
}

Expand Down
38 changes: 32 additions & 6 deletions tests/CMakeLists.txt
View file
Open in desktop
Copy link
Contributor

@jdemel jdemel Jan 15, 2026

Choose a reason for hiding this comment

The 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.

Copy link
Author

@DhanashreePetare DhanashreePetare Jan 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor

@jdemel jdemel Jan 29, 2026

Choose a reason for hiding this comment

The 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
Expand Up @@ -10,13 +10,39 @@ if(NOT ENABLE_TESTING)
return()
endif(NOT ENABLE_TESTING)

find_package(fmt)
find_package(GTest)
include(FetchContent)

if(NOT fmt_FOUND OR NOT GTest_FOUND)
message(warning "Missing fmtlib and/or googletest for this test suite")
return()
endif(NOT fmt_FOUND OR NOT GTest_FOUND)
# Prefer system fmt if available, otherwise fetch a pinned version.
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()
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Prefer system GTest if available, otherwise fetch googletest.
find_package(GTest QUIET)
if(NOT GTest_FOUND)
message(STATUS "GTest not found: fetching googletest via FetchContent")
set(BUILD_GMOCK OFF CACHE BOOL "Disable gmock" FORCE)
set(INSTALL_GTEST OFF CACHE BOOL "Disable gtest install" FORCE)
FetchContent_Declare(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
GIT_TAG release-1.12.1
Copy link
Contributor

@jdemel jdemel Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GIT_SHALLOW TRUE
)
FetchContent_GetProperties(googletest)
if(NOT googletest_POPULATED)
FetchContent_Populate(googletest)
add_subdirectory(${googletest_SOURCE_DIR} ${googletest_BINARY_DIR} EXCLUDE_FROM_ALL)
endif()
endif()

file(GLOB volk_test_files "test_*.cc")

Expand Down
Loading
Loading