Refactor build system

[COM8]

Why this PR?

I have a couple of problems with the current way Kompute handles dependencies:

1. git-Submodules are a rather outdated concept in my eyes and should be replaced since they are always a pain to use and are not flexible in any way.
2. When including Kompute in a project that also uses Spdlog, it beaks a lot of stuff, e.g. Log-Macros stay in code, but we do not link Kompute against Spdlog.
3. On my System (Fedora) Vulkan-Headers >= 1.3.0 are available, but my driver (mesa/intel) supports only >= 1.2.131. Well I need to link against a different version Vulkan-Headers without downgrading my System Vulkan-Headers since other applications depend on those. If I don't change the Vulkan-Header version, I always run into the following assertion: VULKAN_HPP_ASSERT( d.getVkHeaderVersion() == VK_HEADER_VERSION );

Proposed Solutions

• Replace git-Submodules with CMake fetch_content
  • This solves 1. and 3.
  • I also replaced KOMPUTE_OPT_REPO_SUBMODULE_BUILD with KOMPUTE_OPT_USE_BUILD_IN_{SPDLOG, VULKAN_HEADER, ...} and added a deprecation warning for the old way.
  • I added KOMPUTE_OPT_BUILD_IN_VULKAN_HEADER_TAG which allows consumers to set their specific Vulkan-Header version.
  • I added a check_vulkan_version to CMake that checks if your hardware supports for example Vulkan 1.3 if you link against Vulkan-Headers 1.3 and prints a warning if only the patch version of your hardware is < that the Vulkan-Header version. There is also an option to disable this: KOMPUTE_OPT_DISABLE_VULKAN_VERSION_CHECK
• I also cleaned up all CMake options related to dependencies and fixed a few bugs here and there to solve 2.

What is left to do?

• Check if it runs on Windows - working on it
• Check if this works with other GPUs (Nvidia)
• Fix Python Bindings
• Check if it works on Android
• Add all removed CMake options into the deprecated list here: https://github.com/COM8/kompute/blob/master/cmake/deprecation_warnings.cmake
• Update docs

Things that need to be done once the PR has been merged

• Update the repo and hash for the examples
  • https://github.com/COM8/kompute/blob/f6d30d5b0803c48e7f74c44adaf32e9bc613b98f/examples/array_multiplication/CMakeLists.txt#L28-L29
  • https://github.com/COM8/kompute/blob/f6d30d5b0803c48e7f74c44adaf32e9bc613b98f/examples/logistic_regression/CMakeLists.txt#L28-L29

Let me know what you think about this.

[axsaucedo]

Hey @COM8 - thank you for this PR, this all sounds great. Here's a couple of comments on the proposed actions:

• Replace git-Submodules with CMake fetch_content

I haven't used FetchContent, but it does look quite good. It seems we'd need to bump CMAKE from min 3.4.x to 3.11.x, and make sure to mention this somewhere as defaults may be lower in older systems. Overall it does seem like a good step as it would avoid a common error that people don't init the submodules.

• I also replaced KOMPUTE_OPT_REPO_SUBMODULE_BUILD with KOMPUTE_OPT_USE_BUILD_IN_{SPDLOG, VULKAN_HEADER, ...} and added a deprecation warning for the old way.

That sounds good. For naming it would be better to name it explicitly KOMPUTE_OPT_USE_FETCH_REPO_{...}, as otherwise "build in" sounds like "built-in" which implies it's internal as opposed to cloned.

• I added KOMPUTE_OPT_BUILD_IN_VULKAN_HEADER_TAG which allows consumers to set their specific Vulkan-Header version.

This sounds good, I've been meaning to do this for a while. Will be good to make sure it's for both HPP and H headers.

• I added a check_vulkan_version to CMake that checks if your hardware supports for example Vulkan 1.3 if you link against Vulkan-Headers 1.3 and prints a warning if only the patch version of your hardware is < that the Vulkan-Header version. There is also an option to disable this: KOMPUTE_OPT_DISABLE_VULKAN_VERSION_CHECK

This sounds good.

