Bug 195916
| Summary: | www/nginx: compiled-in error log location is littering /var/log | ||
|---|---|---|---|
| Product: | Ports & Packages | Reporter: | VK <vlad-fbsd> |
| Component: | Individual Port(s) | Assignee: | John Marino <marino> |
| Status: | Closed FIXED | ||
| Severity: | Affects Only Me | CC: | freebsd, marino, osa, vlad-fbsd |
| Priority: | --- | Keywords: | easy, patch, patch-ready |
| Version: | Latest | Flags: | bugzilla:
maintainer-feedback?
(osa) |
| Hardware: | Any | ||
| OS: | Any | ||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=212416 | ||
| Attachments: | |||
|
Description
VK
2014-12-12 10:43:07 UTC
Auto-assigned to maintainer osa@FreeBSD.org Thanks for report. Could you please provide a patch) for fix the issue. 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). This PR should be reopened and tossed back in the pool, as it seems like a valid change. Patch is available as well. Created attachment 163668 [details]
Consolidate NGINX logs under /var/log/nginx/
New patch against r402634.
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. :)
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. """ 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. 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. 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. (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. 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.
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. Can you also provide the text for UPDATING then? I'm just a "random committer" in this case. ah, you did via comment #7 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. 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 Thanks for your perseverance, Vladimir! 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. (In reply to John Marino from comment #19) No problem, will do that. |
