Skip to content

Log the origin of manifest search paths#1878

Closed
GloriousTacoo wants to merge 3 commits intoKhronosGroup:mainfrom
GloriousTacoo:loader-search-path
Closed

Log the origin of manifest search paths#1878
GloriousTacoo wants to merge 3 commits intoKhronosGroup:mainfrom
GloriousTacoo:loader-search-path

Conversation

@GloriousTacoo
Copy link
Copy Markdown

@GloriousTacoo GloriousTacoo commented Mar 13, 2026

An attempt to fix Issue #1401.

I have a solid idea on how to log manifest path origins using a custom dynamic list, but I will need to setup infrastructure for this new list. Then I can replace the core function copy_data_file_info() with my own implementation.

[X] Create an API for for this new dynamic list (init, destroy, append, etc.).
[X] Replace copy_data_file_info().

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 13, 2026

CLA assistant check
All committers have signed the CLA.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

5 similar comments
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo
Copy link
Copy Markdown
Author

I'm trying to add a new test file to the test_regression executable to test the functions in loader.h but got the following error:

[2/3] Building CXX object tests/CMakeFiles/test_regression.dir/loader_search_path_list_tests.cpp.o
FAILED: [code=1] tests/CMakeFiles/test_regression.dir/loader_search_path_list_tests.cpp.o 
/usr/lib/ccache/bin/c++ -DDEBUG -DEXTRASYSCONFDIR=\"/etc\" -DFALLBACK_CONFIG_DIRS=\"/etc/xdg\" -DFALLBACK_DATA_DIRS=\"/usr/local/share:/usr/share\" -DFRAMEWORK_CONFIG_HEADER=\"/home/apollo/dev/Vulkan-Loader/build/tests/framework/framework_config_Debug.h\" -DGIT_BRANCH_NAME=\"loader-search-path\" -DGIT_TAG_INFO=\"v1.4.345-6-g19fbea66d\" -DHAVE_REALPATH -DHAVE_SECURE_GETENV -DSYSCONFDIR=\"/usr/local/etc\" -DVK_ENABLE_BETA_EXTENSIONS -DVK_NO_PROTOTYPES -DVK_USE_PLATFORM_WAYLAND_KHR -DVK_USE_PLATFORM_XCB_KHR -DVK_USE_PLATFORM_XLIB_KHR -DVK_USE_PLATFORM_XLIB_XRANDR_EXT -I/home/apollo/dev/Vulkan-Loader/build/tests/framework -I/home/apollo/dev/Vulkan-Loader/tests/framework/util -I/home/apollo/dev/Vulkan-Loader/tests/framework/util/.. -I/home/apollo/dev/Vulkan-Loader/build/tests/framework/util -I/home/apollo/dev/Vulkan-Loader -I/home/apollo/dev/Vulkan-Loader/build/tests/framework/shim -I/home/apollo/dev/Vulkan-Loader/tests/framework/shim -isystem /home/apollo/dev/Vulkan-Loader/external/Debug/64/googletest/googletest/include -isystem /home/apollo/dev/Vulkan-Loader/external/Debug/64/googletest/googletest -isystem /home/apollo/dev/Vulkan-Headers/build/install/include -g -std=c++17 -fvisibility=hidden -fdiagnostics-color=always -fPIC -MD -MT tests/CMakeFiles/test_regression.dir/loader_search_path_list_tests.cpp.o -MF tests/CMakeFiles/test_regression.dir/loader_search_path_list_tests.cpp.o.d -o tests/CMakeFiles/test_regression.dir/loader_search_path_list_tests.cpp.o -c /home/apollo/dev/Vulkan-Loader/tests/loader_search_path_list_tests.cpp
In file included from /home/apollo/dev/Vulkan-Loader/loader/loader.h:32,
                 from /home/apollo/dev/Vulkan-Loader/tests/loader_search_path_list_tests.cpp:2:
/home/apollo/dev/Vulkan-Loader/loader/loader_common.h:39:10: fatal error: vk_layer_dispatch_table.h: No such file or directory
   39 | #include "vk_layer_dispatch_table.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.

Steps to reproduce:

  1. Create a new test file in tests/.
  2. Add the following to the file.
#include "test_environment.h"
#include "loader/loader.h"

TEST(Test, Test) {}
  1. Add it to test_regression executable in tests/CMakeLists.txt
  2. Compile tests.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo
Copy link
Copy Markdown
Author

GloriousTacoo commented Mar 16, 2026

Adding my tests to test_regression looks to be impossible, cause I have to link vulkan to to it which breaks a couple existing tests. Looks like I will have to create a new executable to test the new changes in loader.h

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

Copy link
Copy Markdown
Collaborator

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

