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

Jump to bottom

Init gerbera package and reuse mediatomb service #82429

Closed
wants to merge 2 commits into from
Closed

Init gerbera package and reuse mediatomb service #82429

wants to merge 2 commits into from

Conversation

edwtjo
Copy link
Member

@edwtjo edwtjo commented Mar 12, 2020

Motivation for this change

Mediatomb replacement package which is actively maintained. For now just uses the old mediatomb service since the packages are, from a process level viewpoint, interchangeable.

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

@edwtjo
gerbera: init at 1.4.0
c156365
@edwtjo
nixos/mediatomb: make compatible with gerbera
4cf30b1 
The duplication of the interface xml tag is needed for
the daemon to respect the setting.
aanderse
aanderse requested changes Mar 12, 2020
View reviewed changes
nixos/modules/services/misc/mediatomb.nix
path = [ pkgs.mediatomb ];
serviceConfig.ExecStart = "${pkgs.mediatomb}/bin/mediatomb -p ${toString cfg.port} ${if cfg.interface!="" then "-e ${cfg.interface}" else ""} ${if cfg.customCfg then "" else "-c ${mtConf}"} -m ${cfg.dataDir}";
path = [ pkg ];
serviceConfig.ExecStart = "${pkg}/bin/${name} -p ${toString cfg.port} ${if cfg.interface!="" then "-e ${cfg.interface}" else ""} ${if cfg.customCfg then "" else "-c ${mtConf}"} -m ${cfg.dataDir}";
Copy link
Member

Choose a reason for hiding this comment

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

if then else "" -> optionalString would be nice.

nixos/modules/services/misc/mediatomb.nix
@@ -257,12 +270,12 @@ in {
###### implementation

config = mkIf cfg.enable {
systemd.services.mediatomb = {
description = "MediaTomb media Server";
systemd.services."${name}"= {
Copy link
Member

Choose a reason for hiding this comment

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

Given the module is called mediatomb this might cause some confusion. It might make more sense to continue to call the systemd service mediatomb and then add a gerbera alias.

nixos/modules/services/misc/mediatomb.nix
@@ -206,20 +219,20 @@ in {

dataDir = mkOption {
type = types.path;
default = "/var/lib/mediatomb";
default = "/var/lib/" + name;
Copy link
Member

Choose a reason for hiding this comment

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

This will cause existing installations to mysteriously break. Either leaving the default to mediatomb or explicitly mentioning this in release notes is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, well this was the intention since it has to be done sooner or later. I'll add it to the release notes to ease the transition.

nixos/modules/services/misc/mediatomb.nix
after = [ "network.target" ];
wantedBy = [ "multi-user.target" ];
path = [ pkgs.mediatomb ];
serviceConfig.ExecStart = "${pkgs.mediatomb}/bin/mediatomb -p ${toString cfg.port} ${if cfg.interface!="" then "-e ${cfg.interface}" else ""} ${if cfg.customCfg then "" else "-c ${mtConf}"} -m ${cfg.dataDir}";
path = [ pkg ];
Copy link
Member

Choose a reason for hiding this comment

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

Is this line actually required?

Copy link
Contributor

Choose a reason for hiding this comment

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

I gather it's not since the command line specifies the full path to the binary...
We'll see ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

As expected, it works without it ;)

pkgs/servers/gerbera/default.nix Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix
description = ''
How to identify the server on the network.
'';
};

package = mkOption {
type = types.package;
example = literalExample "pkgs.mediatomb";
Copy link
Member

Choose a reason for hiding this comment

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

Given the wide range of options available it might be more valuable to show an example with an override.

@aanderse aanderse mentioned this pull request Jul 16, 2020
Merged
10 tasks
@ardumont
Copy link
Contributor

ardumont commented Jul 17, 2020 •
edited

Hello,

Do you intend to continue and finish this? Or shall I pick it up?

I started a similar work taking another approach. I did not reuse the existing mediatomb service as I don't really know what's common between gerbera and mediatomb. I generated a new conf out of the latest gerbera 1.5.0 and built from this [1]

Now I realize it's not drastically different than current mediatomb service though, main difference is I opened:

  • a mediaDirectories option to allow the media directories to be inlined in the configuration
  • the transcoding option stops hard-coding values to something unresolvable in nix/nixos
  • if transcoding option to no, then the configuration about it is mostly empty
  • if transcoding option to yes, well that pulls the necessary dependencies and declare those so it's actually runnable
  • prefer long flag option instead of short ones in the systemd command so we can understand immediately what the command does (-m -> --home, -e -> --interface...) when looking into systemctl status
  • etc...

I gather this could be integrated in the original mediatomb service as you did here.

I also intend to extend the currently limited import js instructions.
So users can declare their own in their service configuration (as a next step).
It's my understand that to allow better display of media directories within gerbera client (tv for example), one has to do some javascript (<- ikr?).

[1] https://gitlab.com/ardumont/nixos/-/blob/master/odroid/gerbera/service.nix

Cheers,

@ardumont ardumont mentioned this pull request Jul 19, 2020
Merged
10 tasks
@ardumont
Copy link
Contributor

ardumont commented Oct 8, 2020 •
edited

Heads up, this can be closed now as #93450 which supersedes it has been merged.

Again, I reused some of the work here, thanks!
I amended the commit which bootstraps the mediatomb service improvment to allow the use of gerbera.
I improved according to the improvments suggested here. And then I iterated over my proposed improvments
as detailed in the previous comment.

Cheers,

@timokau timokau closed this Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardumont ardumont ardumont left review comments

@aanderse aanderse aanderse requested changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@edwtjo @ardumont @aanderse @timokau