-
Notifications
You must be signed in to change notification settings - Fork 315
CMake and warning fixes to allow use with KTX-Software #408
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
CMake and warning fixes to allow use with KTX-Software #408
Conversation
* Prefix option names with `BASISU_`. * Add include files to source list for library project. * Fix build of basisu with multi-config generators.
The change at line 13 to use a genex accomplishes the same thing.
using INTERFACE to export OpenCL lib and includes from basisu_encoder to the applications.
Fix OpenCL configuration issues.
basis_universal CMakeLists sets compiler options that disable these warnings when they are included there. Two warnings: * The pragma to ignore -Wmem-access is ifdef'ed for __GNUC__ but clang also defines __GNUC__ and does not have a mem-access warnings. Add !defined(__clang__)to fix. * Comment out an unused typedef in basisu_enc.h.
* move deployment target setting to where it will work; * move ALLOW_MEMORY_GROWTH to link options; * set c++ only options only for c++; * move include directories to BUILD_INTERFACE.
* swapped parameters to calloc in tinyexr.h; * unneeded unnsigned < 0 comparisons.
Rendered unnecessary by addition of !defined(clang) to subsequent #if to support wider clang family.
Note that setting BASISU_SSE TRUE for MSVC needs fixing. It breaks the build on Windows Arm64.
|
Thanks! Once our new build settles I'll merge this in. |
PR 408's cmakelists.txt file has too many issues (OpenCL breaks on Windows/OSX) and changing our options to have the "BASISU_" prefix will break upstream builds.
|
I've merged all code warning fixes from this PR. I added the header files to our CMakeLists.txt file. Unfortunately, OpenCL building under Windows is broken in this PR, and OpenCL is not working correctly on OSX, so I can't merge the CMakeLists changes. |
|
It seems this is missing installation rules: If you could rebase your branch on master, I'll rebase my own branch on top of this and pull out the installation rules I made. Alternatively, just pick and choose whatever you want from Pandapip1@e2f294d. Thank you so much for this work! |
It is building fine under windows with OpenCL in the KTX-Software CI. Does the Windows build in your CI use OpenCL? It too is green. What errors are you seeing? I want to fix the problem. How are you determining that OpenCL is not working correctly on macOS. Please send me a link to the test(s) that show the problem.
There is no way to avoid that if you want to avoid polluting the global variable namespace. Please merge the change in your January 2026 release. |
There were no installation rules last time I pulled from upstream. I will merge the latest changes from master as soon as I have time. |
With these changes KTX-Software now incorporates basis_universal as a CMake subproject, building it using this fork which is the source of the changes in this PR.
Via KTX-Software's CI, basis_universal with these fixes is known to build on Android (gcc), Emscripten (Clang 20), iOS (AppleClang 17), Linux(Ubuntu 22.04/gcc13 and 24.04/gcc15, x86_64 and arm64), minGW (gcc 15), macOS (AppleClang 17, x86_64 & arm64) and Windows (MSVC CL and CLangCL via VisualStudio 2022, x86_64 and arm64). Via manual check, it is also known to build with gcc15 for macOS. KTX-Software's test suite, including various encode and transcode tests, passes on all the platforms it is run on, that is all the above except Android and iOS.
KTX-Software's CI tests all combinations of the SSE and OPENCL options on the 3 major platforms.
You can check the results in the PR for adding the updated encoder to KTX-Software - KhronosGroup/KTX-Software#1078.
There are 3 distinct sets of changes:
reuse lintCMake Project Updates
Make it usable as a subproject:
Enhancements
Bug Fixes
STATICoption, renamedBASISU_STATIC, is now set to TRUE by default because the library is always builtas a static library.
STATICis a keyword for theadd_librarycommand so theSTATICoption was ignored there. (The BASISU_ prefix now makes it clear they are different.) Hence the previous default option of FALSE was incorrect.STATICis TRUE are only needed for MinGW and are now only added for MinGW.STATICis FALSE is pointless with a static library but was happening due to the incorrect value ofSTATIC.buildandbin, but to not ignore the handful of files in there that are tracked by git.Bugs found but not fixed
if (MSVC)so it is incorrectly set on Windows ARM and not set in many other cases where it probably should be.BASISU_STATICis pointless.Warning fixes
Many compile warnings in the code have been fixed. Two I particularly want to note are:
basis_universal/encoder/3rdparty/tinyexr.h
Lines 4930 to 4931 in b111011
basis_universal/transcoder/basisu.h
Lines 110 to 115 in b111011
#if defined(__GNUC__) && !defined(__clang__).reuse lintFixes