[PATCH] update autogen-waf.sh

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

[PATCH] update autogen-waf.sh

Samba - samba-technical mailing list

OK, here is the first one, this is basically just running the script
through 'shellcheck' and fixing any errors found, it should not change
how the script works.

Rowland

autogen-waf.sh-Remove-all-errors-reported-by-shellch.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
Hi Rowland,

On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
samba-technical <[hidden email]> wrote:

> OK, here is the first one, this is basically just running the script
> through 'shellcheck' and fixing any errors found, it should not change
> how the script works.

It all looks good.  Can I suggest a minor improvement in a few places?

Instead of doing:

  "$p"/Makefile

it is probably better to do:

  "$p/Makefile"

That way the quotes are around the whole string and future edits are
less likely to be fragile.

However, you need to be careful with examples like this:

  "$p"/include/config*.h*

If you double-quote the wildcards then they don't get expanded.  There
are a few answers to this, including:

  "$p/include/config"*.h*

Deciding where to end the quotes is quite subjective.  :-)

Personally, I tend to "over-punctuate" my shell scripts.  I would write
the first example as:

  "${p}/Makefile"

While the braces aren't necessary in this case, I believe that they
make the variable expansion more obvious... especially in cases where
there are multiple expansions in a string.  The only place I don't use
the braces is when the variable expansion is the whole string, as in
"$p".

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 11 Aug 2017 18:53:16 +1000
Martin Schwenke <[hidden email]> wrote:

> Hi Rowland,
>
> On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
> samba-technical <[hidden email]> wrote:
>
> > OK, here is the first one, this is basically just running the script
> > through 'shellcheck' and fixing any errors found, it should not
> > change how the script works.
>
> It all looks good.  Can I suggest a minor improvement in a few places?
>
> Instead of doing:
>
>   "$p"/Makefile
>
> it is probably better to do:
>
>   "$p/Makefile"
>
> That way the quotes are around the whole string and future edits are
> less likely to be fragile.
>
> However, you need to be careful with examples like this:
>
>   "$p"/include/config*.h*
>
> If you double-quote the wildcards then they don't get expanded.  There
> are a few answers to this, including:
>
>   "$p/include/config"*.h*
>
> Deciding where to end the quotes is quite subjective.  :-)
>
> Personally, I tend to "over-punctuate" my shell scripts.  I would
> write the first example as:
>
>   "${p}/Makefile"

I understand all this Martin and as I said earlier, where do you stop ?
>
> While the braces aren't necessary in this case, I believe that they
> make the variable expansion more obvious... especially in cases where
> there are multiple expansions in a string.  The only place I don't use
> the braces is when the variable expansion is the whole string, as in
> "$p".

I normally use them in every case, just in case ;-) but in this case,
I am just following 'shellcheck' and fixing obvious errors.

I think I will start again, follow 'shellcheck' but add what I would
normally do and also fix any errors. I will also take on board what you
say about the ctdb tests and ignore them.

Thanks

Rowland



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 11 Aug 2017 11:21:00 +0100, Rowland Penny via samba-technical
<[hidden email]> wrote:

> On Fri, 11 Aug 2017 18:53:16 +1000
> Martin Schwenke <[hidden email]> wrote:
>
> > Hi Rowland,
> >
> > On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
> > samba-technical <[hidden email]> wrote:
> >  
> > > OK, here is the first one, this is basically just running the script
> > > through 'shellcheck' and fixing any errors found, it should not
> > > change how the script works.  
> >
> > It all looks good.  Can I suggest a minor improvement in a few places?
> >
> > Instead of doing:
> >
> >   "$p"/Makefile
> >
> > it is probably better to do:
> >
> >   "$p/Makefile"
> >
> > That way the quotes are around the whole string and future edits are
> > less likely to be fragile.
> >
> > However, you need to be careful with examples like this:
> >
> >   "$p"/include/config*.h*
> >
> > If you double-quote the wildcards then they don't get expanded.  There
> > are a few answers to this, including:
> >
> >   "$p/include/config"*.h*
> >
> > Deciding where to end the quotes is quite subjective.  :-)
> >
> > Personally, I tend to "over-punctuate" my shell scripts.  I would
> > write the first example as:
> >
> >   "${p}/Makefile"  
>
> I understand all this Martin and as I said earlier, where do you stop ?

Cool.  I'm just over-communicating given the usual time turnaround
for long distance email conversations...  :-)

> > While the braces aren't necessary in this case, I believe that they
> > make the variable expansion more obvious... especially in cases where
> > there are multiple expansions in a string.  The only place I don't use
> > the braces is when the variable expansion is the whole string, as in
> > "$p".  
>
> I normally use them in every case, just in case ;-) but in this case,
> I am just following 'shellcheck' and fixing obvious errors.

Yeah, shellcheck often provides hints that the code could be better.

> I think I will start again, follow 'shellcheck' but add what I would
> normally do and also fix any errors. I will also take on board what you
> say about the ctdb tests and ignore them.

I think that's a good move.

That said, if you really do want to attack the CTDB tests then I think
the order could be:

1. Fix shellcheck hits in unit tests
2. Fix shellcheck hits in the (bash) integration tests
3. Convert bash to POSIX sh

However, it is a major undertaking...

TV time.  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 11 Aug 2017 20:32:11 +1000
Martin Schwenke <[hidden email]> wrote:

> On Fri, 11 Aug 2017 11:21:00 +0100, Rowland Penny via samba-technical
> <[hidden email]> wrote:
>
> > On Fri, 11 Aug 2017 18:53:16 +1000
> > Martin Schwenke <[hidden email]> wrote:
> >
> > > Hi Rowland,
> > >
> > > On Thu, 10 Aug 2017 14:21:34 +0100, Rowland Penny via
> > > samba-technical <[hidden email]> wrote:
> > >  
> > > > OK, here is the first one, this is basically just running the
> > > > script through 'shellcheck' and fixing any errors found, it
> > > > should not change how the script works.  
> > >
> > > It all looks good.  Can I suggest a minor improvement in a few
> > > places?
> > >
> > > Instead of doing:
> > >
> > >   "$p"/Makefile
> > >
> > > it is probably better to do:
> > >
> > >   "$p/Makefile"
> > >
> > > That way the quotes are around the whole string and future edits
> > > are less likely to be fragile.
> > >
> > > However, you need to be careful with examples like this:
> > >
> > >   "$p"/include/config*.h*
> > >
> > > If you double-quote the wildcards then they don't get expanded.
> > > There are a few answers to this, including:
> > >
> > >   "$p/include/config"*.h*
> > >
> > > Deciding where to end the quotes is quite subjective.  :-)
> > >
> > > Personally, I tend to "over-punctuate" my shell scripts.  I would
> > > write the first example as:
> > >
> > >   "${p}/Makefile"  
> >
> > I understand all this Martin and as I said earlier, where do you
> > stop ?
>
> Cool.  I'm just over-communicating given the usual time turnaround
> for long distance email conversations...  :-)
>
> > > While the braces aren't necessary in this case, I believe that
> > > they make the variable expansion more obvious... especially in
> > > cases where there are multiple expansions in a string.  The only
> > > place I don't use the braces is when the variable expansion is
> > > the whole string, as in "$p".  
> >
> > I normally use them in every case, just in case ;-) but in this
> > case, I am just following 'shellcheck' and fixing obvious errors.
>
> Yeah, shellcheck often provides hints that the code could be better.
>
> > I think I will start again, follow 'shellcheck' but add what I would
> > normally do and also fix any errors. I will also take on board what
> > you say about the ctdb tests and ignore them.
>
> I think that's a good move.
>
> That said, if you really do want to attack the CTDB tests then I think
> the order could be:
>
> 1. Fix shellcheck hits in unit tests
> 2. Fix shellcheck hits in the (bash) integration tests
> 3. Convert bash to POSIX sh
>
> However, it is a major undertaking...
>
> TV time.  :-)
>
> peace & happiness,
> martin

