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

pulseaudio: add libintl dependency on darwin #172230

Closed
wants to merge 2 commits into from

Conversation

otavio
Copy link
Contributor

@otavio otavio commented May 9, 2022

Log of failure: https://hydra.nixos.org/build/174832426/nixlog/4
Refs: ZHF #172160
Signed-off-by: Otavio Salvador [email protected]

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@otavio otavio added the 0.kind: build failure A package fails to build label May 9, 2022
@otavio otavio requested a review from thiagokokada May 9, 2022 17:44
@siraben
Copy link
Member

siraben commented May 9, 2022

Result of nixpkgs-review pr 172230 run on aarch64-darwin 1

7 packages marked as broken and skipped:
  • anki
  • imgbrd-grabber
  • mnemosyne
  • projectm
  • retro-gtk
  • rootbar
  • shattered-pixel-dungeon
11 packages failed to build:
  • castty
  • gnomeExtensions.sound-output-device-chooser
  • libcanberra
  • libcanberra-gtk2
  • libcanberra-gtk3
  • libpulseaudio
  • libsoundio
  • moc
  • pulsemixer
  • rhvoice
  • roc-toolkit

@siraben
Copy link
Member

siraben commented May 9, 2022

❯ nix log /nix/store/jk3ya7d6i0nycwfmd4mrcnxxz2zsaanq-libpulseaudio-15.0.drv | ix
http://ix.io/3XoZ

@otavio
Copy link
Contributor Author

otavio commented May 9, 2022

@siraben I am on Linux so it is hard for me to debug this. Could you take a look?

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 9, 2022
@tjni
Copy link
Contributor

tjni commented May 9, 2022

When I was looking at this earlier, I found the same issue building locally. After passing the libintl issue, the dbus issue appeared. Passing the dbus issue, other ones appeared. In my opinion, we should merge this since it seems necessary and does not make things worse, even though it does not fix the package on Darwin.

@tjni
Copy link
Contributor

tjni commented May 9, 2022

Note that libintl is defined as:

# On non-GNU systems we need GNU Gettext for libintl.
libintl = if stdenv.hostPlatform.libc != "glibc" then gettext else null;

which means, depending on taste, it might be preferred to include libintl on all systems, not only Darwin.

@thiagokokada
Copy link
Contributor

When I was looking at this earlier, I found the same issue building locally. After passing the libintl issue, the dbus issue appeared. Passing the dbus issue, other ones appeared. In my opinion, we should merge this since it seems necessary and does not make things worse, even though it does not fix the package on Darwin.

One thing that I found strange is that dbus shouldn't be used in macOS AFAIK (it is a Linux specific thing).

I am trying to do an investigation in my local machine right now. Don't think it is worth merging it in the current state.

@ofborg ofborg bot requested a review from lovek323 May 9, 2022 21:45
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 9, 2022
@tjni
Copy link
Contributor

tjni commented May 9, 2022

One thing that I found strange is that dbus shouldn't be used in macOS AFAIK (it is a Linux specific thing).

I am trying to do an investigation in my local machine right now. Don't think it is worth merging it in the current state.

Sounds good! Let me know if I can help. I (only) have an M1 machine.

Still doesn't succesfully build.
@thiagokokada
Copy link
Contributor

One thing that I found strange is that dbus shouldn't be used in macOS AFAIK (it is a Linux specific thing).
I am trying to do an investigation in my local machine right now. Don't think it is worth merging it in the current state.

Sounds good! Let me know if I can help. I (only) have an M1 machine.

Now it starts to build, but it fails during compilation. I think this may be related to the compiler maybe, or something else.

@thiagokokada
Copy link
Contributor

Now maybe it makes sense to merge as-is since at least it tries to build. Still not sure if we want it or not.

BTW, from the looks of it, this release (15.0) seems problematic on Darwin. Both macports and homebrew are still packaging the 14.2 release.

@tjni
Copy link
Contributor

tjni commented May 9, 2022

If dbus should not be used on OSX, it seems we need to disable it explicitly. In meson_options.txt, the dbus feature is declared auto, and nixpkgs defaults auto to required. We could fix this by adding to mesonFlags:

++ lib.optional stdenv.isDarwin "-Ddbus=disabled"

You're right that this then runs into further issues, and I agree it could be best to take it one step at a time.

@tjni
Copy link
Contributor

tjni commented May 9, 2022

I also get compile-time errors, such as:

In file included from ../src/pulsecore/cpu-x86.c:28:
/nix/store/qlicbgwj6mddbcdcfsqqp7g5fq3rphd1-clang-wrapper-11.1.0/resource-root/include/cpuid.h:11:2: error: this header is for x86 only
#error this header is for x86 only

which looks related to being on an M1 Mac.

If we want to try to continue with this version, I would advocate for committing the following:

  1. Add libintl as a build time and run time dependency (I like unconditionally, but it can also be conditional on Darwin).
  2. Disable oss-output and dbus on Darwin (at least for now)
  3. Add glib as a build time and run time dependency (so that gio-2 is available)

That seems to get us past the configuration phase, and then we can try to tackle the compilation (or mark as broken if we can't figure it out).

I suppose we could also continue using 14.2 on Darwin?

Comment on lines +107 to +109
++ lib.optionals (stdenv.isDarwin) [
"-Doss-output=disabled"
];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
++ lib.optionals (stdenv.isDarwin) [
"-Doss-output=disabled"
];
++ lib.optional (stdenv.isDarwin) "-Doss-output=disabled";

@otavio
Copy link
Contributor Author

otavio commented May 9, 2022

@tjni keep the last known working version does make sense.

@ofborg ofborg bot added 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 10, 2022
@thefloweringash
Copy link
Member

I’ve also spent some time on pulseaudio on Darwin but I don’t think I have the time to see it through at the moment. Here’s where I got to: https://github.com/NixOS/nixpkgs/compare/master...thefloweringash:libpulseaudio-darwin?expand=1

@thiagokokada
Copy link
Contributor

thiagokokada commented May 10, 2022

If dbus should not be used on OSX, it seems we need to disable it explicitly.

Ignore what I said before. Dbus is used both for Homebrew and MacPorts.

I still think this is mostly unnecessary for macOS, but if it builds and works we should keep it.

@thiagokokada
Copy link
Contributor

If dbus should not be used on OSX, it seems we need to disable it explicitly.

Ignore what I said before. Dbus is used both for Homebrew and MacPorts.

I still think this is mostly unnecessary for macOS, but if it builds and works we should keep it.

https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/pulseaudio.rb#L55
https://github.com/macports/macports-ports/blob/master/audio/pulseaudio/Portfile#L49

Sorry, so dbus is disabled in Homebrew and enabled in MacPorts. I think we are not really interested in dbus inside Darwin (it does seems to build at least though), so yeah, maybe it makes sense to disable it.

@thiagokokada
Copy link
Contributor

I’ve also spent some time on pulseaudio on Darwin but I don’t think I have the time to see it through at the moment. Here’s where I got to: https://github.com/NixOS/nixpkgs/compare/master...thefloweringash:libpulseaudio-darwin?expand=1

Nice. I took your PR and did a clean-up and made it build: #172307

@SuperSandro2000
Copy link
Member

Nice. I took your PR and did a clean-up and made it build: #172307

Closing in favor of yours.

@wegank wegank mentioned this pull request Jul 31, 2022
13 tasks
@otavio otavio deleted the topic/fix-zhf-libpulseaudio branch August 13, 2022 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants