2.6-beta band-aid: Disable QML to fix link failure avoid undefined behaviour

[daschuer]

Because of multiple concurrent definition of symbols caused by the render graph compile definition, we can't provide 2.6-beta build for all Ubuntu versions on our PPA.
This is a emergency band aid, to provide beta testers with builds until the root cause is fixed.

See:
#14766
#14771

Hopefully we fix the issue fast that we can revert this PR

[ronso0]

Looks good. Other options, or maybe a fix already in the pipeline? @mixxxdj/developers

[JoergAtGithub]

Let's overwrite this default in build.yml. Otherwise CI would no longer detect, if someone commits a change that breaks QML.

[JoergAtGithub]

I fully agree with creating the PPA builds without QML, but the GitHub CI builds of the 2.6 branch, and the PRs targeted to, it must have QML always enabled. This is important so that no one unknowingly adds a PR to 2.6, that breaks the main build after merging.

To my understanding this behavior can easily be achieved by setting the default in CMakeLists.txt to OFF and overwrite it with ON in build.yml and build-checks.yml.

[daschuer]

I don't want to have this type of undefined behavior in our beta builds provided on our download page. This is IMHO more important than the unlikely case that something QML related is pushed to the 2.6 branch.

I expect that one of use will quickly find the root cause so that this situation is only temporary anyway.

..

[acolombier]

Overall, I would be pushing for fixing the problem correctly, rather than yet again silencing QML.

As I said here, the problem doesn't too hard to fix, so for now I think it's fair to build the jammy and oracular PPA with -QML=OFF explicitly set, but please don't change the default.
It seems the problem is compiler-related, so relying on a more recent version or clang would also very likely fix the problem.

Also, remember that QML is now used for controller screen rendering, which is shipped with 2.6.

[acolombier]

After investigation, it looks like the root cause of the multiple definition comes from two Qt shader meta targets (GL and SG) that seems to not support multiple definition. This doesn't impact Qt >=6.7

I suspect this fix would work:

diff --git a/src/rendergraph/shaders/CMakeLists.txt b/src/rendergraph/shaders/CMakeLists.txt
index 277bd3e0aa..7cc84e32b6 100644
--- a/src/rendergraph/shaders/CMakeLists.txt
+++ b/src/rendergraph/shaders/CMakeLists.txt
@@ -34,7 +34,7 @@ if(TARGET rendergraph_gl)
     # Add the .qsb shader bundles; rendergraph::MaterialShader will use
     # QShader to extract the GLSL shader from the bundle.
     message(STATUS "Adding qsb shaders to rendergraph_gl")
-    qt6_add_shaders(rendergraph_gl "shaders-qsb"
+    qt6_add_shaders(rendergraph_gl "shaders-qsb-gl"
           BATCHABLE
           PRECOMPILE
           # FIXME qsl relies on 'spirv-opt' to be available in the path, so only installing it via VCPKG is not sufficient

You could also disable qt6_add_shaders for Qt 6.6:

diff --git a/src/rendergraph/CMakeLists.txt b/src/rendergraph/CMakeLists.txt
index b2f028a2f8..15f1dbddac 100644
--- a/src/rendergraph/CMakeLists.txt
+++ b/src/rendergraph/CMakeLists.txt
@@ -1,7 +1,7 @@
 find_package(QT NAMES Qt6 REQUIRED COMPONENTS Core)
 message(STATUS "Qt version ${QT_VERSION_MAJOR}.${QT_VERSION_MINOR}")
 
-if(QT_VERSION_MINOR GREATER_EQUAL 6)
+if(QT_VERSION_MINOR GREATER_EQUAL 7)
   set(USE_QSHADER_FOR_GL ON)
 endif()

[daschuer]

We have currently no 2.6-beta builds for beta tester.
This blocks spreading the 2.6-beta in social media.
Can we merge this asap and postpone every fix to a follow up PR? This way we have at least one good binary for all targets. And we can fix the issue without pressure.

Regarding the issue. I have digged through it and it is clearly NOT a compiler issue. We have various objects in the archives that are equal named but different.
I can confirm this using objdump, but I cannot reproduce the build error even though I am on Ubuntu Jammy that fails on the Launchpad.

This means that everyone that builds the 2.6-beta may occasionally suffer the build failure in the best case or a crash if the linker picks the wrong object in the wrong situation.

[daschuer]

@acolombier do you have a reference that Qt has something fixed in Qt 6.7?
Unfortunately the other issue will still remain, the

note: type name ‘rendergraph_gl::Context’ should match type name ‘rendergraph_sg::Context’

Here we need probably a proper interface and hide the different types in the implementation in the namespace.

[daschuer]

@JoergAtGithub

Let's overwrite this default in build.yml.

How about reverting this change in main right away?
This way we maintain a stable 2.6-beta and have still the early warning in main. All qml related changes shall go to main anyway.

[acolombier]

How about reverting this change in main right away?

I'd be okay with this trade-off. Arguably, nobody will use QML in 2.6. Even S4 Mk3 user would need a custom build, so as long as we keep 2.7 unchanged regarding QML policy I'm good with it

[daschuer]

Greate, merge? I will take care to not merge this into main.

[daschuer]

@JoergAtGithub merge?

[JoergAtGithub]

Please address the change request above, and we can merge this!
In its current state, merging this PR would be a regression, as it unnecessarily disables the working build of the entire QML code in CI. This leaves all the work of fixing the QML-breaking C++ code changes to the maintainers who will merge 2.6 into the main branch.

[daschuer]

You mean to enable readable it on the CI. Right?

I consider this duplicated symbol issue a critical issue that may leads to a crash on all targets. Therfore I like to have it disabled for all beta builds. To have them stable as possible.
There is not much benefit of QML in 2.6-beta anyway since development is advancing at main. Also there is not much risk of breaking QML because of the same reason.

Let's not merge (revert) it in main after merging this PR and than fix the root cause here.

Who wants to take care by the way @m0dB seems to be caught in real live.

[JoergAtGithub]

Just noticed, that QML is enabled for CI with this PR as it is - so my request is alread fullfilled. Let's merge this therefore!