Remove (some) bashisms from the test scripts

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
Yet another fixes to make self-test work out of the box on FreeBSD.

Despite having '#!/bin/sh' in the test shell scripts there are some
bashisms present,
that make FreeBSD sh croak.

I've run Debian's 'checkbashisms' script against test scripts and tried to
address most of it's (reasonable) warnings.

Also, one failing test spotted the place where direct call to the
'ldbsearch' wasn't replaced with the configurable path.

With best regards,
Timur Bakeyev.

0001-Remove-some-bashisms-from-the-test-scripts.patch (7K) Download Attachment
0002-Fix-incorrect-ldbsearch-invocation.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 12/22/2017 03:58 AM, Timur I. Bakeyev via samba-technical wrote:

> Yet another fixes to make self-test work out of the box on FreeBSD.
>
> Despite having '#!/bin/sh' in the test shell scripts there are some
> bashisms present,
> that make FreeBSD sh croak.
>
> I've run Debian's 'checkbashisms' script against test scripts and tried to
> address most of it's (reasonable) warnings.
>
> Also, one failing test spotted the place where direct call to the
> 'ldbsearch' wasn't replaced with the configurable path.
>
> With best regards,
> Timur Bakeyev.
>

We have another possible path - mark scripts which use bash syntax as
#!/bin/bash (or even "standardize" on bash).

There are tests which are truly bash-specific - I might have introduced
the first one and the bash dependency was discussed on the list (using
bash arrays - the shadow_copy2 test). I also (hours before this patch
submitted :)) updated the Samba wiki about the need to have bash to run
tests.

