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

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

[Andrew Cooper]

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>

./autogen.sh needs rerunning on commit.

v2:
 * Use $(SHELL)

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..50bec20a1e 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 = $(SHELL) $(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 = $(SHELL) $(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

[Andrew Cooper]

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

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

[Paul Durrant]

> -----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

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

[Jan Beulich]

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

[Ian Jackson]

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

[Jan Beulich]

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 [and 1 more messages]

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
> 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:
> >> ... 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.)

I don't think we support building from sources which have been
unpacked onto such filesystems.  Other projects which might actually
need to build on Windows or something do do this $(SHELL) thing or an
equivalent, but I don't think that's us.

But obviously that opinion of mine is not a blocker for Andy's patch
since Andy is going in the right direction, regardless.

Andrew Cooper writes ("[PATCH v2] 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.

In response to this commit message, I wrote:

> > Andrew Cooper writes ("[PATCH] tools/configure: drop BASH configure variable"):
> > 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 ?

And you replied:

> Yes, to all of the above.
> 
> They are both very thin wrappers (doing some argument shuffling) around
> large AWK scripts.

Can you please put this information in the commit message where it
belongs ?  As a rule we should know in future what we were thinking
when a change was made, and as I say "are shebang sh anyway, so don't
need bash in the first place" is weak evidence.

With that change,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Jan Beulich]

On 31.07.2020 15:02, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>> 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:
>>>> ... 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.)
> 
> I don't think we support building from sources which have been
> unpacked onto such filesystems.  Other projects which might actually
> need to build on Windows or something do do this $(SHELL) thing or an
> equivalent, but I don't think that's us.

It's not unexpected that you think of Windows here, but my thoughts
were more towards building from sources on a CD or DVD, where iirc
execute permissions also don't exist. The latest when we have
out-of-tree builds fully working, this ought to be something that
people should be able to do, imo. (Even without out-of-tree builds,
my "next best" alternative of using a tree of symlinks to build in
would similarly have an issue with the links pointing at a mounted
CD/DVD, if the $(SHELL) wasn't present.)

Jan

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Andrew Cooper]

On 31/07/2020 14:30, Jan Beulich wrote:
> On 31.07.2020 15:02, Ian Jackson wrote:
>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>> 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:
>>>>> ... 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.)
>> I don't think we support building from sources which have been
>> unpacked onto such filesystems.  Other projects which might actually
>> need to build on Windows or something do do this $(SHELL) thing or an
>> equivalent, but I don't think that's us.
> It's not unexpected that you think of Windows here, but my thoughts
> were more towards building from sources on a CD or DVD, where iirc
> execute permissions also don't exist. The latest when we have
> out-of-tree builds fully working, this ought to be something that
> people should be able to do, imo. (Even without out-of-tree builds,
> my "next best" alternative of using a tree of symlinks to build in
> would similarly have an issue with the links pointing at a mounted
> CD/DVD, if the $(SHELL) wasn't present.)

See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
time to continue arguing over this point.

~Andrew

Re: [PATCH] tools/configure: drop BASH configure variable [and 1 more messages]

[Jan Beulich]

On 31.07.2020 15:46, Andrew Cooper wrote:
> On 31/07/2020 14:30, Jan Beulich wrote:
>> On 31.07.2020 15:02, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH] tools/configure: drop BASH configure variable"):
>>>> 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:
>>>>>> ... 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.)
>>> I don't think we support building from sources which have been
>>> unpacked onto such filesystems.  Other projects which might actually
>>> need to build on Windows or something do do this $(SHELL) thing or an
>>> equivalent, but I don't think that's us.
>> It's not unexpected that you think of Windows here, but my thoughts
>> were more towards building from sources on a CD or DVD, where iirc
>> execute permissions also don't exist. The latest when we have
>> out-of-tree builds fully working, this ought to be something that
>> people should be able to do, imo. (Even without out-of-tree builds,
>> my "next best" alternative of using a tree of symlinks to build in
>> would similarly have an issue with the links pointing at a mounted
>> CD/DVD, if the $(SHELL) wasn't present.)
> 
> See v2.  I put $(SHELL) in, because it isn't a worthwhile use of our
> time to continue arguing over this point.

I had seen you did; I was merely replying back to Ian's comments.

Jan

[PATCH] tools/configure: drop BASH configure variable

[Ian Jackson]

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

[Andrew Cooper]

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

[Paul Durrant]

> -----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