Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qt: add Vulkan/molten-vk support #111474

Closed
wants to merge 1 commit into from
Closed

qt: add Vulkan/molten-vk support #111474

wants to merge 1 commit into from

Conversation

loganmc10
Copy link

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Hi, I'm hoping to resurrect #50038, I'm not sure why it wasn't merged. See also https://www.qt.io/blog/2018/05/30/vulkan-for-qt-on-macos

I am the developer of simple64 (https://simple64.github.io), an N64 emulator. I'd like to add Mac OS support, and this is blocking me

@BrewTestBot BrewTestBot added ffmpeg FFMPEG use is a significant feature of the PR or issue icu4c ICU use is a significant feature of the PR or issue long build Set a long timeout for formula testing no Linux bottle Formula has no Linux bottle nodejs Node or npm use is a significant feature of the PR or issue perl Perl use is a significant feature of the PR or issue labels Sep 22, 2022
@loganmc10
Copy link
Author

@carlocab I see that you've been handling recent Qt commits

Formula/qt.rb Outdated Show resolved Hide resolved
Formula/qt.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member

Please don't tag individual maintainers, no pull request is not important enough to demand volunteer time like that.

@carlocab carlocab changed the title Add Vulkan/molten-vk support to Qt qt: add Vulkan/molten-vk support Sep 23, 2022
@carlocab
Copy link
Member

carlocab commented Sep 23, 2022

I don't have the time to review this properly, but a few comments:

  1. Commit message needs fixing. (See guide in PR template or summary below.)
  2. This dependency needs to be macOS-only, since MoltenVK is not available on Linux.
  3. MoltenVK is a pretty heavy dependency to add for something that appears pretty niche.

Re commit style:

Please use the preferred commit-message style for homebrew/core. We put the name of the formula first in commit-message headings.

For new formulae:

At Homebrew, we like to put the name of the formula up front like so: foobar 7.3 (new formula).

For existing formulae:

The preferred commit message format for simple version updates is foobar 7.3 and for fixes is foobar: fix flibble matrix..

Refer to the commit style guide for more details. Also, when making further changes to your pull request, use the following guidelines to make sure that @BrewTestBot can merge your commits:

  • One formula per commit; one commit per formula.
  • Keep merge commits out of the pull request.

@loganmc10
Copy link
Author

Commit message needs fixing. (See guide in PR template or summary below.)

Done

This dependency needs to be macOS-only, since MoltenVK is not available on Linux.

Done

MoltenVK is a pretty heavy dependency to add for something that appears pretty niche.

Qt is primarily used for GUI applications, and with OpenGL deprecated on Mac OS, it's really the only good option for GPU accelerated applications. If you install Qt on a Linux distro, or Windows, it's going to come with Vulkan support, so excluding it on brew causes a significant difference in features vs Linux or Windows.

@carlocab
Copy link
Member

Thoughts here, @paperchalice, @jonaski?

@carlocab
Copy link
Member

excluding it on brew causes a significant difference in features vs Linux or Windows.

This doesn't really bother me -- having fewer features than on Linux is pretty typical for macOS. To me, it's really a matter of whether there will be a significant fraction of users of this formula who would find this useful.

@paperchalice
Copy link
Contributor

paperchalice commented Sep 24, 2022

qt with vulkan support in fact doesn't contain the molten-vk linkage, so mark it as build dependency and introduce build dependency vulkan-headers is enough, and the extra cmake flags are unnecessary here. For test, we need vulkan-loader, vulkan-headers and a vulkan implementation. BTW there is an upstream issue about it on macOS.

@carlocab
Copy link
Member

qt with vulkan support in fact doesn't contain the molten-vk linkage, so mark it as build dependency and introduce build dependency vulkan-headers is enough, and the extra cmake flags are unnecessary here. For test, we need vulkan-loader, vulkan-headers and a vulkan implementation. BTW there is an upstream issue about it on macOS.

If it's a build-only dependency then I'm fine with this. We do need a test, though; thanks for laying out a starting point.

@loganmc10
Copy link
Author

Full disclosure: I don't have a Mac or any way to test this, so I'm fine with whatever solution as long as Vulkan support is added to Qt, I'd like to be able to provide a Mac build of the emulator

Formula/qt.rb Outdated Show resolved Hide resolved
Formula/qt.rb Outdated Show resolved Hide resolved
@loganmc10
Copy link
Author

ok PR has been updated per @paperchalice suggestions

@carlocab
Copy link
Member

Full disclosure: I don't have a Mac or any way to test this, so I'm fine with whatever solution as long as Vulkan support is added to Qt, I'd like to be able to provide a Mac build of the emulator

Does this mean you haven't been able to test whether changing our Qt build in this way works for your changes at simple64/simple64#316?

If we add a test for Vulkan support I can cherry-pick this onto #110816 and get a build going.

@loganmc10
Copy link
Author

Does this mean you haven't been able to test whether changing our Qt build in this way works for your changes at simple64/simple64#316?

Correct, simple64 used to support Mac, another developer worked on the support, but it stopped working when we migrated from OpenGL to Vulkan. I'm trying to get a DMG again so that someone can test it and get it working if anything needs to be done.

@carlocab
Copy link
Member

Ok, given that we don't even know if this change works for anybody, I'm going to insist that we have a test for this functionality. You may need to find a Mac to build and test on -- GitHub runners will not suffice (the time to build Qt exceeds their 6 hour time limit).

Or, perhaps @paperchalice might be interested in picking this up.

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Oct 18, 2022
@carlocab
Copy link
Member

I don't think there's going to be any progress here, so I'm closing this.

If you'd still like to pursue this change, please feel free to open a new pull request. However, if you choose to do so: please test that your changes produce the intended result, and update the formula's test to include a minimal check for the usability of the new features.

@carlocab carlocab closed this Oct 18, 2022
@cho-m cho-m mentioned this pull request Dec 5, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ffmpeg FFMPEG use is a significant feature of the PR or issue icu4c ICU use is a significant feature of the PR or issue long build Set a long timeout for formula testing no Linux bottle Formula has no Linux bottle nodejs Node or npm use is a significant feature of the PR or issue perl Perl use is a significant feature of the PR or issue stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants