Fix logrotate and update rpm spec file #56
Conversation
alexjfisher
force-pushed
the
fix_spec_file
branch
from
8e6750d to
85071bc
Compare
|
Commit amended to now also include creation of log directory. This already happens in the debian build. |
alexjfisher
changed the title
rpm/nginx-ldap-auth.spec
Outdated
| @@ -50,6 +51,7 @@ getent passwd nginx-ldap-auth > /dev/null || \ | |||
| %preun | |||
| /usr/bin/systemctl --no-reload disable nginx-ldap-auth.service >/dev/null 2>&1 ||: | |||
| /usr/bin/systemctl stop nginx-ldap-auth.service >/dev/null 2>&1 ||: | |||
| rm -f /var/log/nginx-ldap-auth/* | |||
There was a problem hiding this comment.
There is no check if this is an upgrade or removal. Either way it looks to be a bad idea to purge all logs.
There was a problem hiding this comment.
I wasn't too sure about this (or spec files in general), so went looking for examples. I got this from https://github.com/mozilla-services/squid-rpm/blob/ea9a11c15092788f270730e0264f01ab5503a82e/SPECS/squid.spec#L253
But maybe that's not a very good practice and I'm cargo-cult copying a bad example!
There was a problem hiding this comment.
Just come across https://fedoraproject.org/wiki/PackagingDrafts/Logfiles
logfiles should not be removed by package removals
I can update the PR to follow those guidelines.
rpm/nginx-ldap-auth.spec
Outdated
| @@ -19,7 +19,7 @@ Reference implementation of method for authenticating users on behalf of | |||
| servers proxied by NGINX or NGINX Plus. | |||
|
|
|||
| %prep | |||
| %setup -q | |||
| %setup -q -n nginx-ldap-auth-release-%{version} | |||
There was a problem hiding this comment.
renaming directory from 'nginx-ldap-auth-%version' to 'nginx-ldap-auth-release-%version' seems to be useless. Could you please justify this change?
There was a problem hiding this comment.
That's what's in the 0.0.4 release tarball. After downloading the release, into rpmbuild/SOURCES and extracting the spec file into rpmbuild\SPECS, the build fails with
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.QO2R9V
+ umask 022
+ cd /home/afisher/rpmbuild/BUILD
+ cd /home/afisher/rpmbuild/BUILD
+ rm -rf nginx-ldap-auth-0.0.4
+ /usr/bin/gzip -dc /home/afisher/rpmbuild/SOURCES/nginx-ldap-auth-release-0.0.4.tar.gz
+ /usr/bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd nginx-ldap-auth-0.0.4
/var/tmp/rpm-tmp.QO2R9V: line 35: cd: nginx-ldap-auth-0.0.4: No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.QO2R9V (%prep)
There was a problem hiding this comment.
OK, got it.
I suppose we should change directory name packed into a release tarball. Mostly because it's somehow wrote to make people guess proper directory name while it should be %name-%version.
There was a problem hiding this comment.
I changed release naming (and thus - directory naming inside archives). Please, remove this change from your PR.
|
As far as I can see, in debian we are using an init script which redirects output to a log file. In redhat we rely on a |
|
nginx-ldap-auth/nginx-ldap-auth.default Line 14 in d9a2149 Sourced by nginx-ldap-auth/nginx-ldap-auth.service Line 10 in d9a2149
In |
|
Could you please remove %setup modification and logs removal and I will merge your PR. |
alexjfisher
force-pushed
the
fix_spec_file
branch
from
85071bc to
97f7d0f
Compare
alexjfisher
changed the title
| @@ -1,4 +1,5 @@ | |||
| /var/log/nginx-ldap-auth/daemon.log { | |||
| compress | |||
| delaycompress | |||
There was a problem hiding this comment.
delaycompress doesn't do anything if compress isn't specified. logrotate is deletes all log content without this.
| Name: nginx-ldap-auth | ||
| Version: 0.0.3 | ||
| Version: 0.0.5 |
There was a problem hiding this comment.
On the assumption the tarball layout fix will be in a 0.0.5 release.
* Create log directory in spec file * Fix logrotate file
alexjfisher
force-pushed
the
fix_spec_file
branch
from
97f7d0f to
61d8777
Compare
I think the debian equivalent also needs updating, but with these changes I was able to build a 0.0.4 RPM on CentOS 7.