OK, I have been working through samba-master looking at .sh files and I
am now wondering whether this is where to start. The problem is that I
am coming across files I didn't know exist and I am now thinking that
some of these would be better fixed by deleting them. Most of these are
in the 'examples' dir, why is Samba in the 'telling how to create rpms'
business ? (especially as it doesn't seem to work any more) why do we
have an old 'auto.smb' file ? etc etc

Comments please.

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
Hi Rowland,

> OK, I have been working through samba-master looking at .sh files and I
> am now wondering whether this is where to start. The problem is that I
> am coming across files I didn't know exist and I am now thinking that
> some of these would be better fixed by deleting them.

Yes, we have a lot of ancient stuff we don't use/need anymore
and I guess autogen-waf-sh is one of them...

metze



signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Are these used any more? was [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 18 Aug 2017 10:37:11 +0200
Stefan Metzmacher <[hidden email]> wrote:

> Hi Rowland,
>
> > OK, I have been working through samba-master looking at .sh files
> > and I am now wondering whether this is where to start. The problem
> > is that I am coming across files I didn't know exist and I am now
> > thinking that some of these would be better fixed by deleting them.
>
> Yes, we have a lot of ancient stuff we don't use/need anymore
> and I guess autogen-waf-sh is one of them...
>
> metze
>
>

OK, Are any of these used by anybody, or can they be removed ?

samba-master/buildtools/compare_config_h4.sh
samba-master/buildtools/compare_generated.sh
samba-master/buildtools/wafsamba/test_duplicate_symbol.sh
samba-master/buildtools/scripts/autogen-waf.sh
samba-master/buildtools/scripts/abi_gen.sh
samba-master/buildtools/testwaf.sh
samba-master/buildtools/compare_install.sh

samba-master/examples/autofs/auto.smb
samba-master/examples/scripts/idmap/idmap_nis.sh
samba-master/examples/scripts/mount/mount.smbfs
samba-master/examples/scripts/wins_hook/dns_update
samba-master/examples/systemtap/generate-winbindd.stp.sh

samba-master/packaging/RHEL/setup/filter-requires-samba.sh
samba-master/packaging/RHEL/makerpms.git.sh
samba-master/packaging/RHEL-CTDB/makespec.sh
samba-master/packaging/RHEL-CTDB/makerpms.sh
samba-master/packaging/RHEL-CTDB/setup/filter-requires-samba.sh
samba-master/packaging/Solaris/makepkg.sh

samba-master/source3/script/updatesmbpasswd.sh
samba-master/source3/script/mksmbpasswd.sh
samba-master/source3/script/format_indent.sh
samba-master/source3/script/mksyms.sh
samba-master/source3/script/mknissmbpasswd.sh
samba-master/source3/script/mknissmbpwdtbl.sh

They all seem extremely old, but I am sure some of these are still
required, but which ones ?

Rowland




Reply | Threaded
Open this post in threaded view
|

Re: Are these used any more? was [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 2017-08-18 at 13:22 +0100, Rowland Penny via samba-technical
wrote:

> On Fri, 18 Aug 2017 10:37:11 +0200
> Stefan Metzmacher <[hidden email]> wrote:
>
> > Hi Rowland,
> >
> > > OK, I have been working through samba-master looking at .sh files
> > > and I am now wondering whether this is where to start. The problem
> > > is that I am coming across files I didn't know exist and I am now
> > > thinking that some of these would be better fixed by deleting them.
> >
> > Yes, we have a lot of ancient stuff we don't use/need anymore
> > and I guess autogen-waf-sh is one of them...
> >
> > metze
> >
> >
>
> OK, Are any of these used by anybody, or can they be removed ?
>
> samba-master/buildtools/compare_config_h4.sh
> samba-master/buildtools/compare_generated.sh
> samba-master/buildtools/wafsamba/test_duplicate_symbol.sh
> samba-master/buildtools/scripts/autogen-waf.sh
> samba-master/buildtools/scripts/abi_gen.sh
> samba-master/buildtools/testwaf.sh
> samba-master/buildtools/compare_install.sh
>
> samba-master/examples/autofs/auto.smb
> samba-master/examples/scripts/idmap/idmap_nis.sh
> samba-master/examples/scripts/mount/mount.smbfs
> samba-master/examples/scripts/wins_hook/dns_update
> samba-master/examples/systemtap/generate-winbindd.stp.sh
>
> samba-master/packaging/RHEL/setup/filter-requires-samba.sh
> samba-master/packaging/RHEL/makerpms.git.sh
> samba-master/packaging/RHEL-CTDB/makespec.sh
> samba-master/packaging/RHEL-CTDB/makerpms.sh
> samba-master/packaging/RHEL-CTDB/setup/filter-requires-samba.sh
> samba-master/packaging/Solaris/makepkg.sh
>
> samba-master/source3/script/updatesmbpasswd.sh
> samba-master/source3/script/mksmbpasswd.sh
> samba-master/source3/script/format_indent.sh
> samba-master/source3/script/mksyms.sh
> samba-master/source3/script/mknissmbpasswd.sh
> samba-master/source3/script/mknissmbpwdtbl.sh
>
> They all seem extremely old, but I am sure some of these are still
> required, but which ones ?

To get a short list, first do a 'git grep' over the source tree for the
base name of each script.

$ git grep abi_gen.sh
buildtools/wafsamba/samba_abi.py:    abi_gen = os.path.join(topsrc,
'buildtools/scripts/abi_gen.sh')

Once you remove the scripts that show up there, then you have a smaller
list that is easier for another developer to check manually.

Thanks!

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: Are these used any more? was [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Sat, 19 Aug 2017 08:08:48 +1200
Andrew Bartlett <[hidden email]> wrote:

> On Fri, 2017-08-18 at 13:22 +0100, Rowland Penny via samba-technical
> wrote:
> > On Fri, 18 Aug 2017 10:37:11 +0200
> > Stefan Metzmacher <[hidden email]> wrote:
> >
> > > Hi Rowland,
> > >
> > > > OK, I have been working through samba-master looking at .sh
> > > > files and I am now wondering whether this is where to start.
> > > > The problem is that I am coming across files I didn't know
> > > > exist and I am now thinking that some of these would be better
> > > > fixed by deleting them.
> > >
> > > Yes, we have a lot of ancient stuff we don't use/need anymore
> > > and I guess autogen-waf-sh is one of them...
> > >
> > > metze
> > >
> > >
> >
> > OK, Are any of these used by anybody, or can they be removed ?
> >
> > samba-master/buildtools/compare_config_h4.sh
> > samba-master/buildtools/compare_generated.sh
> > samba-master/buildtools/wafsamba/test_duplicate_symbol.sh
> > samba-master/buildtools/scripts/autogen-waf.sh
> > samba-master/buildtools/scripts/abi_gen.sh
> > samba-master/buildtools/testwaf.sh
> > samba-master/buildtools/compare_install.sh
> >
> > samba-master/examples/autofs/auto.smb
> > samba-master/examples/scripts/idmap/idmap_nis.sh
> > samba-master/examples/scripts/mount/mount.smbfs
> > samba-master/examples/scripts/wins_hook/dns_update
> > samba-master/examples/systemtap/generate-winbindd.stp.sh
> >
> > samba-master/packaging/RHEL/setup/filter-requires-samba.sh
> > samba-master/packaging/RHEL/makerpms.git.sh
> > samba-master/packaging/RHEL-CTDB/makespec.sh
> > samba-master/packaging/RHEL-CTDB/makerpms.sh
> > samba-master/packaging/RHEL-CTDB/setup/filter-requires-samba.sh
> > samba-master/packaging/Solaris/makepkg.sh
> >
> > samba-master/source3/script/updatesmbpasswd.sh
> > samba-master/source3/script/mksmbpasswd.sh
> > samba-master/source3/script/format_indent.sh
> > samba-master/source3/script/mksyms.sh
> > samba-master/source3/script/mknissmbpasswd.sh
> > samba-master/source3/script/mknissmbpwdtbl.sh
> >
> > They all seem extremely old, but I am sure some of these are still
> > required, but which ones ?
>
> To get a short list, first do a 'git grep' over the source tree for
> the base name of each script.
>
> $ git grep abi_gen.sh
> buildtools/wafsamba/samba_abi.py:    abi_gen = os.path.join(topsrc,
> 'buildtools/scripts/abi_gen.sh')
>
> Once you remove the scripts that show up there, then you have a
> smaller list that is easier for another developer to check manually.
>
> Thanks!
>
> Andrew Bartlett

I think I understand what you are saying, if I run the command you
suggest with each base name and get a response showing something else is
using it, remove from the list. Once finished, post the remainder (if
any)

Rowland

Reply | Threaded
Open this post in threaded view
|

Re: Are these used any more? was [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
On Fri, 2017-08-18 at 21:23 +0100, Rowland Penny wrote:
>
> I think I understand what you are saying, if I run the command you
> suggest with each base name and get a response showing something else is
> using it, remove from the list. Once finished, post the remainder (if
> any)

Yes.

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: Are these used any more? was [PATCH] update autogen-waf.sh

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 18 Aug 2017 21:23:14 +0100
Rowland Penny via samba-technical <[hidden email]>
wrote:

> On Sat, 19 Aug 2017 08:08:48 +1200
> Andrew Bartlett <[hidden email]> wrote:
>
> > On Fri, 2017-08-18 at 13:22 +0100, Rowland Penny via samba-technical
> > wrote:
> > > On Fri, 18 Aug 2017 10:37:11 +0200
> > > Stefan Metzmacher <[hidden email]> wrote:
> > >
> > > > Hi Rowland,
> > > >
> > > > > OK, I have been working through samba-master looking at .sh
> > > > > files and I am now wondering whether this is where to start.
> > > > > The problem is that I am coming across files I didn't know
> > > > > exist and I am now thinking that some of these would be better
> > > > > fixed by deleting them.
> > > >
> > > > Yes, we have a lot of ancient stuff we don't use/need anymore
> > > > and I guess autogen-waf-sh is one of them...
> > > >
> > > > metze
> > > >
> > > >
> > >
> > > OK, Are any of these used by anybody, or can they be removed ?
> > >
> > > samba-master/buildtools/compare_config_h4.sh
> > > samba-master/buildtools/compare_generated.sh
> > > samba-master/buildtools/wafsamba/test_duplicate_symbol.sh
> > > samba-master/buildtools/scripts/autogen-waf.sh
> > > samba-master/buildtools/scripts/abi_gen.sh
> > > samba-master/buildtools/testwaf.sh
> > > samba-master/buildtools/compare_install.sh
> > >
> > > samba-master/examples/autofs/auto.smb
> > > samba-master/examples/scripts/idmap/idmap_nis.sh
> > > samba-master/examples/scripts/mount/mount.smbfs
> > > samba-master/examples/scripts/wins_hook/dns_update
> > > samba-master/examples/systemtap/generate-winbindd.stp.sh
> > >
> > > samba-master/packaging/RHEL/setup/filter-requires-samba.sh
> > > samba-master/packaging/RHEL/makerpms.git.sh
> > > samba-master/packaging/RHEL-CTDB/makespec.sh
> > > samba-master/packaging/RHEL-CTDB/makerpms.sh
> > > samba-master/packaging/RHEL-CTDB/setup/filter-requires-samba.sh
> > > samba-master/packaging/Solaris/makepkg.sh
> > >
> > > samba-master/source3/script/updatesmbpasswd.sh
> > > samba-master/source3/script/mksmbpasswd.sh
> > > samba-master/source3/script/format_indent.sh
> > > samba-master/source3/script/mksyms.sh
> > > samba-master/source3/script/mknissmbpasswd.sh
> > > samba-master/source3/script/mknissmbpwdtbl.sh
> > >
> > > They all seem extremely old, but I am sure some of these are still
> > > required, but which ones ?
> >
> > To get a short list, first do a 'git grep' over the source tree for
> > the base name of each script.
> >
> > $ git grep abi_gen.sh
> > buildtools/wafsamba/samba_abi.py:    abi_gen = os.path.join(topsrc,
> > 'buildtools/scripts/abi_gen.sh')
> >
> > Once you remove the scripts that show up there, then you have a
> > smaller list that is easier for another developer to check
> > manually.
> >
> > Thanks!
> >
> > Andrew Bartlett
>
> I think I understand what you are saying, if I run the command you
> suggest with each base name and get a response showing something else
> is using it, remove from the list. Once finished, post the remainder
> (if any)
>
> Rowland
>

OK, here is a shortened list after following Andrews instructions, are
any of these still any use ?

buildtools/compare_config_h4.sh
buildtools/compare_generated.sh
buildtools/testwaf.sh
buildtools/compare_install.sh

examples/autofs/auto.smb
examples/scripts/idmap/idmap_nis.sh
examples/scripts/mount/mount.smbfs
examples/systemtap/generate-winbindd.stp.sh

packaging/RHEL/setup/filter-requires-samba.sh
packaging/RHEL/makerpms.git.sh
packaging/RHEL-CTDB/makespec.sh
packaging/RHEL-CTDB/makerpms.sh
packaging/RHEL-CTDB/setup/filter-requires-samba.sh
packaging/Solaris/makepkg.sh

source3/script/updatesmbpasswd.sh
source3/script/mksyms.sh
source3/script/mknissmbpasswd.sh
source3/script/mknissmbpwdtbl.sh

Rowland