-
Notifications
You must be signed in to change notification settings - Fork 283
Dependency fetching and linking rework #1096
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
base: main
Are you sure you want to change the base?
Conversation
also fixed dependecies linking for test tools
|
@Daniil-SV I am delighted to hear you are working on KTX support in OpenImageIO. Thank you for your efforts. I am new to FetchContent so it may take me a while to fully understand the proposed changes. Please bear with me. @kring (primarily) and @dg0yt are also working on and have interest in improving the dependency handling, in their case for vcpkg. Via this comment, as I can find no way to officially add them as reviewers, I invite them to review this PR. I need to ensure it contains nothing that conflicts with what is needed for vcpkg or will make that work harder. In a quick skim over the changes I noticed that it no longer adds dependent libraries' objects into a static libktx. Why did you make this change? Making a single library is intended to simplify linking for those not using CMake though I have no idea how large is the group of non-CMake users. The golden files for the failing zstd tests should be ideally be updated using GCC. Use of ktxdiff on these tests is because we have previously encountered non-deterministic behaviour across compilers or platforms. Using a particular compiler helps limit the changes in the golden files to those warranted by changes in the code generating them. Addressing your notes in the same order.
|
Right, I forgot about that, thanks for pointing it out. I originally removed it because I was getting a lot of warnings due to this optimization, but I think I can now revert it back and implement it without errors. |
updated basisu source tag improved zstd module behavior
I'm currently working on KTX support in OpenImageIO (OIIO further), and I've encountered some issues along the way.
Builds on some of my MacOS-based CIs were failing due to errors related to missing zstd headers. For some reason, the compiler simply couldn't find them.
I don't think the problem lies with OIIO, as dependencies are compiled in a separate process during cmake configuration stage.
After a little investigation, I concluded that dependencies are working a bit messy here. I suspect that in my case, the compiler was failing because some zstd headers were used directly from basisu while others contained the repository itself.
So I decided to try to "fix"/"rework" it.
Implementation
I decided to use FetchContent, because it is a modern approach to dealing with dependencies, and it gives quite a lot of opportunities for developers. Under the hood, it uses a git clone and all dependencies are collected in one centralized folder, so there shouldn't be such problems with dependency files. In my opinion, this also solves the dependency maintainability issue, you just need to change the git tag inside cmake file to update dependency version. Also, FetchContent calls find_package under the hood, which allows you to use system packages to speed up the build or use custom packages, which is I think will be very useful, at least for OIIO.
Testing
I tested this in OIIO CI and KTX-Software CI. The build works fine in both cases, but some ktxdiff tests fail. I checked each failing test and concluded that the updated zstd version (more details in the notes) changed the compressed data, causing the test to fail when comparing them, even though the actual pixel data is essentially the same. I updated the test files locally and all tests pass successfully on my Windows machine, so if necessary, I can send additional PR to the CTS repository.
Notes