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

mpdris2: use python3 for #74295 #74373

Merged
merged 1 commit into from Dec 4, 2019
Merged

Conversation

pjones
Copy link
Contributor

@pjones pjones commented Nov 27, 2019

Motivation for this change

Update to python3 as per #74295

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @worldofpeace

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

From looking at the imports you need glib and libnotify in buildInputs.
Please also port to using python3Packages.buildPythonApplication, where you'll need to set format = "other"; to use the make build.

Then add gobject-introspection and wrapGAppsHook to nativeBuildInputs.
Note you can also drop wrapPython as buildPythonApplication automatically includes a variety of hooks.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/mpdris2/default.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/mpdris2/default.nix Outdated Show resolved Hide resolved
pkgs/tools/audio/mpdris2/default.nix Outdated Show resolved Hide resolved
@pjones
Copy link
Contributor Author

pjones commented Nov 27, 2019

The only thing that doesn't seem to work is notifications. No matter where I placed glib and libnotify (e.g., buildInputs, nativeBuildInputs, etc.) I couldn't get them to work.

I'm not a Python programmer so if there are any more Python-specific changes that need to be made I'd appreciate patches. Thank you.

@worldofpeace
Copy link
Contributor

@pjones That's because this uses the libnotify library and glib through gobject-introspection.
See my review #74373 (review) with exact instructions.

@worldofpeace
Copy link
Contributor

@worldofpeace
Copy link
Contributor

Oh, forgot to mention the gobject-introspection setup hook doesn't work with stictDeps which is default in buildPython.

You need to set strictDeps = false;.

@jtojnar
Copy link
Contributor

jtojnar commented Nov 28, 2019

And add a comment linking to #56943 next to that.

@pjones
Copy link
Contributor Author

pjones commented Nov 28, 2019

Latest commit includes all recommended changes. However notifications don't seem to be working.

@jtojnar
Copy link
Contributor

jtojnar commented Nov 28, 2019

The notifications work for me.

I created mpd.conf:

music_directory     "/home/jtojnar/Music"
playlist_directory  "/home/jtojnar/Music"
state_file          "/home/jtojnar/.cache/mpd/state"
sticker_file        "/home/jtojnar/.cache/mpd/sticker.sql"
user                "jtojnar"
group               "users"

Then ran mkdir ~/.cache/mpd, followed by nix run -f '<nixpkgs>' mpd -c mpd mpd.conf.

The already running mpDris2 almost immediately shown “Reconnected” notification.

What desktop environment are you using. Do you have notification server running?

@worldofpeace worldofpeace force-pushed the pjones/mpdris2 branch 2 times, most recently from d59e9d2 to 9ac55c6 Compare December 3, 2019 23:07
@worldofpeace
Copy link
Contributor

@pjones I've pushed the requested changes. Could you try again now?

@pjones
Copy link
Contributor Author

pjones commented Dec 4, 2019

@worldofpeace I tried commit 9ac55c6a743cdddf8768b04b8b58b75a23abd7f3 and everything works except notifications. I'm running KDE and have no problems with notifications in general.

I recently stopped using mpdris2. I'm going to push a commit that removes me from the maintainers list.

Also use fetchFromGitHub for hash stability.

Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
@worldofpeace
Copy link
Contributor

worldofpeace commented Dec 4, 2019

@worldofpeace I tried commit 9ac55c6 and everything works except notifications. I'm running KDE and have no problems with notifications in general.

Strange, I believe @jtojnar and I both tested under Gnome3. So perhaps it's related to this, or something is missing from the wrapper that just happen to be in our global environment (or terminal).

@worldofpeace
Copy link
Contributor

I recently stopped using mpdris2. I'm going to push a commit that removes me from the maintainers list.

Got it 👍 I believe you've done this in the force push, will merge.

@worldofpeace worldofpeace merged commit 9782b3e into NixOS:master Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants