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

mediatomb: Improve service + add gerbera support and tests #93450

Merged
merged 9 commits into from Oct 8, 2020

Conversation

@ardumont
Copy link
Contributor

@ardumont ardumont commented Jul 19, 2020

Motivations

  • Allow service to run with the most recent maintained mediatomb fork (gerbera)
  • Add more configuration options (e.g. media directories to autoscan, ...)
  • Leverage options to make the configuration lazy
  • Fix transcoding option which if enabled installs the necessary dependencies to actually work
  • Add tests to the service (there was none), testing scenario are:
    1. service runs with existing mediatomb package
    2. service runs with the new gerbera package
  • Add types to existing service options which did not declare one
  • Open a new option openFirewall (defaults to false) and improve the firewall declaration using it.
  • Add release note (20.09) information about new options and breaking change (openFirewall off by default now)

Actions

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (Linux alderaan 5.7.7 #1-NixOS SMP Tue Jun 30 20:21:22 UTC 2020 x86_64 GNU/Linux)
    • macOS
    • other Linux distributions: debian with nix (Linux yavin4 4.19.0-9-amd64 #1 SMP Debian 4.19.118-2+deb10u1 (2020-06-07) x86_64 GNU/Linux)
  • Added tests as none existed
  • Tested compilation of all pkgs that depend on this: nixpkgs-review pr 93450
  • 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.

Notes

I built the diff on the started works [1] [2]

[1] #82429 (apparently idle, asked but got no reply so i continued from it)

[2] #93226 (my own work about gerbera, merged now)

[4]

$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/93450/head:refs/nixpkgs-review/1
$ git worktree add /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs 9fab6f9b2b2d8927d3984a7549ed4c56829d2065
Preparing worktree (detached HEAD 9fab6f9b2b2)
Updating files: 100% (22851/22851), done.
HEAD is now at 9fab6f9b2b2 Merge pull request #98823 from r-ryantm/auto-update/multimon-ng
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit 2c4cf27d61b07dfbac3ffa73658471c8180eb972
Automatic merge went well; stopped before committing as requested
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs -qaP --xml --out-path --show-trace --meta
Nothing to be built.
https://github.com/NixOS/nixpkgs/pull/93450
$ nix-shell /home/tony/.cache/nixpkgs-review/pr-93450-4/shell.nixnixpkgs-review pr 93450
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/93450/head:refs/nixpkgs-review/1
$ git worktree add /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs 9fab6f9b2b2d8927d3984a7549ed4c56829d2065
Preparing worktree (detached HEAD 9fab6f9b2b2)
Updating files: 100% (22851/22851), done.
HEAD is now at 9fab6f9b2b2 Merge pull request #98823 from r-ryantm/auto-update/multimon-ng
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit 2c4cf27d61b07dfbac3ffa73658471c8180eb972
Automatic merge went well; stopped before committing as requested
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93450-4/nixpkgs -qaP --xml --out-path --show-trace --meta
Nothing to be built.
https://github.com/NixOS/nixpkgs/pull/93450
$ nix-shell /home/tony/.cache/nixpkgs-review/pr-93450-4/shell.nix

Cheers,

@ardumont ardumont force-pushed the gerbera-service branch from 77ae356 to 211b225 Jul 19, 2020
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 19, 2020

I just:

  • fixed a flipped flag option to keep its existing value
  • clarified a docstring

Copy link
Member

@aanderse aanderse left a comment

Here are some initial comments. I think I need more time to do a thorough review. Will circle back to this in the next few days hopefully.

nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix Show resolved Hide resolved
nixos/modules/services/misc/mediatomb.nix Show resolved Hide resolved
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 19, 2020

Here are some initial comments. I think I need more time to do a thorough review. Will circle back to this in the next few days hopefully.

Awesome, thanks.
Will have a look and try to my best to answer the concerns and fix accordingly those ;)

Cheers,

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 19, 2020

(NdA: PR description editing)

[x] Added tests as none existed (note that they pass when run interactively thoughnix-build mediatomb.nix -A driver && ./result/bin/nixos-test-driver)

I can run tests now (cd nixos/tests; nix-build mediatomb.nix) when proper rights are set to /dev/kvm (most probably issue with nix within debian... i'm using kvm from debian and not the nix one because i don't recall being able to do otherwise...)
Anyway, amending the description now ;)

@ardumont ardumont force-pushed the gerbera-service branch from 7e674dd to 762af6d Jul 20, 2020
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 20, 2020

All checks have failed

lol, 1 check failed... In that regards, "All" sounds a bit harsh ;)
Anyway, right, I missed a toString call within a docstring... (somehow tests
are not annoyed by this ¯_(ツ)_/¯)

ardumont force-pushed the ardumont:gerbera-service branch from 7e674dd to 762af6d 2 minutes ago

This is fixed now.

@ardumont ardumont requested a review from aanderse Jul 21, 2020
Copy link
Member

@aanderse aanderse left a comment

Sorry I'm going to need some more time to review this. I'm hoping someone else can jump in and offer some review as well because it has been too many years since I've used mediatomb to be the only person reviewing this.

default = "";
description = ''
A specific interface to bind to.
'';
};

openFirewall = mkOption {
type = types.bool;
default = true; # On by default for retro-compatibility with existing behavior
Copy link
Member

@aanderse aanderse Jul 22, 2020

Hmm... we may want to check on this. I can't recall the details. I think we're supposed to have all ports closed unless explicitly allowed by the sysadmin. If so an entry in the release notes would suffice here.

Copy link
Contributor Author

@ardumont ardumont Jul 23, 2020

Yes, i recall a discussion (github issue) about services should stop opening firewall themselves... (let me search for it... found a rule in a doc instead [1])

I specifically left it open here because it was the existing behavior i did not want to break (thus the comment so reviewer know my intent). If "breaking the existing behavior" is acceptable with an update in the release note though (i don't really know what that is yet ;) then i'll adapt accordingly.

[1] https://nixos.org/nixpkgs/manual/#reviewing-contributions-new-modules (it's for a new module though ¯_(ツ)_/¯)

Thanks again

Copy link
Contributor Author

@ardumont ardumont Jul 24, 2020

So, the option is false by default now. And I added a note in the release-note (20.09) in the breaking-compatibility section about it.

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 23, 2020

Hello,

Sorry I'm going to need some more time to review this.

Well, sure ;)

For information, I separated logically the commits so the review is easier one
by one. I don't really know how to simplify in another way.

I'm hoping someone else can jump in and offer some review as well because it
has been too many years since I've used mediatomb to be the only person
reviewing this.

Hopefully, yes. That'd be great, the more eyes to look, the more improvments we can do ;)

In the mean time, do you have some time to review the less impactful diffs (the
ones the diff is built upon here)?

  • #93226 (gerbera init at 1.5.0)
  • #93048 (libupnp 1.12 -> 1.12.1)

... to be the one reviewing this...

Yes, that's why I opened tests around this. After touching so much part on
this, I wanted to prove I did not break anything both to the reviewers and me.
(Personally, I'm confident now ;)

So in those tests, I'm making sure both old and new mediatomb service still
starts.

Note that I did not add all the possible scenarios though. I was a bit wary of
the time too many checks would take.

I could still improve the testing, so we actually check:

  • some part of the generated configuration (the parts that makes sense, for
    example the mediaDirectories, transcoding, ...).

  • one "pattern" we can grep for in the service logs "Configuration check
    succeeded." This ones ensures the service will start (when it fails, the
    service is in failed state).

  • "The Web UI can be reached by following this link:
    http://${interface}:${toString port}/" as well [1]. This ones actually is
    dependent on the declaration about interface and port so i'd say it's
    relevant.

[1] web ui is hard-coded to on by default. I did not open an option for it
because, well, the "diff" is long already...

Cheers,

@ardumont ardumont force-pushed the gerbera-service branch from 762af6d to 6633914 Jul 23, 2020
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Jul 23, 2020

@ardumont mediatomb: Improve firewall rules + open firewall option (off by defa… … 6633914

Note to self, look into adding a release note [1] to explicit the firewall rule option change [2]

[1] nixpkgs/nixos/doc/manual/release-notes/rl-2009.xml <- in that file probably

[2] as of this commit, the service no longer opens the firewall rules by default, this needs an explicit user declaration openFirewall = true;

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Aug 1, 2020

Do I need to rebase and fix conflicts now?

@aanderse
Copy link
Member

@aanderse aanderse commented Aug 1, 2020

@ardumont sure. We really should find someone who uses mediatomb to test and review this...

@ardumont ardumont force-pushed the gerbera-service branch from c5dca04 to 066fb9d Aug 2, 2020
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Aug 2, 2020

@ardumont sure.

done.
everything is still fine (build and tests).

We really should find someone who uses mediatomb to test and review this...

It'd be neat because since the release notes got updated, I gather conflicts will happen more often...

I note you are still noted as requesting changes. But as far as I know and remember, I addressed all your points, didn't I?

;)

Copy link
Member

@aanderse aanderse left a comment

Have we found anyone to review this yet? Did you post on https://discourse.nixos.org/?

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Aug 21, 2020

Have we found anyone to review this yet? Did you post on https://discourse.nixos.org/?

No, I did not know i'd need to do something and don't know how to actually search for someone to review.
(Also, i was offline a bit ;)

Cheers,

ardumont added 3 commits Oct 8, 2020
This exposes 2 scenario running the mediatomb service:
- one running with the unmaintained mediatomb package
- one running with the new maintained gerbera package
This changes the default behavior which opened by default the firewall rules.
The users now need to declare explicitely they want to open the firewall.
Note that it made into 2 entries, one about new options in the first section.
Another in the breaking compatibility section due to the openFirewall option
which changes the behavior.
@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Oct 8, 2020

ofBorg eval fails:
undefined variable 'serverName' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/packet-spot-eval-2/nixos/modules/services/misc/mediatomb.nix:264:33

Thanks and sorry, I was sleeping...

I don't understand why I don't reproduce this... When running either the build
or the tests locally (because I did run them ;) [1]

It's not clear to me why {name} evals but {serverName} does not. I have
added a {cfg.serverName} and pushed as a tryout.

[1] Then again, I don't understand either why I have the following error locally
(fairly recent) and this does not show up in the output build in the first
place:

error: Alias spidermonkey is still in all-packages.nix
(use '--show-trace' to show detailed location information)

To work around this ^ (which i know is not so great ¯_(ツ)_/¯), I have locally
removed spidermonkey from the buildInputs from the mediatomb derivation
itself (and then build and tests are no longer blocked).

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Oct 8, 2020

It's not clear to me why {name} evals but {serverName} does not. I have
added a {cfg.serverName} and pushed as a tryout.

It works ;)

I'm still unclear on the other part though [1]

@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

ofBorg eval fails:
undefined variable 'serverName' at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/packet-spot-eval-2/nixos/modules/services/misc/mediatomb.nix:264:33

Thanks and sorry, I was sleeping...

Absolutely no need to apologize :) The iteration speed here is already breakneck for open source standards ;)

I don't understand why I don't reproduce this... When running either the build
or the tests locally (because I did run them ;) [1]

Yeah this is a bit confusing at first: Its not about the build, but the evaluation. ofBorg tries to evaluate all of nixpkgs, while the simple build only lazily evaluates whatever it needs.

To avoid these issues, I have this hook in my nixpkgs/.git/hooks/pre-push:

#!/bin/sh
echo "Checking evaluation";
exec nix-env -f . -qaP --meta --json --drv-path --show-trace >/dev/null

I think its not quite the same ofBorg does, but it usually catches evaluation errors.

It's not clear to me why {name} evals but {serverName} does not. I have
added a {cfg.serverName} and pushed as a tryout.

Looks like that worked.

[1] Then again, I don't understand either why I have the following error locally
(fairly recent) and this does not show up in the output build in the first
place:

error: Alias spidermonkey is still in all-packages.nix
(use '--show-trace' to show detailed location information)

To work around this ^ (which i know is not so great ¯_(ツ)_/¯), I have locally
removed spidermonkey from the buildInputs from the mediatomb derivation
itself (and then build and tests are no longer blocked).

This is because spidermonkey is defined in both pkgs/top-level/aliases.nix (as an alias to spidermonkey_68) and pkgs/top-level/all-packages.nix (as an alias to spidermonkey_38). I'm not sure what is going on there. As far as I'm aware, aliases.nix is for name aliases (black = python3.pkgs.black) while version aliases spidermonkey = spidermonkey_68 are usually kept in all-packages.

That knowledge might be outdated, but either way one of the aliases should be removed.

This doesn't show up in ofBorg because this actually is a build error, not an eval error (and it didn't try to build mediatomb here).

@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

spidermonkey was added to aliases.nix in 946369a, as an afterthought while deprecating spidermonkey_38. As a fix, I suggest you remove spidermonkey from aliases.nix and instead update the all-packages.nix version to spidermonkey_68. Its not really your bug and unrelated to this PR, so you can also leave it be if you want. But why not fix it while we're at it.

(CC @mkg20001)

ardumont added a commit to ardumont/nixpkgs that referenced this issue Oct 8, 2020
It's already aliased in all-packages.

This creates issues at least for the mediatomb build.

Related to NixOS#93450#issuecomment-705408544
@ardumont ardumont mentioned this pull request Oct 8, 2020
10 tasks
timokau
timokau approved these changes Oct 8, 2020
@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

Thank you for your patience and your diligence here 🚀

@timokau timokau merged commit 19ac436 into NixOS:master Oct 8, 2020
17 checks passed
default = "/var/lib/mediatomb";
default = "/var/lib/${name}";
description = ''
The directory where ${cfg.serverName} stores its state, data, etc.
Copy link
Member

@timokau timokau Oct 8, 2020

Unfortunately I just noticed this post-merge. How can you refer to the value of an option within an option description? Have you tried building the docs with this? I suspect this will just always expand to the default value.

default = false;
description = ''
If false (the default), this is up to the user to declare the firewall rules.
If true, this opens the 1900 (tcp and udp) and ${toString cfg.port} (tcp) ports.
Copy link
Member

@timokau timokau Oct 8, 2020

Same here: Reference to an option value within an option description.

customCfg = mkOption {
type = types.bool;
default = false;
description = ''
Allow mediatomb to create and use its own config file inside ${cfg.dataDir}.
Allow ${name} to create and use its own config file inside ${cfg.dataDir}.
Copy link
Member

@timokau timokau Oct 8, 2020

Also here. You should probably refer to the options by name instead.

@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

It seems like this breaks the manual build. I'm working on a fix.

@timokau timokau mentioned this pull request Oct 8, 2020
10 tasks
timokau added a commit to timokau/nixpkgs that referenced this issue Oct 8, 2020
Follow-up to NixOS#93450 to fix the manual build.
timokau added a commit to timokau/nixpkgs that referenced this issue Oct 8, 2020
Follow-up to NixOS#93450 to fix the manual build.
@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Oct 8, 2020

Thank you for your patience and your diligence here

Well, thanks for your input on this ;)

How can you refer to the value of an option within an option description?

I actually saw this in other derivations (I think).

Have you tried building the docs with this?

I thought this was covered by the actual build process.
So I probably did not then, sorry ¯_(ツ)_/¯.

How does one build the documentation? Is that [1]?

[1] https://nixos.org/manual/nixpkgs/stable/#chap-contributing

Reference to an option value within an option description.

Ack, I'll stop doing that then.

Cheers,

@ardumont ardumont deleted the gerbera-service branch Oct 8, 2020
@mkg20001
Copy link
Member

@mkg20001 mkg20001 commented Oct 8, 2020

@timokau What do you mean by updating spidermonkey to spidermonkey_68 in all-packages.nix? Should I move the alias there? Or simply drop spidermonkey alias for good?

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Oct 8, 2020

@mkg20001 Hello, i'm pretty sure @timokau meant #100026 since that PR landed ;)

Cheers,

@mkg20001
Copy link
Member

@mkg20001 mkg20001 commented Oct 8, 2020

ah ok, thx

@timokau
Copy link
Member

@timokau timokau commented Oct 8, 2020

Have you tried building the docs with this?

I thought this was covered by the actual build process.
So I probably did not then, sorry ¯_(ツ)_/¯.

No worries. If anything, its a failure of automated testing and a weird doc format (hopefully not much longer, #88488 and NixOS/rfcs#72).

How does one build the documentation? Is that [1]?

[1] https://nixos.org/manual/nixpkgs/stable/#chap-contributing

I built the manpages like this: cd nixos; nix-build release.nix --attr manpage. But the way its documented probably also works.

@worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Oct 11, 2020

Hi, so this touched the 20.09 release notes on master but I don't think this change is going to be backported. I'm going to revert the release note entirely for now as it causes conflicts on release-20.09.

@timokau
Copy link
Member

@timokau timokau commented Oct 12, 2020

Sorry for missing that in the review, I didn't check the filename. @ardumont if you want to re-add it, you can add it to the 21.03 release notes since it will be included in that release.

@ardumont
Copy link
Contributor Author

@ardumont ardumont commented Oct 12, 2020

Sorry for missing that in the review, I didn't check the filename.

Well, i did not realize I was targetting the wrong release, so my bad ;)

@ardumont if you want to re-add it, you can add it to the 21.03 release notes since it will be included in that release.

ok, i'll have a look later in the day.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment