Bug 195916 - www/nginx: compiled-in error log location is littering /var/log
Summary: www/nginx: compiled-in error log location is littering /var/log
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: John Marino
URL:
Keywords: easy, patch, patch-ready
Depends on:
Blocks:
 
Reported: 2014-12-12 10:43 UTC by VK
Modified: 2016-09-06 15:43 UTC (History)
4 users (show)

See Also:
bugzilla: maintainer-feedback? (osa)


Attachments
Consolidate default logs under /var/log/nginx/ (1.74 KB, patch)
2015-06-26 17:21 UTC, VK
no flags Details | Diff
Consolidate NGINX logs under /var/log/nginx/ (1.74 KB, patch)
2015-11-30 00:50 UTC, VK
no flags Details | Diff
Consolidate logs under /var/log/nginx/ (2.26 KB, patch)
2016-05-28 10:32 UTC, VK
no flags Details | Diff
Consolidate nginx logs under /var/log/nginx/ (4.01 KB, patch)
2016-06-17 02:43 UTC, VK
no flags Details | Diff
Consolidate logs under /var/log/nginx, minor cleanups (4.01 KB, patch)
2016-09-06 12:25 UTC, VK
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description VK 2014-12-12 10:43:07 UTC
The default, compiled-in location of error log is littering /var/log. The error log will always be touched at /var/log/nginx-error.log regardless of error_log directive in the nginx.conf, which according to the following is not a bug:

http://trac.nginx.org/nginx/ticket/147

This also breaks shell tab completion if you reconfigure for, eg. /var/log/nginx/ subdir which is imho a sane default because there are then two "nginx*" words under /var/log.

So this bug report is really about bringing the compiled-in config to a sane default of /var/log/nginx/ subdirectory.
Comment 1 Bugzilla Automation freebsd_committer freebsd_triage 2014-12-12 10:43:07 UTC
Auto-assigned to maintainer osa@FreeBSD.org
Comment 2 Sergey A. Osokin freebsd_committer freebsd_triage 2015-05-28 02:29:41 UTC
Thanks for report.

Could you please provide a patch) for fix the issue.
Comment 3 VK 2015-06-26 17:21:51 UTC
Created attachment 158076 [details]
Consolidate default logs under /var/log/nginx/

Attached. I think this would warrant an UPDATING message. Something along the lines of:

As of nginx-1.8.0_3,2 the default error log location is /var/log/nginx/error.log and default access log is thus /var/log/nginx/access.log. This is done to isolate the compile-time hardcoded error log path touched on every nginx startup (regardless of configured error_log directive, see http://trac.nginx.org/nginx/ticket/147) under nginx' own logdir, now /var/log/nginx (which is now also created on installation).
Comment 4 Anonymous 2015-08-11 22:08:16 UTC
This PR should be reopened and tossed back in the pool, as it seems like a valid change. Patch is available as well.
Comment 5 VK 2015-11-30 00:50:16 UTC
Created attachment 163668 [details]
Consolidate NGINX logs under /var/log/nginx/

New patch against r402634.
Comment 6 VK 2016-05-28 10:32:31 UTC
Created attachment 170746 [details]
Consolidate logs under /var/log/nginx/

Newest patch, against HEAD, r415979.

* Bump PORTREVISION (changes the way files are installed)
* Move /var/log/nginx-*.logs to /var/log/nginx/ subdirectory
* Replace spaces with tabs after '=' in some places per portlint fatal error

Requires an UPDATING entry to warn about default log placement.

Passes porttools port test. portlint barks warnings, but two fatals (that were part of the latest rev) are corrected with this patch.

Sergey, please approve or close with rejected or works as intended, if you believe that to be the case. Can't really leave this open forever. :)
Comment 7 VK 2016-06-17 02:43:03 UTC
Created attachment 171499 [details]
Consolidate nginx logs under /var/log/nginx/

New patch against newest nginx version in HEAD.

* Default logs to /var/log/nginx/ as this compiles-in the error log location, which is always created on nginx start and litters /var/log/.
* Clean up some tabs to quiet portlint
* Add a helpful comment about default error log location to nginx.conf
* Bump PORTREVISION

Requires UPDATING entry:

"""
AFFECTS: users of www/nginx
AUTHOR: ...

Nginx now creates logs under /var/log/nginx/ and changes default log names to error.log and access.log respectively. This is important for the error log which location is compiled-in and touched on nginx start regardless of configured error log location.

See http://trac.nginx.org/nginx/ticket/147 for more info on why this happens.
"""
Comment 8 VK 2016-06-17 02:46:10 UTC
C'mon guys, a year and a half... If you don't want this fix, then say so, please don't have me waste my time here trying to push this through.

Maintainer feedback timeout. Back to pool.
Comment 9 John Marino freebsd_committer freebsd_triage 2016-08-18 14:23:31 UTC
osa, 
I agree with the submitter.  He's been proactively updating the patch you asked for 14 months ago.  You should either approve it, reject it outright, or state what improvements are needed (with the implicit understanding they'll be committed).

At this point, if somebody like myself were to commit this as-is (pending build test), you haven't the right to object at all.

I don't think Vladimir's effort is being given its proper respect, but other committers are probably reluctant to commit to nginx without your express approval.  It would be great if you could see this PR to its completion.
Comment 10 John Marino freebsd_committer freebsd_triage 2016-08-27 21:45:20 UTC
Vladimir,
Ping this PR in another 5-7 days.  If osa@ hasn't responded in any way by then, I'll commit the patch as it is.

This is at least a 2-month timeout, but arguably a 9 month timeout, so I'm more than justified to commit it.  One more week won't hurt for CYA purposes though.
Comment 11 VK 2016-08-28 12:02:16 UTC
(In reply to John Marino from comment #10)

Thanks. I'll also submit a new patch as the Makefile changed since the last patch, and I'll run another build test.
Comment 12 VK 2016-09-06 12:25:49 UTC
Created attachment 174411 [details]
Consolidate logs under /var/log/nginx, minor cleanups

Updated patch. Portlint, Poudriere (11.0-RC2, amd64) passes. Quick run test places working logs into /var/log/nginx/.

One more thing, USE_OPENSSL should be replaced with USES+= ssl, but it's under a conditional and I wasn't sure what to do with it wrt older versions of FreeBSD, so I left it as is.
Comment 13 VK 2016-09-06 12:29:01 UTC
Another reminder, just in case, that this requires an UPDATING entry as it's a potentially disruptive change if users relied on old log paths. Comment #7 has a suggestion.
Comment 14 John Marino freebsd_committer freebsd_triage 2016-09-06 13:21:05 UTC
Can you also provide the text for UPDATING then?

I'm just a "random committer" in this case.
Comment 15 John Marino freebsd_committer freebsd_triage 2016-09-06 13:23:44 UTC
ah, you did via comment #7
Comment 16 John Marino freebsd_committer freebsd_triage 2016-09-06 13:38:27 UTC
FYI, you're probably correct wrt to USE_OPENSSL but it's out of scope of this PR.  It would need a separate one.   Your patch builds successfully under synth test.
Comment 17 commit-hook freebsd_committer freebsd_triage 2016-09-06 13:58:28 UTC
A commit references this bug:

Author: marino
Date: Tue Sep  6 13:57:39 UTC 2016
New revision: 421429
URL: https://svnweb.freebsd.org/changeset/ports/421429

Log:
  www/nginx: Change default log locations to avoid /var/log pollution

  The nginx error log default will always be touched regardless of the value
  of the error_log directive in nginx.conf.  This is not a bug.  It also
  breaks shell tab completion for the sane default of /var/log/nginx.

  This change aligns the compiled-in default and the configuration default
  of error log to a new default /var/log/nginx directory.

  PR:		195916
  Approved by:	maintainer timeout (15 months)

Changes:
  head/UPDATING
  head/www/nginx/Makefile
  head/www/nginx/files/patch-conf-nginx.conf
  head/www/nginx/pkg-plist
Comment 18 John Marino freebsd_committer freebsd_triage 2016-09-06 13:59:32 UTC
Thanks for your perseverance, Vladimir!
Comment 19 John Marino freebsd_committer freebsd_triage 2016-09-06 14:22:53 UTC
Vladimir, would you mind terribly in opening a new PR with these exact changes but for the www/nginx-devel port?  There's already been a request to update nginx-devel similarly but we need an active PR for that.
Comment 20 VK 2016-09-06 14:35:43 UTC
(In reply to John Marino from comment #19)

No problem, will do that.