Bug 194959 - multimedia/mediabrowser few suggestions
Summary: multimedia/mediabrowser few suggestions
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Adam Weinberger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-12 06:09 UTC by joshruehlig
Modified: 2014-11-13 21:37 UTC (History)
2 users (show)

See Also:
joshruehlig: maintainer-feedback? (woodsb02)


Attachments
Patch to mediabrowser port (2.34 KB, patch)
2014-11-12 06:10 UTC, joshruehlig
no flags Details | Diff
Patch to plexmediaserver port (2.30 KB, patch)
2014-11-12 06:12 UTC, joshruehlig
no flags Details | Diff
Patch to plexmediaserver port (2.40 KB, patch)
2014-11-12 06:19 UTC, joshruehlig
no flags Details | Diff
Patch to plexmediaserver port (2.42 KB, patch)
2014-11-12 06:26 UTC, joshruehlig
no flags Details | Diff
Patch to mediabrowser port (2.53 KB, patch)
2014-11-12 07:01 UTC, joshruehlig
no flags Details | Diff
PR194959: improvements to mediabrowser port (with maintainer edits) (3.70 KB, patch)
2014-11-13 16:08 UTC, Ben Woods
no flags Details | Diff
PR194959: improvements to mediabrowser port (with maintainer edits) (3.20 KB, patch)
2014-11-13 16:23 UTC, Ben Woods
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description joshruehlig 2014-11-12 06:09:58 UTC
The attached patch does the following..

* add ability to change user mediabrowser runs as instead of hardcoding it. This has the init system drop the user privileges instead of relying on daemon. This is also the reason I had to initialize the pidfile

* remove setting LC_ALL and LANG, is this needed?

* suggest using sysrc instead of echoing to /etc/rc.conf which is less fool proof (I just learned about this cool tool =] )
Comment 1 Bugzilla Automation freebsd_committer 2014-11-12 06:09:58 UTC
Maintainer CC'd
Comment 2 joshruehlig 2014-11-12 06:10:48 UTC
Created attachment 149308 [details]
Patch to mediabrowser port
Comment 3 joshruehlig 2014-11-12 06:12:42 UTC
Created attachment 149310 [details]
Patch to plexmediaserver port
Comment 4 joshruehlig 2014-11-12 06:19:29 UTC
Created attachment 149311 [details]
Patch to plexmediaserver port

keep fixing typos I made =/
Comment 5 Ben Woods freebsd_committer 2014-11-12 06:23:29 UTC
The reason I set LC_ALL and LANG is that without these being set (as per my jail that mediabrowser is running on), mediabrowser gets a System.ArgumentNullException (handled by a try/catch, but seen if you run with mono --trace=N:nothing).

The code that is failing was get_posix_locale was returning NULL in mono/mono/metadata/locales.c, which was caught in mcs/class/corlib/System.Globalization/CultureInfo.cs.
Comment 6 joshruehlig 2014-11-12 06:24:17 UTC
Lol, k way over my head. Guess we shouldn't remove them then =]
Comment 7 Ben Woods freebsd_committer 2014-11-12 06:26:33 UTC
I guess the LC_ALL and LANG settings should first check if they are already set, and only set them to "C" if they are not (do not override them if already set)
Comment 8 joshruehlig 2014-11-12 06:26:35 UTC
Created attachment 149312 [details]
Patch to plexmediaserver port

fixed
Comment 9 Ben Woods freebsd_committer 2014-11-12 06:27:38 UTC
Also... stop calling it plexmediaserver in your patch title ;)
Comment 10 joshruehlig 2014-11-12 06:29:06 UTC
Whoops, I just kept clicking the auto-suggestion in my browser... I guess it's getting too late for me..
Comment 11 Ben Woods freebsd_committer 2014-11-12 06:29:51 UTC
Also, I deliberate didn't provide the flexibility to set user/group, since the FreeBSD UIDs and GIDs files have been updated to define the user/group mediabrowser, and that gets installed when the package is installed. Given that, do you still think we should provide this flexibility? Why would anyone need it?
Comment 12 Ben Woods freebsd_committer 2014-11-12 06:32:27 UTC
Also, is there something wrong with relying on daemon to run mono with a specific user? It avoids having to initialise the pid file, and ensures the pid file is cleaned up (deleted) when the programs stops.

Using your method, the pid file will continue to sit there even after the service has stopped (presumably with the old pid number in it?)
Comment 13 joshruehlig 2014-11-12 06:35:13 UTC
Yes, this is standard; users may want to run certain daemons as certain users.

