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

[WIP] soundux: init at 0.2.7 #160181

Closed
wants to merge 3 commits into from
Closed

[WIP] soundux: init at 0.2.7 #160181

wants to merge 3 commits into from

Conversation

pasqui23
Copy link
Contributor

@pasqui23 pasqui23 commented Feb 15, 2022

Motivation for this change
Things done

Builds succeeds and the programs run.

  • 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.

@Slabity
Copy link
Contributor

Slabity commented Feb 16, 2022

@pasqui23 - A short while ago I took the derivation from an older PR for soundux and fixed it up to compile and work on my system. It might help if you're going to try and get it brought into the repo:

{
  lib, stdenv, fetchFromGitHub, cmake, ninja, pkg-config, makeWrapper, wrapGAppsHook,
  libappindicator-gtk3, openssl, pipewire, pulseaudio, webkitgtk, xorg,
  libpulseaudio, libwnck3,
  downloaderSupport ? true, ffmpeg, youtube-dl
}:

let
  downloaderPath = lib.makeBinPath [ffmpeg youtube-dl ];
  # Soundux loads pipewire, pulse and libwnck optionally during runtime
  dynamicLibraries = lib.makeLibraryPath [libpulseaudio pipewire libwnck3 ];
in
stdenv.mkDerivation rec {
  pname = "soundux";
  version = "0.2.7";

  src = fetchFromGitHub {
    owner = "Soundux";
    repo = "Soundux";
    rev = version;
    fetchSubmodules = true;
    sha256 = "15kd9vl7inn8zm5cqzjkb6zb9xk2xxwpkm7fx1za3dy9m61sq839";
  };

  cmakeFlags = [ "-DCMAKE_BUILD_TYPE=Release" ];

  dontWrapGApps = true;

  installPhase = ''
    mkdir -p $out/opt $out/bin
    cp -r dist soundux-${version} $out/opt
  '';

  postFixup = ''
    makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
      --prefix LD_LIBRARY_PATH ":" ${dynamicLibraries} \
      "''${gappsWrapperArgs[@]}" \
      ${lib.optionalString downloaderSupport "--prefix PATH \":\" " + downloaderPath}
  '';

  nativeBuildInputs = [ cmake ninja pkg-config makeWrapper wrapGAppsHook ];
  buildInputs = [
    libappindicator-gtk3
    openssl
    pipewire
    pulseaudio
    webkitgtk
    xorg.libX11
    xorg.libXtst
  ];

  meta = with lib; {
    homepage = "https://soundux.rocks/";
    description = "cross-platform soundboard";
    license = licenses.gpl3Only;
    platforms = platforms.linux;
  };
}

@pasqui23
Copy link
Contributor Author

@Slabity Sorry even with your fix still does not compile

@Slabity
Copy link
Contributor

Slabity commented Feb 17, 2022

@Slabity Sorry even with your fix still does not compile

Strange. That derivation builds for me and lets me run it without any issue.

@pasqui23 pasqui23 marked this pull request as ready for review February 23, 2022 20:08
@pasqui23
Copy link
Contributor Author

Ok @Slabity my mistake. Now it builds and run perfectly

Copy link
Contributor

@yshym yshym left a comment

Choose a reason for hiding this comment

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

Package file should be formatted with nixpkgs-fmt

mkdir -p $out/opt $out/bin
cp -r dist soundux-${version} $out/opt
'';

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please add desktopItem attribute here (check discord package for example)

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll also need to create share directory and create a symlink to it like here

pkgs/applications/audio/soundux/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/soundux/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/soundux/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/soundux/default.nix Outdated Show resolved Hide resolved
Comment on lines 53 to 94
buildInputs = [
libappindicator-gtk3
openssl
pipewire
pulseaudio
webkitgtk
xorg.libX11
xorg.libXtst
];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
buildInputs = [
libappindicator-gtk3
openssl
pipewire
pulseaudio
webkitgtk
xorg.libX11
xorg.libXtst
];
buildInputs = with xorg; [
libappindicator-gtk3
openssl
pipewire
pulseaudio
webkitgtk
libX11
libXtst
];

@pasqui23 pasqui23 force-pushed the soundux branch 2 times, most recently from e18c8a8 to 7674e08 Compare April 10, 2022 15:03
Copy link
Member

@dali99 dali99 left a comment

Choose a reason for hiding this comment

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

@Slabity What was your fix? as far as I can tell what you posted is just, exactly my package 4943cd2 but with the maintainer field removed.

The reason the original PR wasn't merged was because the libraries used needed to be devendored, you'll have to do that as well if you want this merged.

In it's current form this is 99% my work anyways though lol, and most of the things commented on previously were already covered by the final version in the aforementiond PR (#129571)

I applaud you for picking up the mantle and don't at all mind. But you should be careful about these things in the future since it's important that you actually hold legal copyright over the work you try to commit. (And the line for how much you can copy, and how complex that can be is generally fairly poorly defined)

@Slabity
Copy link
Contributor

Slabity commented Apr 17, 2022

@dali99 - Uh... I think I screwed up my copy-paste because I definitely added a lot more dependencies in mine to fix some linking issues and get all the additional features I wanted. And it doesn't look like I removed the maintainer field. Here's what I have in my repo at the moment:

{
  lib, stdenv, fetchFromGitHub, cmake, ninja, pkg-config, makeWrapper, wrapGAppsHook,
  libappindicator-gtk3, pcre, openssl, pipewire, pulseaudio, webkitgtk, xorg, utillinux,
  libpulseaudio, libwnck3, libselinux, libsepol, libthai, libdatrie,
  libxkbcommon, epoxy, dbus_tools,
  downloaderSupport ? true, ffmpeg, youtube-dl
}:

let
  downloaderPath = lib.makeBinPath [ffmpeg youtube-dl ];
  # Soundux loads pipewire, pulse and libwnck optionally during runtime
  dynamicLibraries = lib.makeLibraryPath [libpulseaudio pipewire libwnck3 ];
in
stdenv.mkDerivation rec {
  pname = "soundux";
  version = "0.2.7";

  src = fetchFromGitHub {
    owner = "Soundux";
    repo = "Soundux";
    rev = version;
    fetchSubmodules = true;
    sha256 = "15kd9vl7inn8zm5cqzjkb6zb9xk2xxwpkm7fx1za3dy9m61sq839";
  };

  cmakeFlags = [ "-DCMAKE_BUILD_TYPE=Release" ];

  dontWrapGApps = true;

  installPhase = ''
    mkdir -p $out/opt $out/bin
    cp -r dist soundux-${version} $out/opt
  '';

  postFixup = ''
    makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
      --prefix LD_LIBRARY_PATH ":" ${dynamicLibraries} \
      "''${gappsWrapperArgs[@]}" \
      ${lib.optionalString downloaderSupport "--prefix PATH \":\" " + downloaderPath}
  '';

  nativeBuildInputs = [ cmake ninja pkg-config makeWrapper wrapGAppsHook ];
  buildInputs = [
    libappindicator-gtk3
    openssl
    libselinux
    libsepol
    libthai
    libdatrie
    utillinux
    pcre
    pipewire
    pulseaudio
    webkitgtk
    xorg.libX11
    xorg.libXtst
    xorg.libXdmcp
    libxkbcommon
    epoxy
    dbus_tools
  ];

  meta = with lib; {
    homepage = "https://soundux.rocks/";
    description = "cross-platform soundboard";
    license = licenses.gpl3Only;
    platforms = platforms.linux;
    maintainers = with maintainers; [ dandellion ];
  };
}

Sorry for the mixup, I wasn't intending to get my changes committed at all. I just happened to see this PR and thought I'd link to the older one since it was closer to working.

@dali99
Copy link
Member

dali99 commented Apr 17, 2022

No worries, everything after the first section was mainly aimed at @pasqui23 anyways :) I was just generally wondering what you did to fix it!

Copy link
Member

@dali99 dali99 left a comment

Choose a reason for hiding this comment

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

By devendoring we refer to building without these libraries coming from the soundux source: https://github.com/Soundux/Soundux/tree/0.2.7/lib and rather being packaged directly in nixpkgs

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/827

Copy link
Contributor

@gador gador left a comment

Choose a reason for hiding this comment

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

Please rebase. If you want me to check again later and build the package, give me a ping.

description = "cross-platform soundboard";
license = licenses.gpl3Only;
platforms = platforms.linux;
maintainers = with maintainers; [ dandellion ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that you are not listed as maintainer?

Copy link
Member

Choose a reason for hiding this comment

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

Listing other people without their approval is not allowed. Please change that.

soundux: made it work(?)

soundux: added desktop file

soundux: formatting
makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
--prefix LD_LIBRARY_PATH ":" ${dynamicLibraries} \
"''${gappsWrapperArgs[@]}" \
${lib.optionalString downloaderSupport "--prefix PATH \":\" " + downloaderPath}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
${lib.optionalString downloaderSupport "--prefix PATH \":\" " + downloaderPath}
${lib.optionalString downloaderSupport "--prefix PATH \":\" ${lib.makeBinPath [ ffmpeg youtube-dl ]}"}

Comment on lines 69 to 70
makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
--prefix LD_LIBRARY_PATH ":" ${dynamicLibraries} \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
--prefix LD_LIBRARY_PATH ":" ${dynamicLibraries} \
# Soundux loads pipewire, pulse and libwnck optionally during runtime
makeWrapper $out/opt/soundux-${version} $out/bin/soundux \
--prefix LD_LIBRARY_PATH ":" ${lib.makeLibraryPath [ libpulseaudio pipewire libwnck3 ]} \

Comment on lines 32 to 37
let
downloaderPath = lib.makeBinPath [ ffmpeg youtube-dl ];
# Soundux loads pipewire, pulse and libwnck optionally during runtime
dynamicLibraries = lib.makeLibraryPath [ libpulseaudio pipewire libwnck3 ];
in
stdenv.mkDerivation rec {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let
downloaderPath = lib.makeBinPath [ ffmpeg youtube-dl ];
# Soundux loads pipewire, pulse and libwnck optionally during runtime
dynamicLibraries = lib.makeLibraryPath [ libpulseaudio pipewire libwnck3 ];
in
stdenv.mkDerivation rec {
stdenv.mkDerivation rec {

owner = "Soundux";
repo = "Soundux";
rev = version;
fetchSubmodules = true;
Copy link
Member

Choose a reason for hiding this comment

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

Vendoring so many libraries is not great.

installPhase = ''
mkdir -p $out/opt $out/bin $out/share
cp -r dist soundux-${version} $out/opt
ln -s "${desktopItem}/share/applications" $out/share/
Copy link
Member

Choose a reason for hiding this comment

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

Please use copyDesktopItems.


installPhase = ''
mkdir -p $out/opt $out/bin $out/share
cp -r dist soundux-${version} $out/opt
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be opt?

description = "cross-platform soundboard";
license = licenses.gpl3Only;
platforms = platforms.linux;
maintainers = with maintainers; [ dandellion ];
Copy link
Member

Choose a reason for hiding this comment

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

Listing other people without their approval is not allowed. Please change that.

Copy link
Member

@dali99 dali99 left a comment

Choose a reason for hiding this comment

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

Your devendoring commit does not devendor.

See my prior comment on what we mean when we say that

@gador
Copy link
Contributor

gador commented Apr 26, 2022

Result of nixpkgs-review pr 160181 run on x86_64-linux 1

1 package failed to build:
  • soundux

@pasqui23 pasqui23 changed the title soundux: init at 0.2.7 [WIP] soundux: init at 0.2.7 Apr 27, 2022
@PAI5REECHO
Copy link

I couldn't get this to run due to some EGL issues:

[00:56:35] [success] Config read
[00:56:35] [success] LibWnck found - Icon support is enabled

(soundux-0.2.7:342196): Gdk-WARNING **: 20:56:35.822: Failed to read portal settings: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface ?org.freedesktop.portal.Settings? on object at path /org/freedesktop/portal/desktop

(soundux-0.2.7:342196): Wnck-WARNING **: 20:56:35.841: libwnck is designed to work in X11 only, no valid display found
[00:56:35] [warning] Failed to get default screen!
[00:56:35] [failure] Could not create IconFetcher instance
[00:56:35] [message] Connected to PipeWire ("pipewire-0") on version "0.3.49"
[00:56:36] [warning] Path "/home/me/audio-clips" does not exist
[00:56:36] [message] Using DISPLAY :0
[00:56:36] [warning] Failed to find iconPath for tray icon
Could not determine the accessibility bus address

(WebKitWebProcess:342222): Gdk-WARNING **: 20:56:36.348: Failed to read portal settings: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.portal.Settings” on object at path /org/freedesktop/portal/desktop
EGLDisplay Initialization failed: EGL_NOT_INITIALIZED
Cannot create EGL context: invalid display (last error: EGL_SUCCESS)

@milahu
Copy link
Contributor

milahu commented May 14, 2022

EGLDisplay Initialization failed: EGL_NOT_INITIALIZED

see also #169201

@SuperSandro2000 SuperSandro2000 marked this pull request as draft June 16, 2022 15:39
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 7, 2023
@NickCao
Copy link
Member

NickCao commented Jan 26, 2023

superseded by #185858

@NickCao NickCao closed this Jan 26, 2023
@pasqui23 pasqui23 deleted the soundux branch February 6, 2023 18:07
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.

None yet

10 participants