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

Add Windows JACK support #5716

Merged
merged 9 commits into from
Oct 30, 2020
Merged

Add Windows JACK support #5716

merged 9 commits into from
Oct 30, 2020

Conversation

tresf
Copy link
Member

@tresf tresf commented Oct 16, 2020

JACK is surprisingly plug-and-play with regards to LMMS on Windows, thanks to the hard work of @falkTX and the rest of the JACK2 team combined with our already existing weakjack (thanks @x42) module (preventing the need for windows dev libs).

  • Adds JACK build support for Windows systems
  • Removes libjack-dev dependency unless specified
  • Adds jack2 as a submodule, related Submodule candidates #3963
  • Bumps weakjack submodule to add MSVC support.

To test:

  • Install JACK for Windows from the website
  • Launch JACK with ASIO support per JACK's website instructions
  • Switch LMMS backend to JACK
  • Configure using qjackctl, found in JACK install directory

image

Closes #2136

@LmmsBot
Copy link

LmmsBot commented Oct 16, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

Linux

macOS

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://10066-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg52f780b5b-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10066?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://10067-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg52f780b5b-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10067?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/5l409d92f1rnlf1l/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36057155"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/33armqsq0i5hnai1/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/36057155"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://10069-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg52f780b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10069?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://10065-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-752%2Bg52f780b5b-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/10065?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "00c98a2a35b79549f117ce4b5bd91af842dcfdc2"}

@PhysSong
Copy link
Member

The weakjack submodule should be updated to support MSVC, see x42/weakjack#9.

@tresf tresf mentioned this pull request Oct 20, 2020
.gitmodules Outdated Show resolved Hide resolved
@tresf
Copy link
Member Author

tresf commented Oct 20, 2020

The weakjack submodule should be updated to support MSVC, see x42/weakjack#9.

Done via b1ab5ae.

@tresf
Copy link
Member Author

tresf commented Oct 20, 2020

All notes have been addressed, but there's a lingering issue from #5636 preventing AppVeyor to succeed. I'm fine waiting for that to be fixed to make sure AppVeyor is happy or merging as-is. Thoughts welcome. Pinging @Spekular since he authored #5636.

@Spekular
Copy link
Member

Huh. Is this related to the issue fixed by #5678 somehow? Other builds since then have been fine, so I'm frankly not sure why an issue would pop up now.

@tresf
Copy link
Member Author

tresf commented Oct 20, 2020

Huh. Is this related to the issue fixed by #5678 somehow? Other builds since then have been fine, so I'm frankly not sure why an issue would pop up now.

Ok, sorry for the ping, I did search beforehand but must have missed it. I'll fast-forward and it should fix it, thanks.

Edit: Done via 7927bdd, just awaiting CI. 🤞

@tresf
Copy link
Member Author

tresf commented Oct 20, 2020

Ok, I fast-forwarded my branch didn't fix this @Spekular, can you comment on the AppVeyor error? It's the same as before.

@PhysSong
Copy link
Member

I think (std::numeric_limits<int>::max)() may work. I'll test it out and fix lmms_basic.h as well if that works.

@tresf
Copy link
Member Author

tresf commented Oct 27, 2020

I'm frankly not sure why an issue would pop up now.

@Spekular as discussed in #devtalk on Discord, it appears this is as a result of adding jack.h into AppVeyor, which is new with this PR. So, although the symptom is the same, the cause is likley a 3rdparty header we don't manage. I've made my first attempt at patching this in d56db57. If that doesn't work, I might fire up a MSVC environment instead to avoid the guesswork.

I think (std::numeric_limits<int>::max)() may work.

@DomClark shared his sentiments on Discord, and I tend to agree with them that we shouldn't have to worry about adding parens to each call to max.

I'll test it out and fix lmms_basic.h as well if that works.

I started writing this on a separate branch, but was largely unsuccessful. I'll revisit if needed.

@tresf
Copy link
Member Author

tresf commented Oct 29, 2020

I'll test it out and fix lmms_basic.h as well if that works.

I decided to opt for a top-level...

ADD_DEFINITIONS(-DNOMINMAX)

... which I believe solves the problem proactively and holistically.

On the contrary, lmms_basics.h can't extend to headers which we don't manage, like weak_jack.h, so I believe this ADD_DEFINITIONS approach to be ideal.

Components which build using ExternalProject_Add may not import all flags, so RemoteVstPlugin is left as-is with its existing NOMINMAX flag.

@tresf
Copy link
Member Author

tresf commented Oct 30, 2020

Something's odd with Travis, it's not even queuing my builds (it's been hours and I've tried restarting it once). I'm not holding up the PR for that specifically. There's also a failure after rebase, but it's unrelated and explained here: 435dbc5#r43742718. Merging.

@tresf tresf merged commit dc78b15 into LMMS:master Oct 30, 2020
@tresf tresf deleted the jack-win branch October 30, 2020 20:57
@tresf tresf added the windows label Oct 30, 2020
IanCaio pushed a commit to IanCaio/lmms that referenced this pull request Mar 28, 2021
Add Windows JACK support
Also adds JACK as a submodule
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Add Windows JACK support
Also adds JACK as a submodule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jack audio support on windows
6 participants