Search for Vulkan before Qt has a chance to #45
Labels
No Label
CI
Low Priority
Needs Approvals
Needs Rebase
android
bug
duplicate
enhancement
help wanted
invalid
question
translations
wontfix
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: suyu/suyu#45
Loading…
Reference in New Issue
No description provided.
Delete Branch "lvella/suyu:qt6-search-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
If I try to build on Ubuntu 23.10 with Qt6 enabled and system Vulkan Headers, Qt6 frontruns Suyu's own attempt of setting Vulkan::Headers, failing the configuration.
This fix solves this problem by running external configurations before Qt.
@ -374,2 +374,4 @@
endif()
# QT6 searches for Vulkan::Headers, too, so we have to define it before QT6 has
# a chance to do it.
No need for two lines here, otherwise looks good
Fixed.
466edad7ba
tofb7025faff
Merged (
a9312c837e
)Searching for Vulkan before QT6, so it doesn't messes with Vulkan.to Search for Vulkan before QT incase they're already installedThis commit is the reason of #55. This is not the proper way to handle your intended fix @lvella because you literally deleted the detection of the externals folder breaking CI jobs and any other compilation making use of the options
SUYU_USE_EXTERNAL_*
,SUYU_USE_BUNDLED_FFMPEG
and probably other options. I am going to revert this now.@Exverge PLEASE verify the pull requests made by users properly. I wasted HOURS trying to find what was the cause of the problem and I completely ignored this pull request because I trusted you.
@lvella Next time open a issue and do some research about CMake options please. Do not ask ChatGPT and make more research.
Here is something you really need to read https://cmake.org/cmake/help/latest/command/add_subdirectory.html
EDIT PS: That is why Yuzu devs preferred QT5, because QT6 was not ready to go.
I had only tested this on my machine that I had disabled bundled externals which (unsurprisingly) compiled successfully, and I should’ve done more testing. I am sorry and thank you for being so willing to help this project with issues like this.
@ -374,2 +374,4 @@
endif()
# QT6 searches for Vulkan::Headers, too, so we have to define it before QT6 has a chance to do it.
add_subdirectory(externals)
No, @Fijxu I didn't. I simply I moved it to here, to before QT detection. Granted, I didn't test building it with bundled externals, but I assumed the CI would catch these kind of problems.
If you took the liberty to suggest me to rely less on ChatGPT and read the CMake documentation, may I be so bold to suggest you improve your git diff reading skills?
Also, you might want to look into
git bisect
, if it took you that long to figure out the problematic commit.You are right, I completly forgot about
git bisect
. I also apologize for my skill issue.I guess we need to find another way to detect
Vulkan::Headers
(which should be pretty easy but I'm kinda tired to do it now) so if you want to contribute your own fix you can do it and test if it breaks the CI jobs using.ci/scripts/clang/docker.sh
.Also, what
cmake
command did you used? To reproduce your problem in a Docker Container/VM if I feel like it. xoxoWith this CMake command, it doesn't work as in
dev
branch, but does work if I cherry-pick the commit of this PR.Search for Vulkan before QT incase they're already installedto WIP: Search for Vulkan before QT incase they're already installedI am reopening, because the problem is still there (issue #94) and I want to see what is the deal with this fix and the CI. Besides, it was never merged in the first place...
fb7025faff
toedefacddd0
edefacddd0
to48e86d6e84
WIP: Search for Vulkan before QT incase they're already installedto Search for Vulkan before QT incase they're already installedSearch for Vulkan before QT incase they're already installedto Search for Vulkan before Qt has a chance to@Fijxu It seems this new version doesn't break CI.
Looks good. I doubt this is going to break anything for Linux since CI passed but we should also test Windows and MacOs. I do not own a MacOs machine nor a Windows one so I can't really test if the build is successful there after this changes. Since we are working in the dev branch and stability is not the main goal I will merge the PR.
If something goes wrong for any Windows or MacOs users and you know how to fix it feel free to reopen the PR or create a new one.
xoxo.