Log the origin of manifest search paths#1878
Log the origin of manifest search paths#1878GloriousTacoo wants to merge 3 commits intoKhronosGroup:mainfrom
Conversation
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
de03c0c to
a0a45d1
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
5 similar comments
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
7a93e51 to
bd81bd4
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
I'm trying to add a new test file to the [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:
#include "test_environment.h"
#include "loader/loader.h"
TEST(Test, Test) {}
|
bd81bd4 to
19fbea6
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Adding my tests to |
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
charles-lunarg
left a comment
There was a problem hiding this comment.
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.)
|
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 |
|
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)
|
|
To replicate the failed tests you need to link vulkan directly to 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. |
a3ba566 to
73254e7
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Not gonna lie testing my changes without exposing internal functions is difficult. I'll probably drop the test commits once the PR is finished. |
charles-lunarg
left a comment
There was a problem hiding this comment.
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
| search_paths->list[search_paths->count] = *path; | ||
| ++search_paths->count; |
There was a problem hiding this comment.
This would be a good place to do de-duplication.
loader/loader_common.h
Outdated
| LOADER_SEARCH_PATH_ORIGIN_XDG_CONFIG, | ||
| LOADER_SEARCH_PATH_ORIGIN_XDG_DATA, | ||
| LOADER_SEARCH_PATH_ORIGIN_SYSCONFIG, | ||
| LOADER_SEARCH_PATH_ORIGIN_HOME, |
There was a problem hiding this comment.
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.
73254e7 to
0676c31
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
I replaced the enum with |
edf3325 to
2a2fc9d
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
2a2fc9d to
ef57c8d
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
| const size_t difference = stop - start; | ||
|
|
||
| if (difference != 0) { | ||
| const size_t string_terminator_size = 1; |
There was a problem hiding this comment.
This is such an odd choice, why bother defining the length of the terminator, it makes the code less legible.
There was a problem hiding this comment.
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.
ef57c8d to
62b9348
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
62b9348 to
94e4817
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
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>
94e4817 to
fbf5daf
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
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>
fbf5daf to
a1f6a84
Compare
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
Author GloriousTacoo not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Ill have to create a windows virtual machine to test my changes |
|
I've tried compiling the main branch in visual studio on a barebones windows 10 virtual machine but 6 tests fail and returns // 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. |
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().