I'm happy to help figure out why the tests do not compile, but github shows only whitespace changes for the new test file.

I can say up front that a vast majority of the tests are 'integration' tests. Testing log output should be done by adding a debug utils logger to the instance create info (other tests do it if you need reference) then looking to make sure the log has the right strings.

While it is conceivable to create a new loader/modify the binary to allow unit testing, this hasn't been done as it means the tests are able to execute on any random loader binary. (of course there are exceptions, the fuzz tests require exporting the handful of functions used in those tests, of which they are all about loading & parsing json files.)

@GloriousTacoo
Copy link
Copy Markdown
Author

GloriousTacoo commented Mar 17, 2026

I'll explain the exact steps I took for the test to fail. Also I'm not testing log output per say. I'm testing every function I create in loader.h to make sure there are not any side effects.

@GloriousTacoo
Copy link
Copy Markdown
Author

GloriousTacoo commented Mar 17, 2026

When this PR is finish logs could look something like this but the exact format can be decided by you.

# OLD
[Vulkan Loader] LAYER:          Searching for implicit layer manifest files
[Vulkan Loader] LAYER:             In following locations:
[Vulkan Loader] LAYER:                /home/fake_home/.config/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /etc/xdg/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /home/fake_home/.local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:             Found the following files:
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json (file version 1.0.0)
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json (file version 1.0.0)
# NEW
[Vulkan Loader] LAYER:          Searching for implicit layer manifest files
[Vulkan Loader] LAYER:             In XDG_DATA_DIRS:
[Vulkan Loader] LAYER:                /usr/local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /home/fake_home/.local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:             Found no files
[Vulkan Loader] LAYER:             In SYSCONFIG:
[Vulkan Loader] LAYER:                /etc/xdg/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:             Found the following files:
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json (file version 1.0.0)
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json (file version 1.0.0)

@GloriousTacoo
Copy link
Copy Markdown
Author

GloriousTacoo commented Mar 17, 2026

To replicate the failed tests you need to link vulkan directly to test_regression in tests/CMakeLists.txt.

target_link_libraries(test_regression PUBLIC testing_dependencies vulkan)

5 tests fail. This is why I created a new test executable, link vulkan to it, and exposed the internal functions, but I see this is wrong now.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo
Copy link
Copy Markdown
Author

Not gonna lie testing my changes without exposing internal functions is difficult. I'll probably drop the test commits once the PR is finished.

Copy link
Copy Markdown
Collaborator

@charles-lunarg charles-lunarg left a comment

Choose a reason for hiding this comment

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

Yeah the way you have written the tests are in line with "unit tests" while the test suite is primarily composed of 'integration' tests.

Because the loader is a C library, there isn't a great way to test individual functions without statically linking the loader. Or exporting functions we want to test with, which definitely is hacky. Refactoring the loader into two libraries - a static one that contains all the functionality and a shared one that links to the static one then implements all the required exports & behavior the loader needs to have - is something I support but haven't had the impetus to do so. The original design of the tests was to not require modifying the loader source code itself in any way, as there was no significant tests of the loader and so we didn't want to break behavior by changing the loader in order to add tests. That was years ago now and the test suite is pretty extensive, so it is reasonable to explore modifying the loader source in order to write tests. But, as much as I agree that the current style of test writing is obnoxious (more setup, harder to validate behavior), there are non-trivial difficulties making individual functions unit-testable. Best thing to do is to create an issue describing how you would like to write tests, and see if we can't hash out details on how that might be achieved. I know how to write tests (cause I wrote the framework), I'd love to have another's opinions on it.

And to not just ramble, here are some actual code review comments.

loader/loader.c Outdated
Comment on lines +1063 to +1064
search_paths->list[search_paths->count] = *path;
++search_paths->count;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This would be a good place to do de-duplication.

LOADER_SEARCH_PATH_ORIGIN_XDG_CONFIG,
LOADER_SEARCH_PATH_ORIGIN_XDG_DATA,
LOADER_SEARCH_PATH_ORIGIN_SYSCONFIG,
LOADER_SEARCH_PATH_ORIGIN_HOME,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Quite a few different ways for manifests to be found are missing from this enum.

  • Add env-vars (VK_ADD_LAYER_PATH, VK_ADD_IMPLICIT_LAYER_PATH, VK_ADD_DRIVER_FILES)
  • Doesn't differentiate XDG_DATA_HOME & XDG_DATA_DIRS, as well as XDG_CONFIG_HOME & XDG_CONFIG_DIRS
  • macOS/iOS app bundles
  • The ever confusing EXTRASYSCONFDIR (which normally is /etc, but is build time configurable...)
  • windows "app package path" (special path microsoft added for compatibility layers found on the windows store)

There are enough different ways to find manifests that I wonder if the strategy is to just have a string that can contain 'anything' instead of locking it down into an ENUM. Plus, you'd have to create an enum->string helper for it anyway.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo
Copy link
Copy Markdown
Author

I replaced the enum with #define statements to make adding new paths easier in 170e7d3.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo GloriousTacoo marked this pull request as ready for review March 26, 2026 17:38
const size_t difference = stop - start;

if (difference != 0) {
const size_t string_terminator_size = 1;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is such an odd choice, why bother defining the length of the terminator, it makes the code less legible.

Copy link
Copy Markdown
Author

@GloriousTacoo GloriousTacoo Mar 29, 2026

Choose a reason for hiding this comment

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

Its to avoid magic numbers. Yeah you can have

// Add vulkan path size with directory symbol and terminator at the end of string.
const size_t string_size = difference + vulkan_relative_path_size + 2;

But this makes the logic dependent on a comment which is bad if the comment becomes outdated in future changes.

@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo GloriousTacoo marked this pull request as draft March 28, 2026 14:10
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

Adds loader_search_path`, `loader_search_path_list`, and macros to
define the origin of a path as a string.

Signed-off-by: Ronald Caesar <github43132@proton.me>
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo GloriousTacoo marked this pull request as ready for review March 29, 2026 23:53
Introduces append_raw_data_file_path_to_search_path_list() which
parses data file paths and appends them to dynamic list
`loader_search_path_list`. copy_data_file_info() concatenated all
OS discovered paths and environment variables into a single,
monolithic string separated by a PATH_SEPARATOR. This destroyed the
origin of each path. This new function aims to fix that.

Signed-off-by: Ronald Caesar <github43132@proton.me>
Rewrites the logic inside read_data_files_in_search_paths to support
logging manifest files from various locations. The logs below shows a
before and after:

# Before
[Vulkan Loader] LAYER:          Searching for implicit layer manifest files
[Vulkan Loader] LAYER:             In following locations:
[Vulkan Loader] LAYER:                /home/fake_home/.config/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /etc/xdg/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /home/fake_home/.local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                /usr/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:             Found the following files:
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json (file version 1.0.0)
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_1.json (file version 1.0.0)

# After
[Vulkan Loader] LAYER:          Searching for implicit layer manifest files
[Vulkan Loader] LAYER:              In FALLBACK_CONFIG_DIRS:
[Vulkan Loader] LAYER:                  /home/fake_home/.config/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                  /etc/xdg/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:              In SYSCONFDIR:
[Vulkan Loader] LAYER:                  /usr/local/etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:              In EXTRASYSCONFDIR:
[Vulkan Loader] LAYER:                  /etc/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:              In FALLBACK_DATA_DIRS:
[Vulkan Loader] LAYER:                  /home/fake_home/.local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                  /usr/local/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:                  /usr/share/vulkan/implicit_layer.d
[Vulkan Loader] LAYER:             Found the following files:
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json
[Vulkan Loader] LAYER:                /usr/local/etc/vulkan/implicit_layer.d/test_layer_2.json
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_0.json (file version 1.0.0)
[Vulkan Loader] INFO:           Found manifest file /usr/local/etc/vulkan/implicit_layer.d/test_layer_2.json (file version 1.0.0)

Signed-off-by: Ronald Caesar <github43132@proton.me>
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

1 similar comment
@ci-tester-lunarg
Copy link
Copy Markdown

Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build.

@GloriousTacoo
Copy link
Copy Markdown
Author

Ill have to create a windows virtual machine to test my changes

@GloriousTacoo GloriousTacoo marked this pull request as draft March 30, 2026 17:15
@GloriousTacoo
Copy link
Copy Markdown
Author

I've tried compiling the main branch in visual studio on a barebones windows 10 virtual machine but 6 tests fail and returns -1 on this random function in folder_manager.cpp

// copy file into this folder
std::filesystem::path Folder::copy_file(std::filesystem::path const& file, std::filesystem::path const& new_name) {
    check_if_first_use();
    insert_file_to_tracking(new_name);

    auto new_filepath = folder / new_name;
    if (!::testing::internal::InDeathTestChild()) {
        std::error_code err;
        if (!std::filesystem::copy_file(file, new_filepath, err)) { // <-- this line cause the test to immediately return -1
            std::cerr << "Failed to copy file " << file << " to " << new_filepath << " because " << err.message() << "\n";
        }
    }
    return new_filepath;
}

This is the main branch so I dont know how these tests are failing when its passing in the CI/CD. I've spent hours trying to debug this and my conclusion is that this is a bug in MSVC. This PR works on linux but I have lost all motivation to get this working on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants