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
Member

@worldofpeace worldofpeace left a comment

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 pjones force-pushed the pjones:pjones/mpdris2 branch from a86c9c5 to c185198 Nov 27, 2019
@pjones
Copy link
Contributor Author

@pjones 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
Member

@worldofpeace worldofpeace commented Nov 27, 2019

@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
Member

@worldofpeace worldofpeace commented Nov 27, 2019

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 jtojnar commented Nov 28, 2019

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

@pjones pjones force-pushed the pjones:pjones/mpdris2 branch from c185198 to 488b4a6 Nov 28, 2019
@pjones
Copy link
Contributor Author

@pjones pjones commented Nov 28, 2019

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

@jtojnar
Copy link
Contributor

@jtojnar 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:pjones/mpdris2 branch 2 times, most recently from d59e9d2 to 9ac55c6 Dec 3, 2019
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Dec 3, 2019

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

@pjones
Copy link
Contributor Author

@pjones pjones 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.

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>
@pjones pjones force-pushed the pjones:pjones/mpdris2 branch from 9ac55c6 to db091af Dec 4, 2019
@worldofpeace
Copy link
Member

@worldofpeace 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
Member

@worldofpeace worldofpeace commented Dec 4, 2019

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
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
mpdris2 on aarch64-linux Success
Details
mpdris2 on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.