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

gst_all_1.gst-plugins-rs: init at 0.10.7, mopidy-spotify: re-init at unstable-2023-04-21 #225143

Merged
merged 3 commits into from
May 17, 2023

Conversation

lilyinstarlight
Copy link
Member

@lilyinstarlight lilyinstarlight commented Apr 7, 2023

Description of changes

Mopidy-Spotify is back!

This adds the gst-plugins-rs package (for the spotify source) and adds it to Mopidy, so that those plugins are available to it

The hacky postFetch in gst-plugins-rs can be removed when annymosse/color-name#2 is merged (or gst-plugins-rs replaces the crate -- which I will try to make one or the other happen upstream soon, in the next month or so), and having several gstreamer-gl-based plugins from gst-plugins-rs on Darwin will depend on #223248 and the SDK bump (which I've subscribed to the relevant issues and will remove the TODO items in gst-plugins-rs as things get merged)

Given you were the maintainer of the previous mopidy-spotify derivation, would you like to be a maintainer on this one as well @rski?

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/)
  • 23.05 Release Notes (or backporting 22.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
  • Fits CONTRIBUTING.md.
nixpkgs-review

Result of nixpkgs-review run on x86_64-linux 1

46 packages built:
  • gst_all_1.gst-plugins-rs
  • gst_all_1.gst-plugins-rs.dev
  • mopidy
  • mopidy-bandcamp
  • mopidy-bandcamp.dist
  • mopidy-iris
  • mopidy-iris.dist
  • mopidy-jellyfin
  • mopidy-jellyfin.dist
  • mopidy-local
  • mopidy-local.dist
  • mopidy-moped
  • mopidy-moped.dist
  • mopidy-mopify
  • mopidy-mopify.dist
  • mopidy-mpd
  • mopidy-mpd.dist
  • mopidy-mpris
  • mopidy-mpris.dist
  • mopidy-muse
  • mopidy-muse.dist
  • mopidy-musicbox-webclient
  • mopidy-musicbox-webclient.dist
  • mopidy-notify
  • mopidy-notify.dist
  • mopidy-podcast
  • mopidy-podcast.dist
  • mopidy-scrobbler
  • mopidy-scrobbler.dist
  • mopidy-somafm
  • mopidy-somafm.dist
  • mopidy-soundcloud
  • mopidy-soundcloud.dist
  • mopidy-spotify
  • mopidy-spotify.dist
  • mopidy-subidy
  • mopidy-subidy.dist
  • mopidy-tidal
  • mopidy-tidal.dist
  • mopidy-tunein
  • mopidy-tunein.dist
  • mopidy-youtube
  • mopidy-youtube.dist
  • mopidy-ytmusic
  • mopidy-ytmusic.dist
  • mopidy.dist

Result of nixpkgs-review run on aarch64-linux 1

46 packages built:
  • gst_all_1.gst-plugins-rs
  • gst_all_1.gst-plugins-rs.dev
  • mopidy
  • mopidy-bandcamp
  • mopidy-bandcamp.dist
  • mopidy-iris
  • mopidy-iris.dist
  • mopidy-jellyfin
  • mopidy-jellyfin.dist
  • mopidy-local
  • mopidy-local.dist
  • mopidy-moped
  • mopidy-moped.dist
  • mopidy-mopify
  • mopidy-mopify.dist
  • mopidy-mpd
  • mopidy-mpd.dist
  • mopidy-mpris
  • mopidy-mpris.dist
  • mopidy-muse
  • mopidy-muse.dist
  • mopidy-musicbox-webclient
  • mopidy-musicbox-webclient.dist
  • mopidy-notify
  • mopidy-notify.dist
  • mopidy-podcast
  • mopidy-podcast.dist
  • mopidy-scrobbler
  • mopidy-scrobbler.dist
  • mopidy-somafm
  • mopidy-somafm.dist
  • mopidy-soundcloud
  • mopidy-soundcloud.dist
  • mopidy-spotify
  • mopidy-spotify.dist
  • mopidy-subidy
  • mopidy-subidy.dist
  • mopidy-tidal
  • mopidy-tidal.dist
  • mopidy-tunein
  • mopidy-tunein.dist
  • mopidy-youtube
  • mopidy-youtube.dist
  • mopidy-ytmusic
  • mopidy-ytmusic.dist
  • mopidy.dist

Result of nixpkgs-review run on x86_64-darwin 1

46 packages built:
  • gst_all_1.gst-plugins-rs
  • gst_all_1.gst-plugins-rs.dev
  • mopidy
  • mopidy-bandcamp
  • mopidy-bandcamp.dist
  • mopidy-iris
  • mopidy-iris.dist
  • mopidy-jellyfin
  • mopidy-jellyfin.dist
  • mopidy-local
  • mopidy-local.dist
  • mopidy-moped
  • mopidy-moped.dist
  • mopidy-mopify
  • mopidy-mopify.dist
  • mopidy-mpd
  • mopidy-mpd.dist
  • mopidy-mpris
  • mopidy-mpris.dist
  • mopidy-muse
  • mopidy-muse.dist
  • mopidy-musicbox-webclient
  • mopidy-musicbox-webclient.dist
  • mopidy-notify
  • mopidy-notify.dist
  • mopidy-podcast
  • mopidy-podcast.dist
  • mopidy-scrobbler
  • mopidy-scrobbler.dist
  • mopidy-somafm
  • mopidy-somafm.dist
  • mopidy-soundcloud
  • mopidy-soundcloud.dist
  • mopidy-spotify
  • mopidy-spotify.dist
  • mopidy-subidy
  • mopidy-subidy.dist
  • mopidy-tidal
  • mopidy-tidal.dist
  • mopidy-tunein
  • mopidy-tunein.dist
  • mopidy-youtube
  • mopidy-youtube.dist
  • mopidy-ytmusic
  • mopidy-ytmusic.dist
  • mopidy.dist

Result of nixpkgs-review run on aarch64-darwin 1

46 packages built:
  • gst_all_1.gst-plugins-rs
  • gst_all_1.gst-plugins-rs.dev
  • mopidy
  • mopidy-bandcamp
  • mopidy-bandcamp.dist
  • mopidy-iris
  • mopidy-iris.dist
  • mopidy-jellyfin
  • mopidy-jellyfin.dist
  • mopidy-local
  • mopidy-local.dist
  • mopidy-moped
  • mopidy-moped.dist
  • mopidy-mopify
  • mopidy-mopify.dist
  • mopidy-mpd
  • mopidy-mpd.dist
  • mopidy-mpris
  • mopidy-mpris.dist
  • mopidy-muse
  • mopidy-muse.dist
  • mopidy-musicbox-webclient
  • mopidy-musicbox-webclient.dist
  • mopidy-notify
  • mopidy-notify.dist
  • mopidy-podcast
  • mopidy-podcast.dist
  • mopidy-scrobbler
  • mopidy-scrobbler.dist
  • mopidy-somafm
  • mopidy-somafm.dist
  • mopidy-soundcloud
  • mopidy-soundcloud.dist
  • mopidy-spotify
  • mopidy-spotify.dist
  • mopidy-subidy
  • mopidy-subidy.dist
  • mopidy-tidal
  • mopidy-tidal.dist
  • mopidy-tunein
  • mopidy-tunein.dist
  • mopidy-youtube
  • mopidy-youtube.dist
  • mopidy-ytmusic
  • mopidy-ytmusic.dist
  • mopidy.dist

@rski
Copy link
Contributor

rski commented Apr 7, 2023

Given you were the maintainer of the previous mopidy-spotify derivation, would you like to be a maintainer on this one as well @rski?

oh this is neat, and thanks for the ping. However I've switched to apple music in the last couple of months so

@lilyinstarlight
Copy link
Member Author

oh this is neat, and thanks for the ping. However I've switched to apple music in the last couple of months so

No worries then! Thought I'd at least check :)

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Apr 7, 2023
@lilyinstarlight lilyinstarlight changed the title gst-plugins-rs: init at 0.10.6, mopidy-spotify: re-init at unstable-2023-03-22 gst_all_1.gst-plugins-rs: init at 0.10.6, mopidy-spotify: re-init at unstable-2023-03-22 Apr 7, 2023
@lilyinstarlight
Copy link
Member Author

I made a push to update the timeout multiplier for checks because they can take a little more than a half hour on slower or busy systems it seems (I ran into it when testing x86_64-darwin through rosetta2)

@lilyinstarlight
Copy link
Member Author

I can't replicate that ofborg x86_64-linux error on any of my own machines

I've opened https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/issues/337 about it and worst case we can just temporarily disable that plugin

@risicle
Copy link
Contributor

risicle commented Apr 7, 2023

nixpkgs-review happy, macos 10.15 & nixos x86_64

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Apr 8, 2023

I've gotta rebase this for a merge conflict anyway, but I'm going to push a test commit first to attempt to get logs out of ofborg for why that one test is failing on x86_64-linux (since I can't seem to replicate it any longer on my aarch64 system 🤔)

@lilyinstarlight lilyinstarlight marked this pull request as draft April 8, 2023 11:57
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 8, 2023
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 8, 2023
@lilyinstarlight lilyinstarlight marked this pull request as ready for review April 8, 2023 12:35
@lilyinstarlight lilyinstarlight marked this pull request as draft April 8, 2023 16:31
@lilyinstarlight lilyinstarlight force-pushed the pkg/mopidy-spotify branch 2 times, most recently from 74b8a4f to 66f4f76 Compare April 8, 2023 17:30
@lilyinstarlight lilyinstarlight marked this pull request as ready for review April 8, 2023 17:30
@lilyinstarlight
Copy link
Member Author

Well, OfBorg isn't failing anymore and I could never reproduce that test failure myself, so I guess problem solved?

If it occurs again in the future (e.g. on Hydra or local builds), I'll be able to provide more data for upstream, but for now I guess that's out of scope for this PR

@lilyinstarlight
Copy link
Member Author

I just pushed an update to make sure gst_all_1.gst-plugins-rs always pulls the latest normal version rather than the gstreamer- tags when running the updateScript

@lilyinstarlight lilyinstarlight marked this pull request as draft April 14, 2023 19:49
@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Apr 14, 2023

@ofborg build gst_all_1.gst-plugins-rs

Apparently ofborg started failing again on x86_64-linux and while I was hoping to get cole to test stuff on the builder itself, I guess I'll just use this PR again to attempt to get debug logs for upstream (apologies for the noise for anyone still subscribed to this...)

@lilyinstarlight lilyinstarlight force-pushed the pkg/mopidy-spotify branch 3 times, most recently from d6c5ea8 to 12788fe Compare April 14, 2023 20:11
@lilyinstarlight lilyinstarlight mentioned this pull request Apr 16, 2023
12 tasks
@hacker1024
Copy link
Member

Can the main Rust derivation not be split up? This seems inconvenient for those who only want a few of the plugins, especially when cross-compiling from scratch due to the lack of binary caches.

See below for example what I've been using for gst-plugin-rs and gst-plugin-rtp.

{ fetchFromGitLab
, rustPlatform
, pkg-config
, wrapGAppsHook
, openssl
, glib
, gst_all_1
}:

rustPlatform.buildRustPackage rec {
  pname = "gst-plugin-webrtc";
  version = "0.10.2";

  src = fetchFromGitLab {
    domain = "gitlab.freedesktop.org";
    owner = "gstreamer";
    repo = "gst-plugins-rs";
    rev = version;
    hash = "sha256-bMqun2uFd32sG2wKT9WrACy5Qxu5jKndcUUl997Di7o=";
  };

  buildAndTestSubdir = "net/webrtc";

  cargoHash = "sha256-TdzK6anh2pnIdx7bfSE43zI3gh6iQAgi3qWEIS96J/4=";

  nativeBuildInputs = [ pkg-config wrapGAppsHook ];
  buildInputs = [
    openssl
    glib
    gst_all_1.gstreamer
    gst_all_1.gst-plugins-base
    gst_all_1.gst-plugins-bad
  ];

  postInstall = ''
    mkdir $out/lib/gstreamer-1.0
    mv $out/lib/*.so $out/lib/gstreamer-1.0
  '';
}
{ fetchFromGitLab
, rustPlatform
, pkg-config
, wrapGAppsHook
, glib
, gst_all_1
}:

rustPlatform.buildRustPackage rec {
  pname = "gst-plugin-rtp";
  version = "0.10.2";

  src = fetchFromGitLab {
    domain = "gitlab.freedesktop.org";
    owner = "gstreamer";
    repo = "gst-plugins-rs";
    rev = version;
    hash = "sha256-bMqun2uFd32sG2wKT9WrACy5Qxu5jKndcUUl997Di7o=";
  };

  buildAndTestSubdir = "net/rtp";

  cargoHash = "sha256-HMEJHyqT6xsEx+NT/8H7atRvO0q5QcrPt3Ta04OZqHk=";

  nativeBuildInputs = [ pkg-config wrapGAppsHook ];
  buildInputs = [
    glib
    gst_all_1.gstreamer
    gst_all_1.gst-plugins-base
  ];

  postInstall = ''
    mkdir $out/lib/gstreamer-1.0
    mv $out/lib/*.so $out/lib/gstreamer-1.0
  '';
}

@lilyinstarlight
Copy link
Member Author

Can the main Rust derivation not be split up? This seems inconvenient for those who only want a few of the plugins, especially when cross-compiling from scratch due to the lack of binary caches.

That could probably be done, but some plugins rely on the meson logic and the use of cargo-c so it would be better to use those instead of buildRustPackage

I'll see if I can add an argument to specify a list of plugins to enable and otherwise enable all by default (e.g. so you could use gst_all_1.gst-plugins-rs.override { plugins = [ "rtp" "webrtc" ]; } for when you need plugins like you currently have now). Would that be acceptable?

@lilyinstarlight
Copy link
Member Author

@hacker1024 It took an upstream MR to fix a minor bug in the meson scripts (https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/1184) but I have a small change to the package now to allow building only a specific list of plugins in the general case

If overriding to select a different plugin list is acceptable to you (e.g. gst_all_1.gst-plugins-rs.override { plugins = [ "rtp" "webrtc" ]; }, which you could even overlay if you wanted for a cross system), then I'll push it to this PR. Otherwise I feel like it's probably best handled in a follow-up since breaking up the gst-plugins-rs package really wouldn't make much sense for the majority of users (unless, like the example you gave, they lack binary caches due to cross)

@hacker1024
Copy link
Member

The override solution is suitable, thanks.

It's still not quite as useful as split derivations (each change to the plugin list will require building all of them), but that's not too bad.

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Apr 18, 2023

The override solution is suitable, thanks.

Awesome! Thank you :)

It's still not quite as useful as split derivations (each change to the plugin list will require building all of them), but that's not too bad.

You can still do gst-plugins-rswebrtc = gst-plugins-rs.override { plugins = [ "webrtc" ]; } if you would really like for each individual plugin, since this allows for general composition (I'm just not sure that's useful by default for most users)

@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented Apr 21, 2023

I just made a push with some changes to better accomodate cross in gst-plugins-rs and to not pull in unnecessary deps when specifying only a limited subset of plugins (so that cross can be less painful in not having to build gtk4 and all that entails if it doesn't have to)

Cross needs #227135 for the csound plugin specifically but it should otherwise be fine now

@lilyinstarlight lilyinstarlight force-pushed the pkg/mopidy-spotify branch 6 times, most recently from e45454f to 43ecf29 Compare April 23, 2023 10:38
@masaeedu
Copy link
Contributor

@lilyinstarlight Don't have anything useful to contribute here (I barely understand most of the issues you outlined above), just wanted to express my gratitude for the heroic effort you're putting in.

@lilyinstarlight lilyinstarlight changed the title gst_all_1.gst-plugins-rs: init at 0.10.6, mopidy-spotify: re-init at unstable-2023-03-22 gst_all_1.gst-plugins-rs: init at 0.10.7, mopidy-spotify: re-init at unstable-2023-04-21 May 15, 2023
@SuperSandro2000
Copy link
Member

darwin timed out, lets hope it works on hydra

@SuperSandro2000 SuperSandro2000 merged commit 235373f into NixOS:master May 17, 2023
@lilyinstarlight
Copy link
Member Author

lilyinstarlight commented May 17, 2023

darwin timed out, lets hope it works on hydra

It does build on the darwin build box and also built on the ofborg builders previously (until I made it respect NUM_JOBS, after which it started timing out in ofborg)

So hopefully there aren't new issues that pop up on Hydra, but Darwin never fails to surprise, so I'll watch it during the next eval

Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants