Fix issues in CMakeLists.txt (see #996)#998
Conversation
|
|
|
uhh.. this CLA thing is kinda fishy. The text of the conditions was in light grey on lighter grey, and when I logged in there with Github (hoping that I could then select what applies to me and see the text black on white) it automatically signs it. To be clear, I'm fine with the actual CLA conditions, I just find the implementation dubious. |
|
@DanielGibson We are using CLA-Assistant to handle our CLA signings (cla-assistant.io). The text colouring is a bit odd. We will see if we can adjust our Gist to clean that up. Thanks. |
MarkCallow
left a comment
MarkCallow
left a comment
There was a problem hiding this comment.
Our CI for GNU/Linux, iOS, macOS and the web is undergoing remodelling. Once that is complete I will ask you to rebase this so we can run it through all the compilation checks. I will not merge it until it successfully passes CI. Sorry for the delay.
DanielGibson
force-pushed
the
cmake-fixes
branch
from
936b2e1 to
1959b01
Compare
|
I did the requested changes and rebased to the current |
|
Why does this now pretend that I didn't sign the CLA yet? |
I don't know. Although this is the first time I am aware of a reset happening during an open PR my own status has been reset between PRs several times. CLAssistant seems to be a black hole. @KhronosWebservices any thoughts on how to prevent this? |
MarkCallow
left a comment
MarkCallow
left a comment
There was a problem hiding this comment.
Looks good now. Thanks. Please fix the typos I've pointed out. Then we'll have to wait until the CI remodel is finished at which point another rebase will be needed.
DanielGibson
force-pushed
the
cmake-fixes
branch
from
1959b01 to
1e50aaf
Compare
|
Please investigate the GCC compile failure in the MingW build. Is this due to the change here or a change in the compiler used in the runner? I do not think it is the latter because the runner is using a version of GCC 12 which is now quite old. |
I don't see how my changes should cause this and I don't even think it's a valid warning. If I understand correctly, the compiler says that can access endpoints[18] ("at offset 18 into destination object 'endpoints' of size 18") which would be out of bounds because it only has 18 elements, see
But I have no idea where that assumption comes from, because c < total_comps, which comes from some global array:
I don't know why the compiler assumes that c can be 9 (then endpoints[c * 2 + 0] would be endpoints[18]). It probably can't "see" the values of that array (it's defined in external/basisu/transcoder/basisu_transcoder.cpp and its biggest value is 4 so c will be <= 3), but if it assumes that it can have invalid values, it could as well assume that c is 254 and that it would access endpoints[508] and endpoints[509] which is even more out of bounds...
Did the compiler version on the runner change? Even if GCC12 is "quite old", if it was an even older version before it could be that the warning only turned up in GCC12 |
Thank you for the analysis. I agree the warning seems strange. However the compiler version did not change and builds on other branches are compiling without warning using this compiler. See https://github.com/KhronosGroup/KTX-Software/actions/runs/14324522719/job/40147530265?pr=1010. Therefore something here is triggering the compiler into issuing that warning. Lines 873 to 901 in 70ed9b9 However your change is taking the COMPILE_OPTIONS from I just noticed a similar bug in the code above w.r.t |
|
Oh, right. These hyperspecific disabled warnings per source file and per compiler version are confusing and painful to handle :-/ |
|
There are other similar bugs in that cmake code, by the way. https://github.com/KhronosGroup/KTX-Software/blob/v4.4.0/CMakeLists.txt#L885-L892 https://github.com/KhronosGroup/KTX-Software/blob/v4.4.0/CMakeLists.txt#L878-L881 |
|
should be better now :) |
DanielGibson
force-pushed
the
cmake-fixes
branch
3 times, most recently
from
0f401c3 to
0540531
Compare
MarkCallow
left a comment
MarkCallow
left a comment
There was a problem hiding this comment.
Thank you for this nice refactoring.
I will wait until Emscripten, iOS, Linux and macOS CI is functional again before merging. I'll let you know when it is functional then you can rebase this and let the updated CI run it.
|
Please rebase against latest main to run CI on all platforms. |
it's never set, probably a leftover from an older basisu version that also contained C code?
Several basisu headers demand doing this ("Important: If compiling with
gcc, be sure strict aliasing is disabled: -fno-strict-aliasing") and
their own CMakeLists.txt (that's not used here) also sets it and
explicitly mentions (in a comment in its first line) that strict
aliasing optimizations must be disabled for both the encoder and decoder
It's an option provided by external/astc-encoder/CMakeLists.txt but setting it to ON in KTX-Software breaks the build. So set it to OFF and make it an INTERNAL variable so it's not shown in cmake-gui where it could lure users to enable it. While at it, also fixed the line number in a comment, and a typo in another comment.
There were bugs where get_source_file_property() was called on a different file than set_source_file_properties() were called on afterwards, which lead to eat least one new compiler warning. A helper function makes this more readable and less error-prone, and allows appending an option to multiple files without overwriting their existing options.
DanielGibson
force-pushed
the
cmake-fixes
branch
from
0540531 to
62f12cc
Compare
|
done (you may have to manually approve running github actions for them to run) |
* `ASTCENC_DECOMPRESSOR` is forced OFF and not shown as an option in
cmake-gui anymore.
* Set `-fno-strict-aliasing` for basisu
* remove the unused `${BASISU_ENCODER_C_SRC}` variable from the basisu
source list.
Fixes KhronosGroup#996.
4 participants
I fixed some of the issues mentioned in #996:
-DASTCENC_DECOMPRESSOR=ONdoesn't break the build anymoreASTCENC_DECOMPRESSORis forced OFF and not shown as an option in cmake-gui anymore.-fno-strict-aliasingfor basisu${BASISU_ENCODER_C_SRC}variable from the basisu source list (this one wasn't mentioned in Issues in the CMakeLists.txt #996, I just noticed this while fixing the other stuff)