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

mopidy-local: init at 3.1.1 #93400

Merged
merged 3 commits into from Oct 18, 2020
Merged

mopidy-local: init at 3.1.1 #93400

merged 3 commits into from Oct 18, 2020

Conversation

@ruuda
Copy link
Contributor

@ruuda ruuda commented Jul 18, 2020

Motivation for this change

Mopidy-Local is the successor to Mopidy-Local-SQLite and
Mopidy-Local-Images, which are both already packaged.

Things done

I had to make gobject-introspection a propagated build input, otherwise Mopidy imported by Mopidy-Local could not import Gst.

  • 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 nixpkgs-review --run "nixpkgs-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.

Tested by building the following:

buildEnv {
  name = "mopidy-with-extensions-${mopidy.version}";
  paths = lib.closePropagation [
    pkgs.mopidy-iris
    pkgs.mopidy-local
  ];
  pathsToLink = [ "/${mopidyPackages.python.sitePackages}" ];
  buildInputs = [ makeWrapper ];
  postBuild = ''
    makeWrapper ${mopidy}/bin/mopidy $out/bin/mopidy \
      --prefix PYTHONPATH : $out/${mopidyPackages.python.sitePackages}
  '';
}

The bin/mopidy in there has a mopidy local scan command that successfully scanned my local collection.

@ruuda
Copy link
Contributor Author

@ruuda ruuda commented Jul 18, 2020

CC @rvolosatovs, who is the maintainer of the mopidy-local-* packages.

Copy link
Member

@rvolosatovs rvolosatovs left a comment

LGTM
Can we remove the other two as well then? I added them initially as a dependency to mopidy-iris, which does not need them anymore apparently.

@ruuda
Copy link
Contributor Author

@ruuda ruuda commented Jul 19, 2020

Can we remove the other two as well then? I added them initially as a dependency to mopidy-iris, which does not need them anymore apparently.

Mopidy-Iris does not need them, it works with the newer Mopidy-Local now. I added a commit to delete the older packages. The only thing I wonder, should this be recorded in a changelog somewhere? I can imagine people have these older plugins in their config, and that will break when upgrading.

Copy link
Member

@mweinelt mweinelt left a comment

Thanks for taking care of this. I've added a few remarks, nothing major.

We should convert the whole mopidy-shebang to python3 shortly, not necessarily in this pr.

checkInputs = [
python3Packages.pytest
];
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

I think its preferable to use pytestCheckHook over pytest.

@@ -21224,8 +21224,7 @@ in
mopidy
mopidy-gmusic
mopidy-iris
mopidy-local-images
mopidy-local-sqlite
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

Let's add throw aliases to aliases.nix explaining the removal and migration steps.

diff --git a/pkgs/top-level/aliases.nix b/pkgs/top-level/aliases.nix
index 3e9cdc8e87e..634ecc3af0e 100644
--- a/pkgs/top-level/aliases.nix
+++ b/pkgs/top-level/aliases.nix
@@ -301,6 +301,8 @@ mapAliases ({
   mcgrid = throw "mcgrid has been removed from nixpkgs, as it's not compatible with rivet 3"; # added 2020-05-23
   mcomix = throw "mcomix has been removed from nixpkgs, as it's unmaintained"; # added 2019-12-10
   mirage = throw "mirage has been femoved from nixpkgs, as it's unmaintained"; # added 2019-12-10
+  mopidy-local-images = throw "mopidy-local-images has been removed as it's unmaintained. It's functionality has been merged into the mopidy-local extension."; # added 2020-10-18
+  mopidy-local-sqlite = throw "mopidy-local-sqlite has been removed as it's unmaintained. It's functionality has been merged into the mopidy-local extension."; # added 2020-10-18
   mysql-client = hiPrio mariadb.client;
   memtest86 = memtest86plus; # added 2019-05-08
   mesa_noglu = mesa; # added 2019-05-28

Copy link
Contributor Author

@ruuda ruuda Oct 18, 2020

Ah, so this is how to deal with removing packages, thanks for pointing that out!

@@ -0,0 +1,30 @@
{ stdenv
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

Suggested change
{ stdenv
{ lib

python3Packages.pytest
];

meta = with stdenv.lib; {
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

Suggested change
meta = with stdenv.lib; {
meta = with lib; {

homepage = "https://github.com/mopidy/mopidy-local";
description = "Mopidy extension for playing music from your local music archive";
license = licenses.asl20;
maintainers = [ maintainers.ruuda ];
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

Suggested change
maintainers = [ maintainers.ruuda ];
maintainers = with maintainers; [ ruuda ];

] ++ (
with pythonPackages; [
Copy link
Member

@mweinelt mweinelt Oct 18, 2020

Suggested change
] ++ (
with pythonPackages; [
] ++ (with pythonPackages; [

@ruuda
Copy link
Contributor Author

@ruuda ruuda commented Oct 18, 2020

Thanks for having a look @mweinelt. I addressed your comments and also rebased this PR on top of the latest master.

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Oct 18, 2020

Do you have experience on how these plugins interact with each other? I see we mix and match python2 and python3 there.

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Oct 18, 2020

20b2e8e

This commit should be split in two (local-images and local-sqlite) and use the format described in CONTRIBUTING.md, e.g.

mopidy-local-images: remove

The what and why here

ruuda added 2 commits Oct 18, 2020
Mopidy-Local is the successor to Mopidy-Local-SQLite and
Mopidy-Local-Images, which are already packaged. I had to make
gobject-introspection a propagated build input, otherwise
Mopidy-Local can't import Mopidy.
This plugin has been merged into the newer "mopidy-local" plugin which I
just added. "mopidy-local-images" and "mopidy-local-sqlite" were added
originally for Mopidy Iris, but Iris now works with mopidy-local, and
does not need the older plugins any more.
This plugin has been merged into the newer "mopidy-local" plugin which I
just added. "mopidy-local-images" and "mopidy-local-sqlite" were added
originally for Mopidy Iris, but Iris now works with mopidy-local, and
does not need the older ones any more.
@ruuda
Copy link
Contributor Author

@ruuda ruuda commented Oct 18, 2020

Do you have experience on how these plugins interact with each other? I see we mix and match python2 and python3 there.

I don’t think we do, the packages reference pythonPackages, but that is set here:

pythonPackages = python.pkgs;

And the Python there is set to python3 here:

mopidyPackages = callPackages ../applications/audio/mopidy/default.nix {
python = python3;
};

So it’s all running with Python 3.

This commit should be split in two (local-images and local-sqlite)

Done.

@mweinelt
Copy link
Member

@mweinelt mweinelt commented Oct 18, 2020

Result of nixpkgs-review pr 93400 1

15 packages built:
  • mopidy
  • mopidy-gmusic
  • mopidy-iris
  • mopidy-local
  • mopidy-moped
  • mopidy-mopify
  • mopidy-mpd
  • mopidy-mpris
  • mopidy-musicbox-webclient
  • mopidy-somafm
  • mopidy-soundcloud
  • mopidy-spotify
  • mopidy-spotify-tunigo
  • mopidy-tunein
  • mopidy-youtube

@mweinelt mweinelt merged commit c3054b7 into NixOS:master Oct 18, 2020
2 of 3 checks passed
@mweinelt
Copy link
Member

@mweinelt mweinelt commented Oct 18, 2020

As the mopidy-local-{sqlite,images} packages are broken on 20.09 I think this is worth backporting to release-20.09.

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

Successfully merging this pull request may close these issues.

None yet

4 participants