Fix QML build regressions on macOS #13334
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of include <GL/gl.h>, which is unavailable on macOS and not used elsewhere in the codebase either, we include the Qt OpenGL headers.
fwcd
added a commit
to fwcd/m1xxx
that referenced
this pull request
Jun 6, 2024
This should hopefully fix the other build issues
Swiftb0y
reviewed
Jun 6, 2024
src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp
Outdated
Show resolved
Hide resolved
As per PR feedback
acolombier
approved these changes
Jun 6, 2024
|
Hm, it looks like |
fwcd
commented
Jun 6, 2024
| @@ -660,7 +654,7 @@ void ControllerScriptEngineLegacy::handleScreenFrame( | |||
| emit previewRenderedScreen(screenInfo, screenDebug); | |||
| } | |||
|
|
|||
| QByteArray input(std::bit_cast<const char*>(frame.constBits()), frame.sizeInBytes()); | |||
| QByteArray input(reinterpret_cast<const char*>(frame.constBits()), frame.sizeInBytes()); | |||
There was a problem hiding this comment.
From what I understand, casting between const char * and const unsigned char * using reinterpret_cast seems to be fine, stackoverflow confirms that. This would fix the build on some older compilers where std::bit_cast isn't available, such as the macOS 11 runners we are building on in CI.
Would appreciate a second pair of eyes checking that this is fine though.
acolombier
reviewed
Jun 6, 2024
| @@ -660,7 +654,7 @@ void ControllerScriptEngineLegacy::handleScreenFrame( | |||
| emit previewRenderedScreen(screenInfo, screenDebug); | |||
| } | |||
|
|
|||
| QByteArray input(std::bit_cast<const char*>(frame.constBits()), frame.sizeInBytes()); | |||
| QByteArray input(reinterpret_cast<const char*>(frame.constBits()), frame.sizeInBytes()); | |||
There was a problem hiding this comment.
AFAIR, this is what I was using, but I was requested to change it to bit_cast. Perhaps you could keep this cast specific to Mac?
There was a problem hiding this comment.
My gut feeling would be that it would be better not to have different code paths here to avoid introducing subtle semantic differences here. From what I understand the semantics shouldn't differ, but I think it would be better to hit a bug on all platforms rather than just one (that isn't as well-tested as e.g. Linux).
Perhaps a comment that this should be refactored to a std::bit_cast once we drop support for macOS 11 (or build on a newer runner)?
There was a problem hiding this comment.
Perhaps a comment that this should be refactored to a std::bit_cast once we drop support for macOS 11 (or build on a newer runner)?
is this specific to the build environment or to the runner? Cuz the runner was recently updated #13296.
Here's a little more context for reinterpret_cast vs std::bit_cast: https://stackoverflow.com/a/53402217
There was a problem hiding this comment.
Should be specific to the compiler, i.e. the Xcode version that the runner uses. macOS 12 is two years old too, so my best guess would be that it doesn't support bit_cast either yet.
There was a problem hiding this comment.
https://developer.apple.com/xcode/cpp/ says bit-casting requires XCode 14.3. Seems like that's only available on MacOS 13...
There was a problem hiding this comment.
Ah okay, thanks for checking. I think as long as we're building against an SDK older than that (in CI), we are going to get an error, regardless of whether we use a newer runner. I'll update the comment.
There was a problem hiding this comment.
That's a neat site btw, have to bookmark that.
There was a problem hiding this comment.
Even better: https://en.cppreference.com/w/cpp/compiler_support
Apparently they also track apple clang now, which they previously haven't which is why I defaulted to the apple site.
Swiftb0y
approved these changes
Jun 6, 2024
|
Any ideas on the test failure? That it only fails on x64 makes it pretty strange |
|
Yep, that's the part that is failing: This is likely because the QML dependencies aren't included in the VCPKG, or aren't installed properly in the build environment, basically meaning that QML doesn't work on Mixxx out of the box. This was tracked in #12267 and should have been fixed in #12604 but not sure this was tested on Mac x64 |
|
Hm okay, so maybe disabling QML in x64 CI would be a temporary fix, not ideal, but at least we'd get QML CI on macOS. |
|
Do you mind creating an issue for macOS x64 /w QML just so we keep track of this? |
|
-> #13336 |
Swiftb0y
approved these changes
Jun 7, 2024
|
Thank you for looking into the issue this quickly. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a few minor regressions introduced by #11407 when building with
-DQML=ONon macOS and re-enables QML in the CI build for macOS.