I personally find the bash syntax less error prone ([[ vs [ mostly) and
would prefer to keep the "bashisms" as bash scripts instead of shell
scripts, but I can see how there can be strong opinions to the other
side, so we should first solicit input on that and decide on the general
direction.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 9:35 AM, Uri Simchoni <[hidden email]> wrote:

> On 12/22/2017 03:58 AM, Timur I. Bakeyev via samba-technical wrote:
> > Yet another fixes to make self-test work out of the box on FreeBSD.
> >
> > Despite having '#!/bin/sh' in the test shell scripts there are some
> > bashisms present,
> > that make FreeBSD sh croak.
> >
> > I've run Debian's 'checkbashisms' script against test scripts and tried
> to
> > address most of it's (reasonable) warnings.
> >
> > Also, one failing test spotted the place where direct call to the
> > 'ldbsearch' wasn't replaced with the configurable path.
>
> We have another possible path - mark scripts which use bash syntax as
> #!/bin/bash (or even "standardize" on bash).
>
> There are tests which are truly bash-specific - I might have introduced
> the first one and the bash dependency was discussed on the list (using
> bash arrays - the shadow_copy2 test). I also (hours before this patch
> submitted :)) updated the Samba wiki about the need to have bash to run
> tests.
>
> I personally find the bash syntax less error prone ([[ vs [ mostly) and
> would prefer to keep the "bashisms" as bash scripts instead of shell
> scripts, but I can see how there can be strong opinions to the other
> side, so we should first solicit input on that and decide on the general
> direction.
>

Well, bash is evil and should be avoided, whenever it's possible, IMHO.
In general if you care about any kind of portability it should be avoided.

Unfortunately, we can't dictate here, so have to follow the path that Samba
project will chose, but if you going to introduce such a dependency, please,
remember that bash for FreeBSD is a foreign program, so it'll be in
/usr/local/bin/bash. Hence you'll have to apply the same magic you used
for the Perl.

And, as of the given patch - the bashisms we see are, clearly results of the
inaccurate programming, where POSIX shell was meant, but some foreign
elements did slip into. There isn't anything Bash specific in those
scripts, just
some syntax elements wrongly used. And, being prefixed with #!/bin/sh they
demand the wrong guy to do the job.

So, we can coop with bash in FreeBSD, but those particular scripts don't
really
require it and I think, on that matter shouldn't demand it and use it's
syntax.

With regards,
Timur Bakeyev.
Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Fri, 22 Dec 2017 10:05:50 +0100, "Timur I. Bakeyev via
samba-technical" <[hidden email]> wrote:

> On Fri, Dec 22, 2017 at 9:35 AM, Uri Simchoni <[hidden email]> wrote:
>
> > On 12/22/2017 03:58 AM, Timur I. Bakeyev via samba-technical wrote:  
> > > Yet another fixes to make self-test work out of the box on FreeBSD.
> > >
> > > Despite having '#!/bin/sh' in the test shell scripts there are some
> > > bashisms present,
> > > that make FreeBSD sh croak.
> > >
> > > I've run Debian's 'checkbashisms' script against test scripts and tried  
> > to  
> > > address most of it's (reasonable) warnings.
> > >
> > > Also, one failing test spotted the place where direct call to the
> > > 'ldbsearch' wasn't replaced with the configurable path.  
> >
> > We have another possible path - mark scripts which use bash syntax as
> > #!/bin/bash (or even "standardize" on bash).
> >
> > There are tests which are truly bash-specific - I might have introduced
> > the first one and the bash dependency was discussed on the list (using
> > bash arrays - the shadow_copy2 test). I also (hours before this patch
> > submitted :)) updated the Samba wiki about the need to have bash to run
> > tests.
> >
> > I personally find the bash syntax less error prone ([[ vs [ mostly) and
> > would prefer to keep the "bashisms" as bash scripts instead of shell
> > scripts, but I can see how there can be strong opinions to the other
> > side, so we should first solicit input on that and decide on the general
> > direction.
> >  
>
> Well, bash is evil and should be avoided, whenever it's possible, IMHO.
> In general if you care about any kind of portability it should be avoided.

I tend to agree.  I try to aim for POSIX compliant shell.  While bash
is very tempting there are very few features that are really needed.

> Unfortunately, we can't dictate here, so have to follow the path that Samba
> project will chose, but if you going to introduce such a dependency, please,
> remember that bash for FreeBSD is a foreign program, so it'll be in
> /usr/local/bin/bash. Hence you'll have to apply the same magic you used
> for the Perl.
>
> And, as of the given patch - the bashisms we see are, clearly results of the
> inaccurate programming, where POSIX shell was meant, but some foreign
> elements did slip into. There isn't anything Bash specific in those
> scripts, just
> some syntax elements wrongly used. And, being prefixed with #!/bin/sh they
> demand the wrong guy to do the job.

For CTDB (ctdb/ subdirectory) we have aimed for POSIX compliant shell
as much as possible.  At the moment the only bash scripts are the
integration tests.  All the run-time scripts (except onnode) and unit
tests are all POSIX.  All of the run-time scripts are run through
shellcheck (if installed) in a unit test suite.  We just don't have the
time to do the same for all of the test scripts too.

However, for IP failover we depend on the Linux iproute2 "ip" command,
so that is unlikely to be portable anyway.  This is about to be
rewritten so it might be possible to contain the Linux-specific parts
fairly well so that people can write non-Linux alternatives.  That
remains to be seen...

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 12/22/2017 11:05 AM, Timur I. Bakeyev wrote:

> On Fri, Dec 22, 2017 at 9:35 AM, Uri Simchoni <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     On 12/22/2017 03:58 AM, Timur I. Bakeyev via samba-technical wrote:
>     > Yet another fixes to make self-test work out of the box on FreeBSD.
>     >
>     > Despite having '#!/bin/sh' in the test shell scripts there are some
>     > bashisms present,
>     > that make FreeBSD sh croak.
>     >
>     > I've run Debian's 'checkbashisms' script against test scripts and
>     tried to
>     > address most of it's (reasonable) warnings.
>     >
>     > Also, one failing test spotted the place where direct call to the
>     > 'ldbsearch' wasn't replaced with the configurable path.
>
>     We have another possible path - mark scripts which use bash syntax as
>     #!/bin/bash (or even "standardize" on bash).
>
>     There are tests which are truly bash-specific - I might have introduced
>     the first one and the bash dependency was discussed on the list (using
>     bash arrays - the shadow_copy2 test). I also (hours before this patch
>     submitted :)) updated the Samba wiki about the need to have bash to run
>     tests.
>
>     I personally find the bash syntax less error prone ([[ vs [ mostly) and
>     would prefer to keep the "bashisms" as bash scripts instead of shell
>     scripts, but I can see how there can be strong opinions to the other
>     side, so we should first solicit input on that and decide on the general
>     direction.
>
>
> Well, bash is evil and should be avoided, whenever it's possible, IMHO.
> In general if you care about any kind of portability it should be avoided.
>
> Unfortunately, we can't dictate here, so have to follow the path that Samba
> project will chose, but if you going to introduce such a dependency, please,
> remember that bash for FreeBSD is a foreign program, so it'll be in
> /usr/local/bin/bash. Hence you'll have to apply the same magic you used
> for the Perl.
>

As for the patches - RB+ me. Can I have another Team reviewer?

Other than that:
Just to make things clear - the Samba test suite policy has always been
not to assume that /bin/sh is bash. On the build server, /bin/sh is
dash. It appears the "bashisms" you pointed out never execute during
selftest (which means we don't have tests for adding/removing/modifying
shares via RPC...).

Having run checkbashisms myself now, there seem to be many more. Other
than false positives, there are:
1. uses of "echo -n"
2. unsafe echo with backlash (e.g. echo '\nFoo' - that's XSI-compliant)
3. uses of "echo -e"
4. 'command' with option other than -p (it's with -v which is XSI-compliant)

What do you make of those w/respect to FreeBSD? (seems like 1 and 3 are
supported, 2 and 4 are not...)

Thanks,
Uri

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via samba-technical wrote:

> Other than that:
> Just to make things clear - the Samba test suite policy has always been
> not to assume that /bin/sh is bash. On the build server, /bin/sh is
> dash. It appears the "bashisms" you pointed out never execute during
> selftest (which means we don't have tests for adding/removing/modifying
> shares via RPC...).

We do have tests for share management through RPC calls, see
the test in source3/script/tests/test_rpcclientsrvsvc.sh. If there are
bashisms, then it looks like they are no problem with Debian's dash, but
mainly with FreeBSD.

Christof

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 12/25/2017 05:07 AM, Christof Schmitt wrote:

> On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via samba-technical wrote:
>
>> Other than that:
>> Just to make things clear - the Samba test suite policy has always been
>> not to assume that /bin/sh is bash. On the build server, /bin/sh is
>> dash. It appears the "bashisms" you pointed out never execute during
>> selftest (which means we don't have tests for adding/removing/modifying
>> shares via RPC...).
>
> We do have tests for share management through RPC calls, see
> the test in source3/script/tests/test_rpcclientsrvsvc.sh. If there are
> bashisms, then it looks like they are no problem with Debian's dash, but
> mainly with FreeBSD.
>
> Christof
>

Ok, missed that one. Sorry. But dash doesn't support '[['. When I run
that (unfixed) test using dash, the server log shows:


smbd version 4.8.0pre1-DEVELOPERBUILD started.
Copyright Andrew Tridgell and the Samba Team 1992-2017
Test dummy executed!
daemon_ready: STATUS=daemon 'smbd' finished starting up and ready to
serve connections
/home/uri/s2/bin/smbchangeshare: 23: /home/uri/s2/bin/smbchangeshare:
[[: not found
/home/uri/s2/bin/smbchangeshare: 30: /home/uri/s2/bin/smbchangeshare:
[[: not found
/home/uri/s2/bin/smbchangeshare: 37: /home/uri/s2/bin/smbchangeshare:
[[: not found
/home/uri/s2/bin/smbchangeshare: 44: /home/uri/s2/bin/smbchangeshare:
[[: not found
delete_share_security: Failed to delete entry for share srvsvctest:
NT_STATUS_NOT_FOUND

... and the test as a whole passes.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Mon, Dec 25, 2017 at 06:55:51AM +0200, Uri Simchoni wrote:

> On 12/25/2017 05:07 AM, Christof Schmitt wrote:
> > On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via samba-technical wrote:
> >
> >> Other than that:
> >> Just to make things clear - the Samba test suite policy has always been
> >> not to assume that /bin/sh is bash. On the build server, /bin/sh is
> >> dash. It appears the "bashisms" you pointed out never execute during
> >> selftest (which means we don't have tests for adding/removing/modifying
> >> shares via RPC...).
> >
> > We do have tests for share management through RPC calls, see
> > the test in source3/script/tests/test_rpcclientsrvsvc.sh. If there are
> > bashisms, then it looks like they are no problem with Debian's dash, but
> > mainly with FreeBSD.
> >
> > Christof
> >
>
> Ok, missed that one. Sorry. But dash doesn't support '[['. When I run
> that (unfixed) test using dash, the server log shows:
>
>
> smbd version 4.8.0pre1-DEVELOPERBUILD started.
> Copyright Andrew Tridgell and the Samba Team 1992-2017
> Test dummy executed!
> daemon_ready: STATUS=daemon 'smbd' finished starting up and ready to
> serve connections
> /home/uri/s2/bin/smbchangeshare: 23: /home/uri/s2/bin/smbchangeshare:
> [[: not found
> /home/uri/s2/bin/smbchangeshare: 30: /home/uri/s2/bin/smbchangeshare:
> [[: not found
> /home/uri/s2/bin/smbchangeshare: 37: /home/uri/s2/bin/smbchangeshare:
> [[: not found
> /home/uri/s2/bin/smbchangeshare: 44: /home/uri/s2/bin/smbchangeshare:
> [[: not found
> delete_share_security: Failed to delete entry for share srvsvctest:
> NT_STATUS_NOT_FOUND
>
> ... and the test as a whole passes.

Yes, i did also some testing and saw the same. It looks like the effect
of the wrong operator in dash is that the error message "[[: not found"
is issued and the check is ignored. As a result, the overall test
succeeds, but it would fail to detect any errors from commands called
form the smb*share scripts.

We should simply fix them. +1 from me for the patch to use the correct [
operator.

Christof

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 12/26/2017 10:21 PM, Christof Schmitt wrote:

> On Mon, Dec 25, 2017 at 06:55:51AM +0200, Uri Simchoni wrote:
>> On 12/25/2017 05:07 AM, Christof Schmitt wrote:
>>> On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via samba-technical wrote:
>>>
>>>> Other than that:
>>>> Just to make things clear - the Samba test suite policy has always been
>>>> not to assume that /bin/sh is bash. On the build server, /bin/sh is
>>>> dash. It appears the "bashisms" you pointed out never execute during
>>>> selftest (which means we don't have tests for adding/removing/modifying
>>>> shares via RPC...).
>>>
>>> We do have tests for share management through RPC calls, see
>>> the test in source3/script/tests/test_rpcclientsrvsvc.sh. If there are
>>> bashisms, then it looks like they are no problem with Debian's dash, but
>>> mainly with FreeBSD.
>>>
>>> Christof
>>>
>>
>> Ok, missed that one. Sorry. But dash doesn't support '[['. When I run
>> that (unfixed) test using dash, the server log shows:
>>
>>
>> smbd version 4.8.0pre1-DEVELOPERBUILD started.
>> Copyright Andrew Tridgell and the Samba Team 1992-2017
>> Test dummy executed!
>> daemon_ready: STATUS=daemon 'smbd' finished starting up and ready to
>> serve connections
>> /home/uri/s2/bin/smbchangeshare: 23: /home/uri/s2/bin/smbchangeshare:
>> [[: not found
>> /home/uri/s2/bin/smbchangeshare: 30: /home/uri/s2/bin/smbchangeshare:
>> [[: not found
>> /home/uri/s2/bin/smbchangeshare: 37: /home/uri/s2/bin/smbchangeshare:
>> [[: not found
>> /home/uri/s2/bin/smbchangeshare: 44: /home/uri/s2/bin/smbchangeshare:
>> [[: not found
>> delete_share_security: Failed to delete entry for share srvsvctest:
>> NT_STATUS_NOT_FOUND
>>
>> ... and the test as a whole passes.
>
> Yes, i did also some testing and saw the same. It looks like the effect
> of the wrong operator in dash is that the error message "[[: not found"
> is issued and the check is ignored. As a result, the overall test
> succeeds, but it would fail to detect any errors from commands called
> form the smb*share scripts.
>
> We should simply fix them. +1 from me for the patch to use the correct [
> operator.
>
> Christof
>

Pushed that one (not the ldbsearch one).

Thanks,
Uri

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
Thanks, Uri!

But what is wrong with the ldbsearch one? It's obviously a bug in the
script and that exposes clearly, if you compile with external t* libs.

With regards,
Timur Bakeyev.


On Wed, Dec 27, 2017 at 6:03 AM, Uri Simchoni <[hidden email]> wrote:

> On 12/26/2017 10:21 PM, Christof Schmitt wrote:
> > On Mon, Dec 25, 2017 at 06:55:51AM +0200, Uri Simchoni wrote:
> >> On 12/25/2017 05:07 AM, Christof Schmitt wrote:
> >>> On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via
> samba-technical wrote:
> >>>
> >>>> Other than that:
> >>>> Just to make things clear - the Samba test suite policy has always
> been
> >>>> not to assume that /bin/sh is bash. On the build server, /bin/sh is
> >>>> dash. It appears the "bashisms" you pointed out never execute during
> >>>> selftest (which means we don't have tests for
> adding/removing/modifying
> >>>> shares via RPC...).
> >>>
> >>> We do have tests for share management through RPC calls, see
> >>> the test in source3/script/tests/test_rpcclientsrvsvc.sh. If there are
> >>> bashisms, then it looks like they are no problem with Debian's dash,
> but
> >>> mainly with FreeBSD.
> >>>
> >>> Christof
> >>>
> >>
> >> Ok, missed that one. Sorry. But dash doesn't support '[['. When I run
> >> that (unfixed) test using dash, the server log shows:
> >>
> >>
> >> smbd version 4.8.0pre1-DEVELOPERBUILD started.
> >> Copyright Andrew Tridgell and the Samba Team 1992-2017
> >> Test dummy executed!
> >> daemon_ready: STATUS=daemon 'smbd' finished starting up and ready to
> >> serve connections
> >> /home/uri/s2/bin/smbchangeshare: 23: /home/uri/s2/bin/smbchangeshare:
> >> [[: not found
> >> /home/uri/s2/bin/smbchangeshare: 30: /home/uri/s2/bin/smbchangeshare:
> >> [[: not found
> >> /home/uri/s2/bin/smbchangeshare: 37: /home/uri/s2/bin/smbchangeshare:
> >> [[: not found
> >> /home/uri/s2/bin/smbchangeshare: 44: /home/uri/s2/bin/smbchangeshare:
> >> [[: not found
> >> delete_share_security: Failed to delete entry for share srvsvctest:
> >> NT_STATUS_NOT_FOUND
> >>
> >> ... and the test as a whole passes.
> >
> > Yes, i did also some testing and saw the same. It looks like the effect
> > of the wrong operator in dash is that the error message "[[: not found"
> > is issued and the check is ignored. As a result, the overall test
> > succeeds, but it would fail to detect any errors from commands called
> > form the smb*share scripts.
> >
> > We should simply fix them. +1 from me for the patch to use the correct [
> > operator.
> >
> > Christof
> >
>
> Pushed that one (not the ldbsearch one).
>
> Thanks,
> Uri
>
Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 12/27/2017 07:28 AM, Timur I. Bakeyev wrote:
> Thanks, Uri!
>
> But what is wrong with the ldbsearch one? It's obviously a bug in the
> script and that exposes clearly, if you compile with external t* libs.
>
> With regards,
> Timur Bakeyev.
>

Nothing is wrong IMHO but it requires two reviewers. Please bear with us :)

Thanks,
Uri.

>
> On Wed, Dec 27, 2017 at 6:03 AM, Uri Simchoni <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     On 12/26/2017 10:21 PM, Christof Schmitt wrote:
>     > On Mon, Dec 25, 2017 at 06:55:51AM +0200, Uri Simchoni wrote:
>     >> On 12/25/2017 05:07 AM, Christof Schmitt wrote:
>     >>> On Sat, Dec 23, 2017 at 09:38:06PM +0200, Uri Simchoni via
>     samba-technical wrote:
>     >>>
>     >>>> Other than that:
>     >>>> Just to make things clear - the Samba test suite policy has
>     always been
>     >>>> not to assume that /bin/sh is bash. On the build server, /bin/sh is
>     >>>> dash. It appears the "bashisms" you pointed out never execute
>     during
>     >>>> selftest (which means we don't have tests for
>     adding/removing/modifying
>     >>>> shares via RPC...).
>     >>>
>     >>> We do have tests for share management through RPC calls, see
>     >>> the test in source3/script/tests/test_rpcclientsrvsvc.sh. If
>     there are
>     >>> bashisms, then it looks like they are no problem with Debian's
>     dash, but
>     >>> mainly with FreeBSD.
>     >>>
>     >>> Christof
>     >>>
>     >>
>     >> Ok, missed that one. Sorry. But dash doesn't support '[['. When I run
>     >> that (unfixed) test using dash, the server log shows:
>     >>
>     >>
>     >> smbd version 4.8.0pre1-DEVELOPERBUILD started.
>     >> Copyright Andrew Tridgell and the Samba Team 1992-2017
>     >> Test dummy executed!
>     >> daemon_ready: STATUS=daemon 'smbd' finished starting up and ready to
>     >> serve connections
>     >> /home/uri/s2/bin/smbchangeshare: 23: /home/uri/s2/bin/smbchangeshare:
>     >> [[: not found
>     >> /home/uri/s2/bin/smbchangeshare: 30: /home/uri/s2/bin/smbchangeshare:
>     >> [[: not found
>     >> /home/uri/s2/bin/smbchangeshare: 37: /home/uri/s2/bin/smbchangeshare:
>     >> [[: not found
>     >> /home/uri/s2/bin/smbchangeshare: 44: /home/uri/s2/bin/smbchangeshare:
>     >> [[: not found
>     >> delete_share_security: Failed to delete entry for share srvsvctest:
>     >> NT_STATUS_NOT_FOUND
>     >>
>     >> ... and the test as a whole passes.
>     >
>     > Yes, i did also some testing and saw the same. It looks like the
>     effect
>     > of the wrong operator in dash is that the error message "[[: not
>     found"
>     > is issued and the check is ignored. As a result, the overall test
>     > succeeds, but it would fail to detect any errors from commands called
>     > form the smb*share scripts.
>     >
>     > We should simply fix them. +1 from me for the patch to use the
>     correct [
>     > operator.
>     >
>     > Christof
>     >
>
>     Pushed that one (not the ldbsearch one).
>
>     Thanks,
>     Uri
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 2017-12-27 at 07:36 +0200 Uri Simchoni via samba-technical sent off:
> > But what is wrong with the ldbsearch one? It's obviously a bug in the
> > script and that exposes clearly, if you compile with external t* libs.
> >
> > With regards,
> > Timur Bakeyev.
> >
>
> Nothing is wrong IMHO but it requires two reviewers. Please bear with us :)

+1 from me

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On 01/02/2018 06:24 PM, Björn JACKE wrote:

> On 2017-12-27 at 07:36 +0200 Uri Simchoni via samba-technical sent off:
>>> But what is wrong with the ldbsearch one? It's obviously a bug in the
>>> script and that exposes clearly, if you compile with external t* libs.
>>>
>>> With regards,
>>> Timur Bakeyev.
>>>
>>
>> Nothing is wrong IMHO but it requires two reviewers. Please bear with us :)
>
> +1 from me
>

I tried pushing this and it consistently fails on
samba4.blackbox.dbcheck.release-4-0-0.

I suspect that the following patch is causing a test that previously
never ran to run, and fail:

--- a/testprogs/blackbox/dbcheck-oldrelease.sh
+++ b/testprogs/blackbox/dbcheck-oldrelease.sh
@@ -366,19 +366,19 @@ check_expected_after_deleted_objects() {
 }

 referenceprovision() {
-    if [ x$RELEASE == x"release-4-0-0" ]; then
+    if [ x$RELEASE = x"release-4-0-0" ]; then
         $PYTHON $BINDIR/samba-tool domain provision --server-role="dc"
--domain=SAMBA --host-name=ares --realm=${RELEASE}.samba.corp
--targetdir=$PREFIX_ABS/${RELEASE}_reference --use-ntvfs
--host-ip=127.0.0.1 --host-ip6=::1 --function-l
     fi
 }

 ldapcmp() {
-    if [ x$RELEASE == x"release-4-0-0" ]; then
+    if [ x$RELEASE = x"release-4-0-0" ]; then
          $PYTHON $BINDIR/samba-tool ldapcmp
tdb://$PREFIX_ABS/${RELEASE}_reference/private/sam.ldb
tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb --two --skip-missing-dn
--filter=dnsRecord
     fi
 }

 ldapcmp_sd() {
-    if [ x$RELEASE == x"release-4-0-0" ]; then
+    if [ x$RELEASE = x"release-4-0-0" ]; then
         $PYTHON $BINDIR/samba-tool ldapcmp
tdb://$PREFIX_ABS/${RELEASE}_reference/private/sam.ldb
tdb://$PREFIX_ABS/${RELEASE}/private/sam.ldb --two --sd --skip-missing-dn
     fi
 }


It also consistently fails without the patch on my bash-based autobuild
machine (consistent with bash understanding ==), but I can't yet explain
why it started failing, since this is an old test and I've been using
bash for autobuild all along.

Andrew, can you please have a look?

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Wed, 3 Jan 2018 23:45:55 +0200, Uri Simchoni via samba-technical
<[hidden email]> wrote:

> On 01/02/2018 06:24 PM, Björn JACKE wrote:
> > On 2017-12-27 at 07:36 +0200 Uri Simchoni via samba-technical sent off:  
>  [...]  
> >>
> >> Nothing is wrong IMHO but it requires two reviewers. Please bear with us :)  
> >
> > +1 from me
> >  
>
> I tried pushing this and it consistently fails on
> samba4.blackbox.dbcheck.release-4-0-0.
>
> I suspect that the following patch is causing a test that previously
> never ran to run, and fail:
>
> --- a/testprogs/blackbox/dbcheck-oldrelease.sh
> +++ b/testprogs/blackbox/dbcheck-oldrelease.sh
> @@ -366,19 +366,19 @@ check_expected_after_deleted_objects() {
>  }
>
>  referenceprovision() {
> -    if [ x$RELEASE == x"release-4-0-0" ]; then
> +    if [ x$RELEASE = x"release-4-0-0" ]; then
>          $PYTHON $BINDIR/samba-tool domain provision --server-role="dc"
> [...]

I'm struggling to understand why we have code that uses the

  [ x$FOO = xbar ]

idiom.

My understanding is that this used to protect against the expansion of
$FOO starting with a '-' and tricking the test command into doing the
wrong thing.  Are there still any shells around that have this problem?

If not, surely when this code is touched it should be updated to use

  [ "$FOO" = "bar" ]

so that the code says what is meant?  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
The patch removes bashisms, not sh'isms :)

I'll add that if I'm the one to eventually push this fix.
Thanks,
Uri

On 01/04/2018 06:22 AM, Martin Schwenke via samba-technical wrote:

> On Wed, 3 Jan 2018 23:45:55 +0200, Uri Simchoni via samba-technical
> <[hidden email]> wrote:
>
>> On 01/02/2018 06:24 PM, Björn JACKE wrote:
>>> On 2017-12-27 at 07:36 +0200 Uri Simchoni via samba-technical sent off:  
>>  [...]  
>>>>
>>>> Nothing is wrong IMHO but it requires two reviewers. Please bear with us :)  
>>>
>>> +1 from me
>>>  
>>
>> I tried pushing this and it consistently fails on
>> samba4.blackbox.dbcheck.release-4-0-0.
>>
>> I suspect that the following patch is causing a test that previously
>> never ran to run, and fail:
>>
>> --- a/testprogs/blackbox/dbcheck-oldrelease.sh
>> +++ b/testprogs/blackbox/dbcheck-oldrelease.sh
>> @@ -366,19 +366,19 @@ check_expected_after_deleted_objects() {
>>  }
>>
>>  referenceprovision() {
>> -    if [ x$RELEASE == x"release-4-0-0" ]; then
>> +    if [ x$RELEASE = x"release-4-0-0" ]; then
>>          $PYTHON $BINDIR/samba-tool domain provision --server-role="dc"
>> [...]
>
> I'm struggling to understand why we have code that uses the
>
>   [ x$FOO = xbar ]
>
> idiom.
>
> My understanding is that this used to protect against the expansion of
> $FOO starting with a '-' and tricking the test command into doing the
> wrong thing.  Are there still any shells around that have this problem?
>
> If not, surely when this code is touched it should be updated to use
>
>   [ "$FOO" = "bar" ]
>
> so that the code says what is meant?  :-)
>
> peace & happiness,
> martin
>

Reply | Threaded
Open this post in threaded view
|

Re: Remove (some) bashisms from the test scripts

Samba - samba-technical mailing list
On Thu, 4 Jan 2018 06:43:10 +0200, Uri Simchoni <[hidden email]> wrote:

> The patch removes bashisms, not sh'isms :)
>
> I'll add that if I'm the one to eventually push this fix.

:-)

Separate patch?  ;-)

peace & happiness,
martin