[PATCH] tools/configure: drop BASH configure variable

Andrew Cooper posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200626170038.27650-1-andrew.cooper3@citrix.com
Maintainers: Wei Liu <wl@xen.org>, Daniel De Graaf <dgdegra@tycho.nsa.gov>, Ian Jackson <ian.jackson@eu.citrix.com>
config/Tools.mk.in                      | 1 -
tools/configure.ac                      | 1 -
xen/xsm/flask/Makefile                  | 8 ++------
xen/xsm/flask/policy/mkaccess_vector.sh | 0
xen/xsm/flask/policy/mkflask.sh         | 0
5 files changed, 2 insertions(+), 8 deletions(-)
mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh

[PATCH] tools/configure: drop BASH configure variable

Posted by Andrew Cooper 2 weeks ago
This is a weird variable to have in the first place.  The only user of it is
XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
run with this are shebang sh anyway, so don't need bash in the first place.

Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
CONFIG_SHELL, and drop the $BASH variable to prevent further use.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
CC: Paul Durrant <paul@xen.org>

./autogen.sh needs rerunning on commit.

RFC for 4.14.  This is a cleanup to the build system.

There is a separate check for [BASH] in the configure script, which is
checking the requirement for the Linux hotplug scripts.  Really, that is a
runtime requirement not a build time requirement, and it is rude to require
bash in a build environment on this basis.  IMO, that check wants dropping as
well.
---
 config/Tools.mk.in                      | 1 -
 tools/configure.ac                      | 1 -
 xen/xsm/flask/Makefile                  | 8 ++------
 xen/xsm/flask/policy/mkaccess_vector.sh | 0
 xen/xsm/flask/policy/mkflask.sh         | 0
 5 files changed, 2 insertions(+), 8 deletions(-)
 mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
 mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh

diff --git a/config/Tools.mk.in b/config/Tools.mk.in
index 23df47af8d..48bd9ab731 100644
--- a/config/Tools.mk.in
+++ b/config/Tools.mk.in
@@ -12,7 +12,6 @@ PYTHON              := @PYTHON@
 PYTHON_PATH         := @PYTHONPATH@
 PY_NOOPT_CFLAGS     := @PY_NOOPT_CFLAGS@
 PERL                := @PERL@
-BASH                := @BASH@
 XGETTTEXT           := @XGETTEXT@
 AS86                := @AS86@
 LD86                := @LD86@
diff --git a/tools/configure.ac b/tools/configure.ac
index 9d126b7a14..6614a4f130 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -297,7 +297,6 @@ AC_ARG_VAR([PYTHON], [Path to the Python parser])
 AC_ARG_VAR([PERL], [Path to Perl parser])
 AC_ARG_VAR([BISON], [Path to Bison parser generator])
 AC_ARG_VAR([FLEX], [Path to Flex lexical analyser generator])
-AC_ARG_VAR([BASH], [Path to bash shell])
 AC_ARG_VAR([XGETTEXT], [Path to xgetttext tool])
 AC_ARG_VAR([AS86], [Path to as86 tool])
 AC_ARG_VAR([LD86], [Path to ld86 tool])
diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
index 07f36d075d..ba8f31a02c 100644
--- a/xen/xsm/flask/Makefile
+++ b/xen/xsm/flask/Makefile
@@ -8,10 +8,6 @@ CFLAGS-y += -I./include
 
 AWK = awk
 
-CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
-          else if [ -x /bin/bash ]; then echo /bin/bash; \
-          else echo sh; fi ; fi)
-
 FLASK_H_DEPEND = policy/security_classes policy/initial_sids
 AV_H_DEPEND = policy/access_vectors
 
@@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
 
 mkflask := policy/mkflask.sh
 quiet_cmd_mkflask = MKFLASK $@
-cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
+cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
 
 $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
 	$(call if_changed,mkflask)
 
 mkaccess := policy/mkaccess_vector.sh
 quiet_cmd_mkaccess = MKACCESS VECTOR $@
-cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
+cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)
 
 $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
 	$(call if_changed,mkaccess)
diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
old mode 100644
new mode 100755
diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
old mode 100644
new mode 100755
-- 
2.11.0


[PATCH] tools/configure: drop BASH configure variable

Posted by Ian Jackson 1 week ago
Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.

Thanks for this cleanup.  I agree with the basic idea.

However, did you run these scripts with dash, or review them, to check
for bashisms ?  It is not unusual to find scripts which need bash but
which mistaken have #!/bin/sh - especially if the usual arrangements
for running them always use bash anyway.  So the presence of the
#!/bin/sh is only rather weak evidence that these scripts will be fine
when run with sh.

> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.

Since the build currently uses bash for these, a more neutral change
would be to change to #!/bin/bash at the same time.

> RFC for 4.14.  This is a cleanup to the build system.

I see this already has a release-ack.  However, I would not have
recommended granting one at least on the basis of the description
above.

I agree that this is cleanup.  But the current situation is not buggy.
I'm not sure exactly what the release criteria are but ISTM that this
patch adds risk to the release rather than removing it.

Thanks for your attention.

Ian.

Re: [PATCH] tools/configure: drop BASH configure variable

Posted by Andrew Cooper 1 week ago
On 29/06/2020 14:34, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
>> This is a weird variable to have in the first place.  The only user of it is
>> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
>> run with this are shebang sh anyway, so don't need bash in the first place.
> Thanks for this cleanup.  I agree with the basic idea.
>
> However, did you run these scripts with dash, or review them, to check
> for bashisms ?

Yes, to all of the above.

They are both very thin wrappers (doing some argument shuffling) around
large AWK scripts.

>> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
>> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> Since the build currently uses bash for these, a more neutral change
> would be to change to #!/bin/bash at the same time.

That will break FreeBSD, which has no `bash` in sight.

>> RFC for 4.14.  This is a cleanup to the build system.
> I see this already has a release-ack.  However, I would not have
> recommended granting one at least on the basis of the description
> above.
>
> I agree that this is cleanup.  But the current situation is not buggy.
> I'm not sure exactly what the release criteria are but ISTM that this
> patch adds risk to the release rather than removing it.

I agree that the current state of play isn't a major issue, but having
./configure check for bash is buggy for both uses.

~Andrew

RE: [PATCH] tools/configure: drop BASH configure variable

Posted by Paul Durrant 1 week ago
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 29 June 2020 14:51
> To: Ian Jackson <ian.jackson@citrix.com>
> Cc: Xen-devel <xen-devel@lists.xenproject.org>; Wei Liu <wl@xen.org>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Paul Durrant <paul@xen.org>
> Subject: Re: [PATCH] tools/configure: drop BASH configure variable
> 
> On 29/06/2020 14:34, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> >> This is a weird variable to have in the first place.  The only user of it is
> >> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> >> run with this are shebang sh anyway, so don't need bash in the first place.
> > Thanks for this cleanup.  I agree with the basic idea.
> >
> > However, did you run these scripts with dash, or review them, to check
> > for bashisms ?
> 
> Yes, to all of the above.
> 
> They are both very thin wrappers (doing some argument shuffling) around
> large AWK scripts.
> 
> >> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> >> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> > Since the build currently uses bash for these, a more neutral change
> > would be to change to #!/bin/bash at the same time.
> 
> That will break FreeBSD, which has no `bash` in sight.
> 
> >> RFC for 4.14.  This is a cleanup to the build system.
> > I see this already has a release-ack.  However, I would not have
> > recommended granting one at least on the basis of the description
> > above.
> >
> > I agree that this is cleanup.  But the current situation is not buggy.
> > I'm not sure exactly what the release criteria are but ISTM that this
> > patch adds risk to the release rather than removing it.
> 
> I agree that the current state of play isn't a major issue, but having
> ./configure check for bash is buggy for both uses.
> 

Even if it is not buggy, it is pointless complexity since FreeBSD would clearly have been broken all this time had there been bash-isms in the scripts.

  Paul

> ~Andrew


Re: [PATCH] tools/configure: drop BASH configure variable

Posted by Jan Beulich 1 week ago
On 26.06.2020 19:00, Andrew Cooper wrote:
> @@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
>  
>  mkflask := policy/mkflask.sh
>  quiet_cmd_mkflask = MKFLASK $@
> -cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> +cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)

This and ...

>  $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
>  	$(call if_changed,mkflask)
>  
>  mkaccess := policy/mkaccess_vector.sh
>  quiet_cmd_mkaccess = MKACCESS VECTOR $@
> -cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
> +cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)

... this should still use $(SHELL) though, as ...

> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> old mode 100644
> new mode 100755
> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> old mode 100644
> new mode 100755

... this may or may not take effect on the file system the sources
are stored on.

Jan

Re: [PATCH] tools/configure: drop BASH configure variable

Posted by Ian Jackson 1 week ago
Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
> On 26.06.2020 19:00, Andrew Cooper wrote:
> > diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> > old mode 100644
> > new mode 100755
> 
> ... this may or may not take effect on the file system the sources
> are stored on.

In what circumstances might this not take effect ?

IME all changes to files are properly replicated by git.  Tarball
distributions replicate the permissions of course.

The only difficulty would be if this change were to be carried as a
patch somewhere, by a downstream, but that seems unlikely, and can be
avoided by that downstream not taking this patch.

Ian.

Re: [PATCH] tools/configure: drop BASH configure variable

Posted by Jan Beulich 1 week ago
On 29.06.2020 14:05, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>> On 26.06.2020 19:00, Andrew Cooper wrote:
>>> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
>>> old mode 100644
>>> new mode 100755
>>
>> ... this may or may not take effect on the file system the sources
>> are stored on.
> 
> In what circumstances might this not take effect ?

When the file system is incapable of recording execute permissions?
It has been a common workaround for this in various projects that
I've worked with to use $(SHELL) to account for that, so the actual
permissions from the fs don't matter. (There may be mount options
to make everything executable on such file systems, but people may
be hesitant to use them.)

Jan

> IME all changes to files are properly replicated by git.  Tarball
> distributions replicate the permissions of course.
> 
> The only difficulty would be if this change were to be carried as a
> patch somewhere, by a downstream, but that seems unlikely, and can be
> avoided by that downstream not taking this patch.
> 
> Ian.
> 


RE: [PATCH] tools/configure: drop BASH configure variable

Posted by Paul Durrant 1 week ago
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 26 June 2020 18:01
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wl@xen.org>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; Paul Durrant <paul@xen.org>
> Subject: [PATCH] tools/configure: drop BASH configure variable
> 
> This is a weird variable to have in the first place.  The only user of it is
> XSM's CONFIG_SHELL, which opencodes a fallback to sh, and the only two scripts
> run with this are shebang sh anyway, so don't need bash in the first place.
> 
> Make the mkflask.sh and mkaccess_vector.sh scripts executable, drop the
> CONFIG_SHELL, and drop the $BASH variable to prevent further use.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> CC: Paul Durrant <paul@xen.org>
> 
> ./autogen.sh needs rerunning on commit.
> 
> RFC for 4.14.  This is a cleanup to the build system.
> 
> There is a separate check for [BASH] in the configure script, which is
> checking the requirement for the Linux hotplug scripts.  Really, that is a
> runtime requirement not a build time requirement, and it is rude to require
> bash in a build environment on this basis.  IMO, that check wants dropping as
> well.

That sounds quite reasonable.

Release-acked-by: Paul Durrant <paul@xen.org>

> ---
>  config/Tools.mk.in                      | 1 -
>  tools/configure.ac                      | 1 -
>  xen/xsm/flask/Makefile                  | 8 ++------
>  xen/xsm/flask/policy/mkaccess_vector.sh | 0
>  xen/xsm/flask/policy/mkflask.sh         | 0
>  5 files changed, 2 insertions(+), 8 deletions(-)
>  mode change 100644 => 100755 xen/xsm/flask/policy/mkaccess_vector.sh
>  mode change 100644 => 100755 xen/xsm/flask/policy/mkflask.sh
> 
> diff --git a/config/Tools.mk.in b/config/Tools.mk.in
> index 23df47af8d..48bd9ab731 100644
> --- a/config/Tools.mk.in
> +++ b/config/Tools.mk.in
> @@ -12,7 +12,6 @@ PYTHON              := @PYTHON@
>  PYTHON_PATH         := @PYTHONPATH@
>  PY_NOOPT_CFLAGS     := @PY_NOOPT_CFLAGS@
>  PERL                := @PERL@
> -BASH                := @BASH@
>  XGETTTEXT           := @XGETTEXT@
>  AS86                := @AS86@
>  LD86                := @LD86@
> diff --git a/tools/configure.ac b/tools/configure.ac
> index 9d126b7a14..6614a4f130 100644
> --- a/tools/configure.ac
> +++ b/tools/configure.ac
> @@ -297,7 +297,6 @@ AC_ARG_VAR([PYTHON], [Path to the Python parser])
>  AC_ARG_VAR([PERL], [Path to Perl parser])
>  AC_ARG_VAR([BISON], [Path to Bison parser generator])
>  AC_ARG_VAR([FLEX], [Path to Flex lexical analyser generator])
> -AC_ARG_VAR([BASH], [Path to bash shell])
>  AC_ARG_VAR([XGETTEXT], [Path to xgetttext tool])
>  AC_ARG_VAR([AS86], [Path to as86 tool])
>  AC_ARG_VAR([LD86], [Path to ld86 tool])
> diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile
> index 07f36d075d..ba8f31a02c 100644
> --- a/xen/xsm/flask/Makefile
> +++ b/xen/xsm/flask/Makefile
> @@ -8,10 +8,6 @@ CFLAGS-y += -I./include
> 
>  AWK = awk
> 
> -CONFIG_SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
> -          else if [ -x /bin/bash ]; then echo /bin/bash; \
> -          else echo sh; fi ; fi)
> -
>  FLASK_H_DEPEND = policy/security_classes policy/initial_sids
>  AV_H_DEPEND = policy/access_vectors
> 
> @@ -24,14 +20,14 @@ extra-y += $(ALL_H_FILES)
> 
>  mkflask := policy/mkflask.sh
>  quiet_cmd_mkflask = MKFLASK $@
> -cmd_mkflask = $(CONFIG_SHELL) $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> +cmd_mkflask = $(mkflask) $(AWK) include $(FLASK_H_DEPEND)
> 
>  $(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(mkflask) FORCE
>  	$(call if_changed,mkflask)
> 
>  mkaccess := policy/mkaccess_vector.sh
>  quiet_cmd_mkaccess = MKACCESS VECTOR $@
> -cmd_mkaccess = $(CONFIG_SHELL) $(mkaccess) $(AWK) $(AV_H_DEPEND)
> +cmd_mkaccess = $(mkaccess) $(AWK) $(AV_H_DEPEND)
> 
>  $(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(mkaccess) FORCE
>  	$(call if_changed,mkaccess)
> diff --git a/xen/xsm/flask/policy/mkaccess_vector.sh b/xen/xsm/flask/policy/mkaccess_vector.sh
> old mode 100644
> new mode 100755
> diff --git a/xen/xsm/flask/policy/mkflask.sh b/xen/xsm/flask/policy/mkflask.sh
> old mode 100644
> new mode 100755
> --
> 2.11.0