Created attachment 171236 [details] patch The patch is from the github pull request https://github.com/freebsd/freebsd-ports/pull/19
Looks fine, please go ahead and commit. Thanks!
@mat, Forget about this?
This is not mine, I just moved the github issue to a PR.
(In reply to Mathieu Arnold from comment #3) (In reply to Richard Gallamore from comment #2) +1 please add this
Created attachment 185195 [details] svnlite diff svn diff instead of git diff mail
Created attachment 185196 [details] svn patch v 2 added missing linebreak
(In reply to David Wahlund from comment #6) Just tested the patch and I'm not sure it is actually changing anything during startup. Can you please verify this? Setting the var suggests it will work in that directory but the option looks like it just runs as emperor, which I'm not fully sure what that means either. Does it even accept an arg? --emperor run the Emperor
(In reply to Richard Gallamore from comment #7) Yes the syntax is '--emperor <path to dir>'. Instead if spawning an instance mapped to a specific application it searches the dir for configs to multiple applications. Also doing reloading, load balancing and such. pidfile is changed to emperor-pidfile and uid/gid is changed to vassal-uid/gid and the socket argument is removed. I just passed the patch along. Brb after some testing.
So, this will probably need some rewriting to work. Looking att the nginx rc-script for inspiration I'd say it probably is a good idea to move all config to a config file (eg /usr/local/etc/uwsgi/uwsgi.ini) and remove it from the script. Keeping the profiles part. Today config is passed using flags and flags in the script are conflicting (the emperor has no sockets etc).
Created attachment 185444 [details] Proposed new rc-script So, I've tried to change the script. It's my first ever rc script so bear with me. Changes: - new config: 'uwsgi_configfile (path)' to explicitly specify the location of the config file (default: /usr/local/etc/uwsgi/uwsgi.ini). If file is not present the flag is not set - new config: 'uwsgi_emperor (bool)' set this to use special set of flags for emperor - new config: 'uwsgi_vassals_dir (path)' explicit location of the dir for vassals files - not using command_args but uwsgi_flags throughout - not spawning emperor as master (spawned two processes, near impossible to kill) - changed socket owner to uwsgi:uwsgi (add www to group uwsgi instead) Feel free to comment
(In reply to David Wahlund from comment #10) Few things, * Pretty sure adding www to group uwsgi would require user interaction, or am I wrong? Maybe best to leave at least the group to www for socket. * All the /usr/local entries should be %%PREFIX%% * Why was command_args was removed and changed to uwsgi_flags? This should probably be changed back to command_args after the if checkyesno uwsgi_emperor and add uwsgi_flags to it. _flags is usually not modified in the rc.d script, but sometimes it is necessary. * The beginning quote should start the defining variable. Pretty sure that if a space were added at the end of uwsgi_configfile, this would drop the rest of the line. Same with the other variables at the end of the defining variable. other than that it looks good to me. Thanks for working on this David.
(In reply to Richard Gallamore from comment #11) * There seems to be a problem with using different users for the sockets than the uid/gid och the running process. Figured it's cleaner to use www in the uwsgi group than having uwsgi talking to a www:www socket. https://github.com/unbit/uwsgi/issues/1471 * Yep, for the uwsgi.in-file? Got it. * Point 1 from here: https://www.freebsd.org/doc/en_US.ISO8859-1/articles/rc-scripting/rcng-daemon-adv.html#rcng-daemon-adv-args "Never include dashed options, like -X or --foo, in command_args." Well the flags _are_ in fact being changed in the rc.d script based on config so I figured it should probably go in that var. * I didn't get this part. Which line is it?
(In reply to David Wahlund from comment #12) As long as you test the socket being 165:165 and you don't think it will break anything for users upgrading without interaction it should be okay. We need to add update note otherwise. I just need to know which it is. Uwsgi_flags should be okay then. I was reading the man rc.subr variable for it, if this is what the docs suggest though then it is fine. Nevermind about the last point. I just ran a test to verify my statement and it was incorrect. It works, I usually add quotes at start and end of variable assignments which is what I thought would cause issues.
* 165:165 seems to be the reserved uwsgi uid/gid since jan https://reviews.freebsd.org/D9398. As I'm using --chown-socket to specify uid:gid I'm sure what the uid:gid is. However, it seems a good idea to user uwsgi:www instead. Making sure uwsgi has rw to socket after it has dropped to the uwsgi user. This seems to have been solved previously by running uwsgi as www:www. Pretty sure that's not a good idea. Also it shouldn't break anything on update. * I've found a problem with flags though. Current installations, I think (atleast I am), uses the uwsgi_flags to pass the config-file argument. This overwrites the default flags (-M -L). Seems to be impossible to pass a config file without overwriting them. Seems to work based on the only argument passed not being a flag is intepreted as a config file. So the actual command is now 'uwsgi <path to config> <lots of flags passed as args>', not pretty. Maybe put an update notice on using the new uwsgi_configfile instead. This is limited to ini-format right now even though uwsgi supports multiple formats. Adding uwsgi_configfile_json etc might be an idea for the future. I reworked the flags handling a bit and now I'm using the start_precmd() to set rc_flags and then concating the uwsgi_flags at the end of it. This will work with existing rc.conf settings using uwsgi_flags to specify conf-file.
Created attachment 185485 [details] New rc-script v2 - using rc_flags in start_precmd() - using chmod uwsgi:www on socket though running as uwsgi:uwsgi
A commit references this bug: Author: ultima Date: Sun Aug 20 22:53:57 UTC 2017 New revision: 448441 URL: https://svnweb.freebsd.org/changeset/ports/448441 Log: * Bump port revision, added emperor support to rc script * Changed socket uid/gid and mode, this should fix github issue #1471[1] [1] https://github.com/unbit/uwsgi/issues/1471 PR: 210170 Submitted by: David Wahlund Reviewed by: matthew (mentor) Approved by: matthew (mentor) Differential Revision: https://reviews.freebsd.org/D12089 Changes: head/UPDATING head/www/uwsgi/Makefile head/www/uwsgi/files/uwsgi.in
Committed, thanks!
(In reply to Richard Gallamore from comment #17) Nice! Thanks. I think you can close https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216924 and https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=217178 Though it probably needs testing. The sockets are created as root (I think) but recreated as uwsgi on reload. So uwsgi needs execute on the directory. Atleast now uwsgi has rw on the socket.
(In reply to David Wahlund from comment #18) The /tmp directory should already have execute perms.