-
Notifications
You must be signed in to change notification settings - Fork 357
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
macOS support and fixes #2201
macOS support and fixes #2201
Conversation
This reverts commit 518ce01.
Hey, thanks for stepping in. I'll check your changes another day, but please don't put merge commits in a PR branch. If you want to update your PR branch with the latest upstream changes, rebase instead of merge. |
.github/workflows/build.yml
Outdated
ccache | ||
expat | ||
fmt | ||
googletest | ||
pcre2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for removing all these packages? This means MPD will build without these features, and these features will not be verified by the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macos-latest workflow ships with all of these packages preinstalled, except for libnfs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them because I thought it doesn't hurt to mention these, even if they are preinstalled; if some future image happens to remove them, we're still safe.
This reverts commit bbf71df.
Your last commit is a revert commit. That means you suggest to merge a commit plus a revert of that commit. That does not make much sense, I'd rather have the commit removed from the PR and only merge known-good commits. |
Thanks, the commit messages and PR was a bit of a mess; I was thinking about squashing things or recreating a new proper PR, but that's not needed anymore! I'll keep my eye out for osx issues. |
Please start by checking the CI build failure: https://github.com/MusicPlayerDaemon/MPD/actions/runs/13090691432/job/36526975260 |
I saw that macOS will be dropped in the next version of mpd because there are no maintainers for it. Considering I currently work on an mpd client for macOS, and in addition to that, I am a very happy macOS user of mpd, I was quite disappointed!
I only have basic knowledge of cpp, but I am willing to maintain macOS specific code for mpd. This PR, which might be extended in the coming days, currently:
fork()
is called, macOS's Objective-C runtime may detect that a class initialization was in progress on another thread. This triggers a crash to prevent potential deadlocks or data corruption, as fork() in a multi-threaded context is unsafe. The solution is to callfork()
before initializing any macOS frameworks or threads, I've done this for now by creating an apple specific main (like how there is a win32 specific main), that handles forking at a very early stage. An alternative way would be to launch mpd with theOBJC_DISABLE_INITIALIZE_FORK_SAFETY
environment variable.As I've said before, my knowledge of cpp is quite basic, so if you see any room for improvement or if I'm doing things wrong, I'd love to hear it.