I also cleaned up all CMake options related to dependencies and fixed a few bugs here and there to solve 2.

Sounds good, would be good to explicitly list them in the PR description.

My main suggestion would be to do this in iterative chunks, as opposed to a massive PR, mainyl to avoid potentially a situation where there's too much at once. Only mentioning this as I've started to see some changes in the git jobs, which I actually do like, but mainly to break down into different PRs so it's simpler/smoother to merge.

[COM8]

Agree with all of your points, Then I will keep this PR open as "current" stat and as soon as I'm happy with the result, I will create smaller PRs.

[COM8]

When I tried splitting up my changes into multiple smaller PRs I noticed a lot of other stuff needing to be fixed. So basically I'm planing to do a rewrite of the complete build system over at my fork: https://github.com/COM8/kompute

This will roughly take me one month and since especially the Makefile stuff is not really portable I plan to integrate what ever possible directly into CMake e.g. compiling shaders: https://github.com/COM8/movement-sim/blob/main/cmake/vulkan_shader_compiler.cmake

I will the try to outline every breaking change I did and why it was necessary in a clear manner and try to get in touch with you in case I need to change anything large.

[axsaucedo]

Ok interesting @COM8 - I would still be keen to look towards merging the initial improvements in an iterative way where possible, as I like already some of the ideas on moving away from git modules as that would already make it much better. Certianly if you do need a hand let me know - happy to help here, or you can also join the discord and I am active there for any questions

[COM8]

This PR is now ready for its first review.
Things that need to be done:

• Update Android integration
• Update GoDot integration
• Python CI is timing out since it needs too much resources
• Sign off commits
• Windows CI - Working on it, but I cant get CMake to find the Vulkan installation: initial Windows CI setup COM8/kompute#1 Will be a separate PR
• MacOS CI - Can be done in a different PR Will be a separate PR
• What should we do with the Makefile? It's not required any more for normal use and contains only convenience helpers.

[axsaucedo]

Looks great @COM8 - thank you so much for investing the time to having an initial look, the work so far is great! Apologies I didn't get through this before, I actually completely missed your comment saying ti was ready for review - I have done a first pass of comments, overall the core additions are great in principle, the main things that we'll have to consider are:

• Ensuring spdlog is still an optional dependency
• Validate the need for helper components in the logger files
• Discuss further the separation of various components into different libraries
• Explore simplify the shader compilation (ie compile all shaders in a single folder instead of one by one)
• Making shader compilation optional

Once again awesome effort - I will do a deeper dive later today, if you provide thoughts on the comments I can also provide deeper detial as required.

[axsaucedo]

I'm currently testing locally and I'm getting

Cannot create Vulkan instance.
This problem is often caused by a faulty installation of the Vulkan driver or attempting to use a GPU that does not support Vulkan.
ERROR at /build/vulkan-tools-1.2.148.0-1ubuntu18.04/vulkaninfo/vulkaninfo.h:641:vkCreateInstance failed with ERROR_INCOMPATIBLE_DRIVER
CMake Error at cmake/check_vulkan_version.cmake:61 (string):
  string sub-command REGEX, mode MATCHALL needs at least 5 arguments total to
  command.
Call Stack (most recent call first):
  CMakeLists.txt:116 (check_vulkan_version)

Have you seen this before? Would it be due to the version of my vulkan setup?

[COM8]

I'm currently testing locally and I'm getting

Cannot create Vulkan instance.
This problem is often caused by a faulty installation of the Vulkan driver or attempting to use a GPU that does not support Vulkan.
ERROR at /build/vulkan-tools-1.2.148.0-1ubuntu18.04/vulkaninfo/vulkaninfo.h:641:vkCreateInstance failed with ERROR_INCOMPATIBLE_DRIVER
CMake Error at cmake/check_vulkan_version.cmake:61 (string):
  string sub-command REGEX, mode MATCHALL needs at least 5 arguments total to
  command.
Call Stack (most recent call first):
  CMakeLists.txt:116 (check_vulkan_version)

Have you seen this before? Would it be due to the version of my vulkan setup?