Even if you don't put the hints there a FreeBSD user could set mediabrowser_user and it would break things. the init system would try to run "command" as that user, which would fail because whatever user they set can't run "daemon" that forks to the mediabrowser user.
Comment 14 joshruehlig 2014-11-12 06:38:08 UTC
True that the pidfile doesn't get cleaned. Are you saying daemon as root does clean the pidfile?

Either way, this seems like a minor problem compared to not using the standard %%PORTNAME%%_user method built into the init system.
Comment 15 Ben Woods freebsd_committer 2014-11-12 06:43:28 UTC
Yes, using daemon does clean the pid file when the service is stopped.
Also, in your patch the pid file is created with ownership of mediabrowser_user the first time the user runs the service.
If they try and run as a different user after that, it will fail since they won't have write access to the pid file. Can the script be made to be flexible to that (I guess we can define a stop_postcmd that cleans up (deletes) the pidfile... that will fix both of these things).
Comment 16 joshruehlig 2014-11-12 06:52:59 UTC
we could either take it out of the "if" so the pidfile is always reinitialized with the correct user.

or, cleanup the pidfile in a postcmd.

If we don't do anything and a FreeBSD user did change the user mediabrowser ran as, (and they'd need to change the ownership of the data_dir). They'd encounter this message, which I think they could figure out.

daemon: pidfile ``/var/run/mediabrowser.pid'': Permission denied
/usr/local/etc/rc.d/mediabrowser: WARNING: failed to start mediabrowser

####

I think it'd be fine to clean the pidfile, or not.
Comment 17 Ben Woods freebsd_committer 2014-11-12 06:54:21 UTC
Lets clean it up, its... cleaner...
Comment 18 joshruehlig 2014-11-12 06:58:58 UTC
Lol, k.

I'll see what I can cook up real quick. After that going to bed so feel free to work on it or I can work on it tomorrow.
Comment 19 joshruehlig 2014-11-12 07:01:23 UTC
Created attachment 149313 [details]
Patch to mediabrowser port

hopefully the last one, lol
Comment 20 joshruehlig 2014-11-12 14:25:44 UTC
another alternative is we create the directory /var/run/mediabrowser owned by mediabrowser_user. I think this would allow daemon to clean the pid but an empty folder would be left instead.

see net-p2p/btsync as an example.
Comment 21 Ben Woods freebsd_committer 2014-11-12 14:29:17 UTC
I quite like the way we have it at the moment. Sure it means having a start_precmd and a stop_postcmd, but its quite neat.
Comment 22 joshruehlig 2014-11-12 14:31:53 UTC
k, either way works. just giving an alternative.
Comment 23 Ben Woods freebsd_committer 2014-11-13 16:08:00 UTC
Created attachment 149380 [details]
PR194959: improvements to mediabrowser port (with maintainer edits)

I have made some minor edits to these suggested improvements.
Maintainer approval granted to commit (this version of the patch).
Comment 24 Adam Weinberger freebsd_committer 2014-11-13 16:16:54 UTC
Okay. I'll commit this later today.

Personally I have reservations about changing to "sysrc". Most ports, and the FreeBSD Handbook, just say "add 'foo_enable="YES"' to /etc/rc.conf."

sysrc is only part of 9.2+. 9.0 is supported until 2016/12/31, and 9.1 is supported until 2014/12/31.

Your call, Ben.
Comment 25 Ben Woods freebsd_committer 2014-11-13 16:23:30 UTC
Created attachment 149381 [details]
PR194959: improvements to mediabrowser port (with maintainer edits)

Thanks for the heads up Adam. I have removed the change to pkg-message that mentioned sysrc, in favour of having accurate comments on all supported versions of FreeBSD.
Comment 26 Adam Weinberger freebsd_committer 2014-11-13 21:37:19 UTC
Committed. Thanks, Josh and Ben.
Comment 27 commit-hook freebsd_committer 2014-11-13 21:37:55 UTC
A commit references this bug:

Author: adamw
Date: Thu Nov 13 21:37:06 UTC 2014
New revision: 372539
URL: https://svnweb.freebsd.org/changeset/ports/372539

Log:
  - Allow overriding the user/group as which the service is run
  - Don't clobber LC_ALL/LANG
  - Add a deinstallation message

  PR:		194959
  Submitted by:	joshruehlig@gmail.com / maintainer
  Approved by:	Ben Woods (maintainer)

Changes:
  head/multimedia/mediabrowser/files/mediabrowser.in
  head/multimedia/mediabrowser/pkg-deinstall