suyu
/
suyu
153
183
Fork 26
Code Issues 76 Pull Requests 4 Actions 6 Packages Projects Releases 2 Wiki Activity

Search for Vulkan before Qt has a chance to #45

Merged
Fijxu merged 1 commits from lvella/suyu:qt6-search-fix into dev 2024-03-30 20:08:53 +01:00
Conversation 6 Commits 1 Files Changed 1 +64 -62
lvella commented 2024-03-24 17:46:49 +01:00
Contributor
Copy Link

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.

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.
lvella added 1 commit 2024-03-24 17:46:49 +01:00
suyu-ci / reuse (pull_request) Failing after 10s Details
codespell / Check for spelling errors (pull_request) Failing after 11s Details
suyu verify / verify format (pull_request) Failing after 9s Details
suyu verify / test build (linux-fresh, clang) (pull_request) Has been skipped Details
suyu verify / test build (linux-fresh, linux) (pull_request) Has been skipped Details
suyu verify / test build (linux-mingw, windows) (pull_request) Has been skipped Details
suyu verify / android (pull_request) Has been skipped Details
466edad7ba Searching for Vulkan before QT6, so it doesn't messes with Vulkan.
Exverge reviewed 2024-03-24 17:55:12 +01:00
CMakeLists.txt Outdated
@ -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.
Exverge commented 2024-03-24 17:55:12 +01:00
Member
Copy Link

No need for two lines here, otherwise looks good

No need for two lines here, otherwise looks good
lvella commented 2024-03-24 18:25:36 +01:00
Author
Contributor
Copy Link

Fixed.

Fixed.
lvella force-pushed qt6-search-fix from 466edad7ba to fb7025faff 2024-03-24 18:25:28 +01:00 Compare
Exverge approved these changes 2024-03-24 18:39:48 +01:00
Exverge commented 2024-03-24 19:04:15 +01:00
Member
Copy Link

Merged (a9312c837e)

Merged (a9312c837e)
Exverge closed this pull request 2024-03-24 19:04:15 +01:00
Exverge changed title from Searching for Vulkan before QT6, so it doesn't messes with Vulkan. to Search for Vulkan before QT incase they're already installed 2024-03-24 19:06:26 +01:00
Fijxu commented 2024-03-25 04:03:34 +01:00
Owner
Copy Link

This 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.

This 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](https://git.suyu.dev/suyu/suyu/src/branch/dev/externals) 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.
Exverge commented 2024-03-25 13:17:30 +01:00
Member
Copy Link

@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.

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.

> @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. 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.
lvella reviewed 2024-03-26 21:46:18 +01:00
CMakeLists.txt Outdated
@ -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)
lvella commented 2024-03-26 21:46:18 +01:00
Author
Contributor
Copy Link

This is not the proper way to handle your intended fix @lvella because you literally deleted the detection of the externals folder

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.

> This is not the proper way to handle your intended fix @lvella because you literally deleted the detection of the externals folder 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.
Fijxu commented 2024-03-27 01:41:02 +01:00
Owner
Copy Link

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. xoxo

>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. xoxo
lvella commented 2024-03-29 02:05:28 +01:00
Author
Contributor
Copy Link
cmake .. -G Ninja -DSUYU_TESTS=off -DSUYU_USE_EXTERNAL_SDL2=off -DENABLE_QT6=on -DSUYU_USE_PRECOMPILED_HEADERS=off

With this CMake command, it doesn't work as in dev branch, but does work if I cherry-pick the commit of this PR.

``` cmake .. -G Ninja -DSUYU_TESTS=off -DSUYU_USE_EXTERNAL_SDL2=off -DENABLE_QT6=on -DSUYU_USE_PRECOMPILED_HEADERS=off ``` With this CMake command, it doesn't work as in `dev` branch, but does work if I cherry-pick the commit of this PR.
lvella changed title from Search for Vulkan before QT incase they're already installed to WIP: Search for Vulkan before QT incase they're already installed 2024-03-30 15:28:29 +01:00
lvella commented 2024-03-30 15:28:40 +01:00
Author
Contributor
Copy Link

I 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...

I 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...
lvella reopened this pull request 2024-03-30 15:28:41 +01:00
lvella force-pushed qt6-search-fix from fb7025faff to edefacddd0 2024-03-30 15:28:57 +01:00 Compare
lvella force-pushed qt6-search-fix from edefacddd0 to 48e86d6e84 2024-03-30 16:15:09 +01:00 Compare
lvella changed title from WIP: Search for Vulkan before QT incase they're already installed to Search for Vulkan before QT incase they're already installed 2024-03-30 16:39:16 +01:00
lvella changed title from Search for Vulkan before QT incase they're already installed to Search for Vulkan before Qt has a chance to 2024-03-30 16:40:12 +01:00
lvella commented 2024-03-30 17:06:11 +01:00
Author
Contributor
Copy Link

@Fijxu It seems this new version doesn't break CI.

@Fijxu It seems this new version doesn't break CI.
lvella requested review from Fijxu 2024-03-30 17:06:30 +01:00
Fijxu approved these changes 2024-03-30 20:00:09 +01:00
Fijxu commented 2024-03-30 20:08:16 +01:00
Owner
Copy Link

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.

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.
Fijxu merged commit 48e86d6e84 into dev 2024-03-30 20:08:53 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Exverge
Fijxu
Labels
Clear labels   
CI

   
Low Priority

   
Needs Approvals

   
Needs Rebase

   
android

Relating to the android build

  
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
translations

Relating to strings for different langauges

  
wontfix

This won't be fixed

No Label
CI
Low Priority
Needs Approvals
Needs Rebase
android
bug
duplicate
enhancement
help wanted
invalid
question
translations
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: suyu/suyu#45
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?