Bug 25510

Summary: Typo in koha-common.postinst causing shell errors
Product: Koha Reporter: David Cook <dcook>
Component: PackagingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: critical    
Priority: P5 - low CC: 1joynelson, jonathan.druart, kyle, lucas, martin.renvoize, mirko, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24905
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25524
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06
Bug Depends on: 15253, 24905, 25032, 25068    
Bug Blocks: 25524, 25529    
Attachments: Bug 25510: Fix shell syntax for updating log4perl.conf for instances
Bug 25510: Fix syntax error in koha-common.postinst
Bug 25510: Put space between negation operator and function name
Bug 25510: Fix syntax error in koha-common.postinst
Bug 25510: Put space between negation operator and function name
Bug 25510: Fix syntax error in koha-common.postinst
Bug 25510: Put space between negation operator and function name
[19.11] Bug 25510: Fix syntax error in koha-common.postinst
[19.11] Bug 25510: Put space between negation operator and function name
[19.11] Bug 25510: Fix syntax error in koha-common.postinst
[19.11] Bug 25510: Put space between negation operator and function name

Description David Cook 2020-05-15 05:17:08 UTC
When installing a custom Koha 19.11.05, I got the following error:

/var/lib/dpkg/info/koha-common.postinst: 155: [: !log4perl_component: unexpected operator

I don't think we should be using the square brackets in that line at all and should put a space between ! and log4perl_component.
Comment 1 David Cook 2020-05-15 05:20:43 UTC
Created attachment 104912 [details] [review]
Bug 25510: Fix shell syntax for updating log4perl.conf for instances

This patch fixes the shell syntax when updating log4perl.conf
for instances.
Comment 2 Jonathan Druart 2020-05-15 08:01:50 UTC
So Koha install is broken using the Debian package?
Comment 3 Jonathan Druart 2020-05-15 08:11:43 UTC
If I execute:

log4perl_component()
{
    local config=$1
    local component=$2

    if [ grep -q -x "log4perl.logger.$component" $config ]; then
        return 0
    else
        return 1
    fi
}


for site in $(koha-list); do
    log4perl_config="/etc/koha/sites/$site/log4perl.conf"
    if ! log4perl_component $log4perl_config "sip"; then
        echo $site
    fi  
done


I get 
xxx.pl: line 67: [: too many arguments

Looks like we need another change
Comment 4 Jonathan Druart 2020-05-15 08:13:04 UTC
Created attachment 104921 [details] [review]
Bug 25510: Fix syntax error in koha-common.postinst
Comment 5 Jonathan Druart 2020-05-15 08:15:06 UTC
Comment on attachment 104912 [details] [review]
Bug 25510: Fix shell syntax for updating log4perl.conf for instances

This patch is not sufficient for master.
Comment 6 Victor Grousset/tuxayo 2020-05-15 23:31:56 UTC
IIUC from various IRC discussions, the cleanest[1] way to test package install patches while being simple[1] enough to be less likely to messed things up is:

- apply comment 4
- make packages with https://hub.docker.com/r/koha/koha-dpkg
- spin a standard Debian (9?) however one wants (container, VM, VPS)
- install the packages following https://wiki.koha-community.org/wiki/Koha_on_Debian

[1] for someone without experience with building packages and installing Koha from them
Comment 7 Victor Grousset/tuxayo 2020-05-15 23:32:13 UTC
Anything missing?
Comment 8 David Cook 2020-05-17 23:06:52 UTC
(In reply to Jonathan Druart from comment #2)
> So Koha install is broken using the Debian package?

Arguably yes. The majority of the install works correctly, but it looks like it ditches out at the end of koha-common.postinst.
Comment 9 David Cook 2020-05-17 23:14:23 UTC
(In reply to Victor Grousset/tuxayo from comment #6)
> IIUC from various IRC discussions, the cleanest[1] way to test package
> install patches while being simple[1] enough to be less likely to messed
> things up is:
> 
> - apply comment 4
> - make packages with https://hub.docker.com/r/koha/koha-dpkg
> - spin a standard Debian (9?) however one wants (container, VM, VPS)
> - install the packages following
> https://wiki.koha-community.org/wiki/Koha_on_Debian
> 
> [1] for someone without experience with building packages and installing
> Koha from them

That's mostly correct, although "install the packages" would need to use either "apt install ./koha-common<blahblah>" or "dpkg -i koha-common<blahblah>"

For making packages, I use my own Docker image (https://gitlab.com/minusdavid/koha-deb-builder-docker), but I imagine there's community support for https://hub.docker.com/r/koha/koha-dpkg, so that's probably better for someone without experience.
Comment 10 Tomás Cohen Arazi 2020-05-17 23:27:33 UTC
(In reply to David Cook from comment #9)
> (In reply to Victor Grousset/tuxayo from comment #6)
> > IIUC from various IRC discussions, the cleanest[1] way to test package
> > install patches while being simple[1] enough to be less likely to messed
> > things up is:
> > 
> > - apply comment 4
> > - make packages with https://hub.docker.com/r/koha/koha-dpkg
> > - spin a standard Debian (9?) however one wants (container, VM, VPS)
> > - install the packages following
> > https://wiki.koha-community.org/wiki/Koha_on_Debian
> > 
> > [1] for someone without experience with building packages and installing
> > Koha from them
> 
> That's mostly correct, although "install the packages" would need to use
> either "apt install ./koha-common<blahblah>" or "dpkg -i
> koha-common<blahblah>"
> 
> For making packages, I use my own Docker image
> (https://gitlab.com/minusdavid/koha-deb-builder-docker), but I imagine
> there's community support for https://hub.docker.com/r/koha/koha-dpkg, so
> that's probably better for someone without experience.

That's interesting, David. Mine is not community per-se. I like yours, might steal some of it for mine
Comment 11 David Cook 2020-05-17 23:49:10 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> That's interesting, David. Mine is not community per-se. I like yours, might
> steal some of it for mine

Cool ^_^.

It's a work-in-progress, but I've been using it for a while and have it integrated in a CD pipeline, which is working pretty well.
Comment 12 David Cook 2020-05-18 06:22:19 UTC
(In reply to Jonathan Druart from comment #4)
> Created attachment 104921 [details] [review] [review]
> Bug 25510: Fix syntax error in koha-common.postinst

I'm getting the following errors:

/var/lib/dpkg/info/koha-common.postinst: 155: /var/lib/dpkg/info/koha-common.postinst: !log4perl_component: not found
/var/lib/dpkg/info/koha-common.postinst: 168: /var/lib/dpkg/info/koha-common.postinst: !log4perl_component: not found
/var/lib/dpkg/info/koha-common.postinst: 184: /var/lib/dpkg/info/koha-common.postinst: !log4perl_component: not found

I think we need that space between the ! and the function name. I'll add a patch for that and try again.
Comment 13 David Cook 2020-05-18 06:24:19 UTC
Created attachment 105005 [details] [review]
Bug 25510: Put space between negation operator and function name

This patch puts a space between the negation operator ! and
the function name (ie log4perl_component).
Comment 14 David Cook 2020-05-18 06:43:43 UTC
(In reply to David Cook from comment #13)
> Created attachment 105005 [details] [review] [review]
> Bug 25510: Put space between negation operator and function name
> 
> This patch puts a space between the negation operator ! and
> the function name (ie log4perl_component).

With this patch, there are no longer error messages... but log4perl_component() doesn't look like it's working correctly because it just keeps appending to /etc/koha/sites/$site/log4perl.conf
Comment 15 David Cook 2020-05-18 06:47:37 UTC
(In reply to David Cook from comment #14)
> With this patch, there are no longer error messages... but
> log4perl_component() doesn't look like it's working correctly because it
> just keeps appending to /etc/koha/sites/$site/log4perl.conf

And it's because the grep in log4perl_component() is bad...
Comment 16 David Cook 2020-05-18 07:04:33 UTC
Note this affects 19.11 and not just master
Comment 17 Jonathan Druart 2020-05-18 08:19:25 UTC
Indeed, I had the space in my test script but did not correctly applied the changes :-/
Comment 18 David Cook 2020-05-18 08:22:15 UTC
Created attachment 105007 [details] [review]
Bug 25510: Fix syntax error in koha-common.postinst

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 19 David Cook 2020-05-18 08:22:19 UTC
Created attachment 105008 [details] [review]
Bug 25510: Put space between negation operator and function name

This patch puts a space between the negation operator ! and
the function name (ie log4perl_component).

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 20 Victor Grousset/tuxayo 2020-05-18 11:13:22 UTC
(In reply to David Cook from comment #9)
> (In reply to Victor Grousset/tuxayo from comment #6)
> > - install the packages following
> > https://wiki.koha-community.org/wiki/Koha_on_Debian
> 
> That's mostly correct, although "install the packages" would need to use
> either "apt install ./koha-common<blahblah>" or "dpkg -i
> koha-common<blahblah>"

`apt-get install koha-common` is indeed part of https://wiki.koha-community.org/wiki/Koha_on_Debian

I feel I didn't get your meaning.

Thanks a lot for the signing.
Comment 21 Tomás Cohen Arazi 2020-05-18 23:08:29 UTC
Created attachment 105052 [details] [review]
Bug 25510: Fix syntax error in koha-common.postinst

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2020-05-18 23:08:33 UTC
Created attachment 105053 [details] [review]
Bug 25510: Put space between negation operator and function name

This patch puts a space between the negation operator ! and
the function name (ie log4perl_component).

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 David Cook 2020-05-19 02:32:33 UTC
Thanks Tomas :)

We can't forget Bug 25524 either, as fixing Bug 25510 will cause Bug 25524 to become an issue.
Comment 24 Tomás Cohen Arazi 2020-05-19 03:00:54 UTC
(In reply to David Cook from comment #23)
> Thanks Tomas :)
> 
> We can't forget Bug 25524 either, as fixing Bug 25510 will cause Bug 25524
> to become an issue.

I know. But internet gets painfully slow here around 8 pm with all the neighbours streamming Netflix et al, and I got pulled to the kitchen to make dinner :-D

I have everything set to properly QA all this packaging bugs (koha-dpkg, vagrant+virtualbox) I just need more time as installing 19.11 (and deps) to test upgrades takes a lot of time here.
Comment 25 David Cook 2020-05-19 03:26:51 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> I know. But internet gets painfully slow here around 8 pm with all the
> neighbours streamming Netflix et al, and I got pulled to the kitchen to make
> dinner :-D
> 
> I have everything set to properly QA all this packaging bugs (koha-dpkg,
> vagrant+virtualbox) I just need more time as installing 19.11 (and deps) to
> test upgrades takes a lot of time here.

No worries. Very grateful for your work on these! 

I just didn't want to be responsible for adding bugs while fixing bugs heh.
Comment 26 Martin Renvoize 2020-05-19 07:30:32 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Victor Grousset/tuxayo 2020-05-19 16:49:49 UTC
19.11 mantainer: relaying here a message from bug 25524 comment 3

> 19.11 mantainer: this should be pushed at the same time we push 25510.
Comment 28 Joy Nelson 2020-05-19 23:17:09 UTC
missing dependencies-not backported to 19.11.x
Comment 29 Joy Nelson 2020-05-19 23:18:23 UTC
not sure how to push this to 19.11.x branch.
Comment 30 David Cook 2020-05-20 01:57:58 UTC
(In reply to Joy Nelson from comment #29)
> not sure how to push this to 19.11.x branch.

Oh right, it'll need a different patch, as the koha-common.postinst in 19.11 is a fair bit different to master.

I did my own local 19.11 patch but I meant to put one up here too.
Comment 31 David Cook 2020-05-20 02:05:30 UTC
Created attachment 105134 [details] [review]
[19.11] Bug 25510: Fix syntax error in koha-common.postinst

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 David Cook 2020-05-20 02:05:34 UTC
Created attachment 105135 [details] [review]
[19.11] Bug 25510: Put space between negation operator and function name

This patch puts a space between the negation operator ! and
the function name (ie log4perl_component).

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 David Cook 2020-05-20 02:06:28 UTC
Not sure what the process is for backporting, but someone in QA might want to look over those 19.11 patches. I haven't fully tested them. I just fixed the merge conflicts when applying the master patches.
Comment 34 Kyle M Hall 2020-05-20 14:41:20 UTC
Created attachment 105152 [details] [review]
[19.11] Bug 25510: Fix syntax error in koha-common.postinst

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Kyle M Hall 2020-05-20 14:41:46 UTC
Created attachment 105153 [details] [review]
[19.11] Bug 25510: Put space between negation operator and function name

This patch puts a space between the negation operator ! and
the function name (ie log4perl_component).

Signed-off-by: David Cook <dcook@prosentient.com.au>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Joy Nelson 2020-05-20 14:44:21 UTC
19.11.x patches reviewed by Kyle and pushed by me for 19.11.06
Thanks!
Comment 37 Victor Grousset/tuxayo 2020-05-20 17:53:44 UTC
Is 19.05 also affected?
Comment 38 David Cook 2020-05-20 23:26:55 UTC
(In reply to Joy Nelson from comment #36)
> 19.11.x patches reviewed by Kyle and pushed by me for 19.11.06
> Thanks!

Woo! You're welcome ^_^.
Comment 39 David Cook 2020-05-20 23:28:51 UTC
(In reply to Victor Grousset/tuxayo from comment #37)
> Is 19.05 also affected?

I don't think so. Since Bug 24905 wasn't pushed to 19.05, it should be safe.

We skipped over 19.05 and are doing many upgrades to 19.11, so that's all I've been targeting at the moment for my work.
Comment 40 Lucas Gass 2020-05-22 14:47:04 UTC
missing dependencies for 19.05.x, no backport