Hmmm, I have never seen such an error before. But I see a potential error in cmake/check_vulkan_version.cmake:61 resulting from this "crash".

[axsaucedo]

Just tried from the latest to build but it seems it gets frozen iwth infinite loop on:

...etc
--   KOMPUTE_OPT_BUILD_SHADERS: 1
--   KOMPUTE_OPT_USE_BUILD_IN_SPDLOG: ON
--   KOMPUTE_OPT_USE_BUILD_IN_FMT: ON
--   KOMPUTE_OPT_USE_BUILD_IN_GOOGLE_TEST: ON
--   KOMPUTE_OPT_USE_BUILD_IN_PYBIND11: ON
--   KOMPUTE_OPT_USE_BUILD_IN_VULKAN_HEADER: ON
--   KOMPUTE_OPT_BUILD_IN_VULKAN_HEADER_TAG: v1.2.203
-- =======================================================
-- Ensuring the currently installed driver supports the Vulkan version requested by the Vulkan Header.
-- Found Vulkan Header version 1.2.203.

Wondering if the trailing . could be causing an issue?

[axsaucedo]

Nice one - I added a couple more comments. Overall the new amendments are great. I have given it more thought on specifically the logger module, and added a few suggestions that may need rethinking on that part specifically.

[axsaucedo]

Nice one with updates, it seems it's getting closer, I was now able to run the build and start compiling some of the tests. I added a couple of extra comments and questions on build errors as well as how tests can be run. There's still the outstanding question on the logger, but as suggested we can discuss during the next community call - i also replied on the community discord with suggestion for other times that could work as an alternate. Let me know if you have any thoughts ahead of then.

[COM8]

@axsaucedo Is there anything else I can do on this PR?

[axsaucedo]

@COM8 thank you for following up - just pinged on Discord, I will be keen to merge soon, planning to do a pass asap in the next / following week

[axsaucedo]

@COM8 I am thinking of merging this quite soon to make sure we can get the ball rolling on the rest of the PRs, however I can't seem to get the Android examples working unfortunately - are you able to run these example successfully on your end?

[COM8]

Nope, never have tried compiling them for android since I have no toolchain set up for this. But I can try to set up one if this helps.

[axsaucedo]

That would be great if you get a chance @COM8 - I will be diving into it during the weekend as well to get this and the rest of the examples working correctly, I want then to move towards merging so any further changes can be done from master as there's already also some other interesting PRs to explore (Such as upgrading to vulkan 1.3)

[COM8]

ACK, will try to work on it during the next week.

[COM8]

Quick update: I got it almost up and running. Just some minor things like fixing the Vulkan function templates still left to do.
Sadly I won't have too much time over the next three weeks while I'm finishing my thesis.
So this one might get delayed a bit. Sorry...

[axsaucedo]

@COM8 that's fantastic news! It would be really helpful if you do manage to get it through as for some reason I'm still having issues in my set up - no worries at all tho, great to hear you're looking to finish/wrap your theses, hopefully I am able to finalise it before then but otherwise we can have a look once you get a chance again

[COM8]

@axsaucedo it is done!
The android example builds again.
But when clicking on the button and when creating a kp::Manager it crashes with a segfault...

I'm looking into this one right now.

Could you please confirm quickly it's also building on your system?

[COM8]

Done. Now everything works. Ready for a final review.

[axsaucedo]

That's fantastic @COM8 ! I am just back from a conference this week so I will be able to review and rerun all the examples.

PS Hope everything went smooth with the thesis

[COM8]

Great to hear. Yes, from my side everything worked perfectly!

[axsaucedo]

@COM8 I have finally been able to test every single example and I have to say that this contribution is beyond fantastic - the additions and improvements to the project are really taking the project to the next level. I am very keen on expanding from this great work, I will highlight this contribution with the linux foundation group, and it gets us a significant step closer to full graduation as a project.

Thank you very much for persevering on this great contribution Fabian, it's a truly appreciated contribution!

[axsaucedo]

TODO replace once this is in master

[axsaucedo]

TODO: as comment suggests we'll replace with tag