Skip to content
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

Virtual File System #462

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Conversation

TomAtkinsonArm
Copy link
Contributor

@TomAtkinsonArm TomAtkinsonArm commented May 27, 2022

Description

A small improvement on the previous virtual file system PR.

Added the RE2 library for better regex, the API is easier to use and it removes some of the compiler specific issues in C++ regex

StackError

  • Hopefully a more descriptive error mechanism
  • tracks filenames and line usage in a more controllable way than the current Vulkan Samples
  • Eventually we should be able to produce detailed error stacks

Strings

  • the start of replacing the strings header

VFS

  • Errors moved from vfs::status to StackError
  • Added a test - hope to add more in the future

Testing

  • Adds catch2
  • Adds basic unit tests
  • Adds vkb__register_test
  • Compile target vkb_tests bundles all tests into the same compile target
  • Adds CI unit test workflow

@TomAtkinsonArm TomAtkinsonArm added the framework This is relevant to the framework label May 27, 2022
@TomAtkinsonArm TomAtkinsonArm self-assigned this May 27, 2022
@TomAtkinsonArm TomAtkinsonArm changed the base branch from master to framework/v2.0 May 27, 2022 13:09
@TomAtkinsonArm TomAtkinsonArm added this to In progress in Framework Improvements May 30, 2022
@TomAtkinsonArm TomAtkinsonArm removed this from In progress in Framework Improvements May 30, 2022
Copy link
Contributor

@gpx1000 gpx1000 left a comment

Choose a reason for hiding this comment

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

FWIW, we might be able to use the C++17 stl Filesystem classes and save a bit of platform dependent effort here. However, in general I really like this approach you have working thus far.
Lemme know if you need any help.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@TomAtkinsonArm
Copy link
Contributor Author

FWIW, we might be able to use the C++17 stl Filesystem classes and save a bit of platform dependent effort here. However, in general I really like this approach you have working thus far.
Lemme know if you need any help.

If you have chance please add or groom tickets on the Vulkan Samples Framework Improvements board. Currently its mainly broad statements of what we may want but it would be great to add more and prioritise which areas need to be worked on - which are bad ideas and which are good....

For now i've just been working on cleaning up some of the peripheral stuff. If you would like to make an issue discussing how the C++17 filesystem can simplify this PR then id be all for it @gpx1000!

@gpx1000 gpx1000 mentioned this pull request May 31, 2022
@TomAtkinsonArm
Copy link
Contributor Author

Force pushed the test framework to framework/v2.0 so that I can use it on other branches. It works as expected as it has been used in multiple PRs now. This should be one of the last force pushes to the branch.

@TomAtkinsonArm
Copy link
Contributor Author

test framework added to framework/v2.0. rebases and other fixes applied to this PR

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Jun 4, 2022

Wanted to give this a spin, bud oddly my project structure looks completely different using this PR, and no samples are generated that I could run:

image

What I did:

  • Checkout this PR
  • Delete old build/windows folder
  • Re-run cmake
  • Open project in VS2019

Looking at the CI it does seem to have the same problem.

Am I missing something?

@TomAtkinsonArm
Copy link
Contributor Author

Wanted to give this a spin, bud oddly my project structure looks completely different using this PR, and no samples are generated that I could run

This is the correct behaviour. Making the framework changes propagate between every sample would be too much work and counter productive for reviewers (larger diffs) lots of manual testing.

The framework/v2.0 branch has no samples or framework folder.

The aim is to write tests and code for the parts of the framework which we deem need the most attention. The CI should then validate that the contribution works on a given system with the desired behaviour. Code should build and the tests should pass. The workflows for this are Build Project and Unit Test Project.

The end goal for a sample is shown here #458. The new framework should be more of a sandbox of components that samples can be made from.

In terms of Vulkan, most will stay the same with some exterior API changes to simplify things. This PR #471 shows how I write unit tests against Vulkan usage. Extremely experimental but a promising example.

Although simple, PRs #469 and #470 show this process working fairly well.

If you would like to build and run the unit tests locally you can use:

cmake --build "build/windows" --target vkb_tests --config Release -j 2

and

ctest --output-on-failure --test-dir "build/windows" -C Release

You can also run the test executables directly to see the catch2 output

@SaschaWillems
Copy link
Collaborator

Thanks for the clarification. I think I get it now ;)

This builds fine for me with VS2019 on Windows, and all tests pass. So this looks good to me.

use catch v2.13.6

catch v3.0.1

StackErrors and Strings and tests

CMake changes

Unit Test GitHub Workflow

vfs::helpers::join test

fix windows file paths + add RE2 for regex

CMake set -> option for global variables

VKB_WSI_SELECTION to set
@TomAtkinsonArm TomAtkinsonArm changed the title Framework V2.0: Virtual File System Virtual File System Jun 21, 2022
@TomAtkinsonArm
Copy link
Contributor Author

Merging with 1 approval

@TomAtkinsonArm TomAtkinsonArm merged commit 745736e into KhronosGroup:framework/v2.0 Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework This is relevant to the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants