~alpine/aports

4 2

[alpine-aports] [PATCH] testing/rspamd: upgrade to 1.4.4

Details
Message ID
<20170224143718.22639-1-valery.kartel@gmail.com>
Sender timestamp
1487947038
DKIM signature
missing
Download raw message
Patch: +7 -21
fixes in init script
---
 testing/rspamd/APKBUILD     | 18 +++---------------
 testing/rspamd/rspamd.initd | 10 ++++------
 2 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/testing/rspamd/APKBUILD b/testing/rspamd/APKBUILD
index 85fc20a1b8..bed5b9b0f0 100644
--- a/testing/rspamd/APKBUILD
+++ b/testing/rspamd/APKBUILD
@@ -2,8 +2,8 @@
# Contributor: Valery Kartel <valery.kartel@gmail.com>
# Contributor: Nathan Angelacos <nangel@alpinelinux.org>
pkgname=rspamd
pkgver=1.4.3
pkgrel=1
pkgver=1.4.4
pkgrel=0
pkgdesc="Fast, free and open-source spam filtering system"
url="https://rspamd.com"
arch="x86_64 x86 armhf"
@@ -106,19 +106,7 @@ utils() {
	mv "$pkgdir"/usr/bin/${pkgname}-redirector "$subpkgdir"/usr/bin
}

md5sums="a64e00b906b7f3590e855be50106f99f  rspamd-1.4.3.tar.xz
c152c6a90f6ae9e5a7a1d137dfbc0305  rspamd.logrotated
0656acc12dcce7cba232857a848d0390  rspamd.initd
ab12f33ad8e12a7437fb5ea4a9c92eef  rspamd.confd
ff0bf4f1b1447ca401865369f91418f1  config.patch
83f76787389649af63ea921f81cf35b3  cmakelists.patch"
sha256sums="144cddc25ce8d8519b289d2c00d34b37c931f3c7293e2b0d16c408680021a1bf  rspamd-1.4.3.tar.xz
6c5e79e9052d957f3d0d634b2ae7a56bbc0901a5d6946dc991c92f19a72fce97  rspamd.logrotated
5bcf68a72e0582859799cd335a14d48914e5291f4087327b3b42562ef64e2958  rspamd.initd
fceba39a20755d48ef3838f7daa92ed9686a118c0a26f06bfe72cc7c15b2e384  rspamd.confd
7e0adb4a2e7e8f806fc5fdae0c37e6948ed56e9d46d4eed0c681806decb90e49  config.patch
d4bc7851bb32b49be98e3964aa9195033b586da27f7f3afa5ce12dfbeadd96b0  cmakelists.patch"
sha512sums="bae86953d881be446f049384cc8bf8451b04619b461a7e2d571407d5af5f2547af8bc9db578e84190e8c55d01f9e32ea1d29e998daa2bbb2fac431ed50df27ca  rspamd-1.4.3.tar.xz
sha512sums="df2260de6585699a5b6692aa210647270b7132372ae6a2437f8532265018fe4db8270a8989ce0c0664cad17cbc281ae92f935c4f0974dc2f8da309e54c234b14  rspamd-1.4.4.tar.xz
2efe28575c40d1fba84b189bb872860e744400db80dce2f6330be6c6287fb3f46e6511284729b957488bf40bcb9b0952e26df9934f5f138334bd2766075c45cb  rspamd.logrotated
e240983c2fd91d8061b17e35e83a75c56ac2c3625dbe07b83a2e89e3dbf69b57c675d01772b93968aa6b22d150d54d366f2fb4a0c6551b3cfdedc77d82e28652  rspamd.initd
1320a752cbefe021079b3ed23e312231c39fc600baba7d8440c3e8bdd33ed673c8831b44a0c76788a8d905469e81088adf26cf245c09b4c2c4fdce7ff4219ee7  rspamd.confd
diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rspamd.initd
index 93148ccb4b..b0eb9b057a 100644
--- a/testing/rspamd/rspamd.initd
+++ b/testing/rspamd/rspamd.initd
@@ -18,20 +18,18 @@ depend() {
}

start_pre() {
	ebegin
	checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
	[ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group ${pidfile%/*}
	$command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
	eend $?
}

reload() {
	ebegin "Reloading ${SVCNAME}"
	start-stop-daemon --signal HUP --pidfile $pidfile
	ebegin "Reloading $RC_SVCNAME config"
	start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
	eend $?
}

reopen() {
	ebegin "Reopening ${SVCNAME} log files"
	ebegin "Reopening $RC_SVCNAME log files"
	start-stop-daemon --signal USR1 --pidfile $pidfile
	eend $?
}
-- 
2.11.1



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20170224164839.47bce33d@ncopa-desktop.copa.dup.pw>
In-Reply-To
<20170224143718.22639-1-valery.kartel@gmail.com> (view parent)
Sender timestamp
1487951319
DKIM signature
missing
Download raw message
On Fri, 24 Feb 2017 16:37:18 +0200
Valery Kartel <valery.kartel@gmail.com> wrote:

> fixes in init script
> ---
>  testing/rspamd/APKBUILD     | 18 +++---------------
>  testing/rspamd/rspamd.initd | 10 ++++------
>  2 files changed, 7 insertions(+), 21 deletions(-)
> 

... 

> diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rspamd.initd
> index 93148ccb4b..b0eb9b057a 100644
> --- a/testing/rspamd/rspamd.initd
> +++ b/testing/rspamd/rspamd.initd
> @@ -18,20 +18,18 @@ depend() {
>  }
>  
>  start_pre() {
> -	ebegin
> -	checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> +	[ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group ${pidfile%/*}
>  	$command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> -	eend $?
>  }
>  
>  reload() {
> -	ebegin "Reloading ${SVCNAME}"
> -	start-stop-daemon --signal HUP --pidfile $pidfile
> +	ebegin "Reloading $RC_SVCNAME config"
> +	start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
>  	eend $?
>  }
>  
>  reopen() {
> -	ebegin "Reopening ${SVCNAME} log files"
> +	ebegin "Reopening $RC_SVCNAME log files"
>  	start-stop-daemon --signal USR1 --pidfile $pidfile
>  	eend $?
>  }


I would have prefered:

checkconfig() {
	$command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
}

start_pre() {
	checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
	checkconfig
}

reload() {
	ebegin "Reloading $RC_SVCNAME config"
	checkconfig && start-stop-daemon --signal HUP --pidfile $pidfile
	eend $?
}

-nc


---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Details
Message ID
<CAKTwcDNt305ELVuptOT+w1DXjKwVQ=hYB-iiLcn17tQvRYj6NA@mail.gmail.com>
In-Reply-To
<20170224164839.47bce33d@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1487966193
DKIM signature
missing
Download raw message
Yes, it's a good idea, and sometimes useful.

But I think checkconfig() need to be just a wrapper for start_pre() covered
with user-friendly ebegin...eend.
Because start_pre() is an internal function with silent code, but
checkconfig is a user-called command with some smarter output.

so start_pre() would stay as-is and checkconfig would be added to
extra_commands list ... like this:

===
extra_commands="checkconfig"
description_checkconfig="Verify configuration file"
....
checkconfig() {
    ebegin "Checking $RC_SVCNAME config"
    startuplog=/dev/tty; start_pre
    eend $?
}
===

What do you think?

2017-02-24 17:48 GMT+02:00 Natanael Copa <ncopa@alpinelinux.org>:

> On Fri, 24 Feb 2017 16:37:18 +0200
> Valery Kartel <valery.kartel@gmail.com> wrote:
>
> > fixes in init script
> > ---
> >  testing/rspamd/APKBUILD     | 18 +++---------------
> >  testing/rspamd/rspamd.initd | 10 ++++------
> >  2 files changed, 7 insertions(+), 21 deletions(-)
> >
>
> ...
>
> > diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rspamd.initd
> > index 93148ccb4b..b0eb9b057a 100644
> > --- a/testing/rspamd/rspamd.initd
> > +++ b/testing/rspamd/rspamd.initd
> > @@ -18,20 +18,18 @@ depend() {
> >  }
> >
> >  start_pre() {
> > -     ebegin
> > -     checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> > +     [ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group
> ${pidfile%/*}
> >       $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> > -     eend $?
> >  }
> >
> >  reload() {
> > -     ebegin "Reloading ${SVCNAME}"
> > -     start-stop-daemon --signal HUP --pidfile $pidfile
> > +     ebegin "Reloading $RC_SVCNAME config"
> > +     start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
> >       eend $?
> >  }
> >
> >  reopen() {
> > -     ebegin "Reopening ${SVCNAME} log files"
> > +     ebegin "Reopening $RC_SVCNAME log files"
> >       start-stop-daemon --signal USR1 --pidfile $pidfile
> >       eend $?
> >  }
>
>
> I would have prefered:
>
> checkconfig() {
>         $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> }
>
> start_pre() {
>         checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
>         checkconfig
> }
>
> reload() {
>         ebegin "Reloading $RC_SVCNAME config"
>         checkconfig && start-stop-daemon --signal HUP --pidfile $pidfile
>         eend $?
> }
>
> -nc
>
Natanael Copa <ncopa@alpinelinux.org>
Details
Message ID
<20170225190143.5b599511@ncopa-desktop.copa.dup.pw>
In-Reply-To
<CAKTwcDNt305ELVuptOT+w1DXjKwVQ=hYB-iiLcn17tQvRYj6NA@mail.gmail.com> (view parent)
Sender timestamp
1488045703
DKIM signature
missing
Download raw message
On Fri, 24 Feb 2017 21:56:33 +0200
Valery Kartel <valery.kartel@gmail.com> wrote:

> Yes, it's a good idea, and sometimes useful.
> 
> But I think checkconfig() need to be just a wrapper for start_pre() covered
> with user-friendly ebegin...eend.

I think it needs to be the other way around.

> Because start_pre() is an internal function with silent code, but
> checkconfig is a user-called command with some smarter output.
> 
> so start_pre() would stay as-is and checkconfig would be added to
> extra_commands list ... like this:
> 
> ===
> extra_commands="checkconfig"
> description_checkconfig="Verify configuration file"
> ....
> checkconfig() {
>     ebegin "Checking $RC_SVCNAME config"
>     startuplog=/dev/tty; start_pre
>     eend $?
> }
> ===
> 
> What do you think?

I still don't like it. The goal was not to add support for a
"checkconfig" command, the goal was to improve readability. Code that
is easy to read and clearly describes what it does.

The name "start_pre" describes *when* something happens but it does not
describe anything about *what* it does. The name "checkconfig"
describes *what* but does not really say anything about *when*.

If I would have seen a function without the context like this:

checkconfig() {
	start_pre
}

I would have thought something like: "to check the config it is needed
to start something called pre?". Which is not the case. It would work
and do what it should but it would be misleading or confusing.

The same thing happens with the start_pre function. If I see a function
called "start_pre" that looks like:

start_pre () {
	[ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group ${pidfile%/*}  
	$command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
}

The function name indicates that this is something that happens before
start, but in the code, there is a condition for the "checkpath". To
understand when checkpath actually happen you need to understand when
RC_CMD is set to "start" and you start wonder, how and what sets RC_CMD?

You can also read that it runs a command with arg -t but you have no
idea what this does. To figure out what it does you need to first
figure out what command it actually runs, then you need to check the
manpage what -t does.

Lets say that you at some point someone else would need to make
something else happen before start. He or she would find the
"start_pre" function and add the needed thing, but if this person does
not pay close attention to what actually happens (which is not very
clear) this thing would also happen whenever someone wanted to only
check the config with "checkconfig". A bug could easily creep in here.

If we do it the other way around though:

checkconfig() {
	$command $command_args -t
}

start_pre () {
	checkpath -d -m 750 -o $user:$group ${pidfile%/*}
	checkconfig >/dev/null 2>>${startuplog:-/dev/null}
}

It becomes much clearer what is going on.  We could drop checkconfig
alltogether and just do "$command -t" all the places we need to check the
config, but "$command -t" is not a good description of what is actually
happening. By wrapping it in a function named "checkconfig" it because
easier to understand what it does. (btw we should probably avoid using
$command_args there)

The function "start_pre" also becomes more readable. It becomes fairly
clear that before start we need to check some path and then check the
config. If someone else needs to change the code in the future, he or
she can easily just add it to the "start_pre" without unexpected things
happens when someone wants to check the config.

We also get rid of the [ "$RC_CMD" = "start" ] conditional which also
makes code clearer.

In this case, the file is small and there are few lines of code so you
will quickly figure out whats going on, but if this would have been a
huge file or those functions would have been spread out over multiple
files, then it would been a real problem.

So yes, your patch works, but code needs to be easier to read. As a
side-effect of making the code readable you also get the extra command
"checkconfig", but thats just a side-effect. The goal is to make code
more readable and prevent future bugs.

-nc

> 
> 2017-02-24 17:48 GMT+02:00 Natanael Copa <ncopa@alpinelinux.org>:
> 
> > On Fri, 24 Feb 2017 16:37:18 +0200
> > Valery Kartel <valery.kartel@gmail.com> wrote:
> >  
> > > fixes in init script
> > > ---
> > >  testing/rspamd/APKBUILD     | 18 +++---------------
> > >  testing/rspamd/rspamd.initd | 10 ++++------
> > >  2 files changed, 7 insertions(+), 21 deletions(-)
> > >  
> >
> > ...
> >  
> > > diff --git a/testing/rspamd/rspamd.initd b/testing/rspamd/rspamd.initd
> > > index 93148ccb4b..b0eb9b057a 100644
> > > --- a/testing/rspamd/rspamd.initd
> > > +++ b/testing/rspamd/rspamd.initd
> > > @@ -18,20 +18,18 @@ depend() {
> > >  }
> > >
> > >  start_pre() {
> > > -     ebegin
> > > -     checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> > > +     [ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group  
> > ${pidfile%/*}  
> > >       $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> > > -     eend $?
> > >  }
> > >
> > >  reload() {
> > > -     ebegin "Reloading ${SVCNAME}"
> > > -     start-stop-daemon --signal HUP --pidfile $pidfile
> > > +     ebegin "Reloading $RC_SVCNAME config"
> > > +     start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
> > >       eend $?
> > >  }
> > >
> > >  reopen() {
> > > -     ebegin "Reopening ${SVCNAME} log files"
> > > +     ebegin "Reopening $RC_SVCNAME log files"
> > >       start-stop-daemon --signal USR1 --pidfile $pidfile
> > >       eend $?
> > >  }  
> >
> >
> > I would have prefered:
> >
> > checkconfig() {
> >         $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> > }
> >
> > start_pre() {
> >         checkpath --directory --mode 750 --owner $user:$group ${pidfile%/*}
> >         checkconfig
> > }
> >
> > reload() {
> >         ebegin "Reloading $RC_SVCNAME config"
> >         checkconfig && start-stop-daemon --signal HUP --pidfile $pidfile
> >         eend $?
> > }
> >
> > -nc
> >  



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Details
Message ID
<CAKTwcDOrm-muwUsZL-US7pwy2kyR4rKMJ8NnX7ksfzSVmDKzfQ@mail.gmail.com>
In-Reply-To
<20170225190143.5b599511@ncopa-desktop.copa.dup.pw> (view parent)
Sender timestamp
1488184542
DKIM signature
missing
Download raw message
got it. I'll remade init-script.

2017-02-25 20:01 GMT+02:00 Natanael Copa <ncopa@alpinelinux.org>:

> On Fri, 24 Feb 2017 21:56:33 +0200
> Valery Kartel <valery.kartel@gmail.com> wrote:
>
> > Yes, it's a good idea, and sometimes useful.
> >
> > But I think checkconfig() need to be just a wrapper for start_pre()
> covered
> > with user-friendly ebegin...eend.
>
> I think it needs to be the other way around.
>
> > Because start_pre() is an internal function with silent code, but
> > checkconfig is a user-called command with some smarter output.
> >
> > so start_pre() would stay as-is and checkconfig would be added to
> > extra_commands list ... like this:
> >
> > ===
> > extra_commands="checkconfig"
> > description_checkconfig="Verify configuration file"
> > ....
> > checkconfig() {
> >     ebegin "Checking $RC_SVCNAME config"
> >     startuplog=/dev/tty; start_pre
> >     eend $?
> > }
> > ===
> >
> > What do you think?
>
> I still don't like it. The goal was not to add support for a
> "checkconfig" command, the goal was to improve readability. Code that
> is easy to read and clearly describes what it does.
>
> The name "start_pre" describes *when* something happens but it does not
> describe anything about *what* it does. The name "checkconfig"
> describes *what* but does not really say anything about *when*.
>
> If I would have seen a function without the context like this:
>
> checkconfig() {
>         start_pre
> }
>
> I would have thought something like: "to check the config it is needed
> to start something called pre?". Which is not the case. It would work
> and do what it should but it would be misleading or confusing.
>
> The same thing happens with the start_pre function. If I see a function
> called "start_pre" that looks like:
>
> start_pre () {
>         [ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group
> ${pidfile%/*}
>         $command $command_args -t >/dev/null 2>>${startuplog:-/dev/null}
> }
>
> The function name indicates that this is something that happens before
> start, but in the code, there is a condition for the "checkpath". To
> understand when checkpath actually happen you need to understand when
> RC_CMD is set to "start" and you start wonder, how and what sets RC_CMD?
>
> You can also read that it runs a command with arg -t but you have no
> idea what this does. To figure out what it does you need to first
> figure out what command it actually runs, then you need to check the
> manpage what -t does.
>
> Lets say that you at some point someone else would need to make
> something else happen before start. He or she would find the
> "start_pre" function and add the needed thing, but if this person does
> not pay close attention to what actually happens (which is not very
> clear) this thing would also happen whenever someone wanted to only
> check the config with "checkconfig". A bug could easily creep in here.
>
> If we do it the other way around though:
>
> checkconfig() {
>         $command $command_args -t
> }
>
> start_pre () {
>         checkpath -d -m 750 -o $user:$group ${pidfile%/*}
>         checkconfig >/dev/null 2>>${startuplog:-/dev/null}
> }
>
> It becomes much clearer what is going on.  We could drop checkconfig
> alltogether and just do "$command -t" all the places we need to check the
> config, but "$command -t" is not a good description of what is actually
> happening. By wrapping it in a function named "checkconfig" it because
> easier to understand what it does. (btw we should probably avoid using
> $command_args there)
>
> The function "start_pre" also becomes more readable. It becomes fairly
> clear that before start we need to check some path and then check the
> config. If someone else needs to change the code in the future, he or
> she can easily just add it to the "start_pre" without unexpected things
> happens when someone wants to check the config.
>
> We also get rid of the [ "$RC_CMD" = "start" ] conditional which also
> makes code clearer.
>
> In this case, the file is small and there are few lines of code so you
> will quickly figure out whats going on, but if this would have been a
> huge file or those functions would have been spread out over multiple
> files, then it would been a real problem.
>
> So yes, your patch works, but code needs to be easier to read. As a
> side-effect of making the code readable you also get the extra command
> "checkconfig", but thats just a side-effect. The goal is to make code
> more readable and prevent future bugs.
>
> -nc
>
> >
> > 2017-02-24 17:48 GMT+02:00 Natanael Copa <ncopa@alpinelinux.org>:
> >
> > > On Fri, 24 Feb 2017 16:37:18 +0200
> > > Valery Kartel <valery.kartel@gmail.com> wrote:
> > >
> > > > fixes in init script
> > > > ---
> > > >  testing/rspamd/APKBUILD     | 18 +++---------------
> > > >  testing/rspamd/rspamd.initd | 10 ++++------
> > > >  2 files changed, 7 insertions(+), 21 deletions(-)
> > > >
> > >
> > > ...
> > >
> > > > diff --git a/testing/rspamd/rspamd.initd
> b/testing/rspamd/rspamd.initd
> > > > index 93148ccb4b..b0eb9b057a 100644
> > > > --- a/testing/rspamd/rspamd.initd
> > > > +++ b/testing/rspamd/rspamd.initd
> > > > @@ -18,20 +18,18 @@ depend() {
> > > >  }
> > > >
> > > >  start_pre() {
> > > > -     ebegin
> > > > -     checkpath --directory --mode 750 --owner $user:$group
> ${pidfile%/*}
> > > > +     [ "$RC_CMD" = "start" ] && checkpath -d -m 750 -o $user:$group
> > > ${pidfile%/*}
> > > >       $command $command_args -t >/dev/null
> 2>>${startuplog:-/dev/null}
> > > > -     eend $?
> > > >  }
> > > >
> > > >  reload() {
> > > > -     ebegin "Reloading ${SVCNAME}"
> > > > -     start-stop-daemon --signal HUP --pidfile $pidfile
> > > > +     ebegin "Reloading $RC_SVCNAME config"
> > > > +     start_pre && start-stop-daemon --signal HUP --pidfile $pidfile
> > > >       eend $?
> > > >  }
> > > >
> > > >  reopen() {
> > > > -     ebegin "Reopening ${SVCNAME} log files"
> > > > +     ebegin "Reopening $RC_SVCNAME log files"
> > > >       start-stop-daemon --signal USR1 --pidfile $pidfile
> > > >       eend $?
> > > >  }
> > >
> > >
> > > I would have prefered:
> > >
> > > checkconfig() {
> > >         $command $command_args -t >/dev/null
> 2>>${startuplog:-/dev/null}
> > > }
> > >
> > > start_pre() {
> > >         checkpath --directory --mode 750 --owner $user:$group
> ${pidfile%/*}
> > >         checkconfig
> > > }
> > >
> > > reload() {
> > >         ebegin "Reloading $RC_SVCNAME config"
> > >         checkconfig && start-stop-daemon --signal HUP --pidfile
> $pidfile
> > >         eend $?
> > > }
> > >
> > > -nc
> > >
>
>
Reply to thread Export thread (mbox)