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

macOS support and fixes #2201

Closed
wants to merge 10 commits into from

Conversation

CamilleScholtz
Copy link
Contributor

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:

  • fixes the macOS GitHub workflow (it builds now!)
  • fixes issue OSXAudio causes SEGV only when daemonized  #1879 / Daemon mode does not work with MacOS #1901. The problem here seems to be that when 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 call fork() 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 the OBJC_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.

@MaxKellermann
Copy link
Member

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.

.gitignore Outdated Show resolved Hide resolved
ccache
expat
fmt
googletest
pcre2
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

@MaxKellermann
Copy link
Member

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.
I cherry-picked the commits manually, omitting the gitignore commit, the redundant dependency removal commit and the merge commit.
Thanks so far, hope to get more code from you!

@CamilleScholtz
Copy link
Contributor Author

CamilleScholtz commented Feb 1, 2025

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.

@MaxKellermann
Copy link
Member

Please start by checking the CI build failure: https://github.com/MusicPlayerDaemon/MPD/actions/runs/13090691432/job/36526975260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants