[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Ian Jackson <ian.jackson@xxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Mon, 29 Jun 2020 14:51:05 +0100
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Mon, 29 Jun 2020 13:51:31 +0000
  • Ironport-sdr: olhtQNkxxzVsjqWXNDwG1Ru67t2OnFClDj56q+owXhZbScbVBvbsZLEodpZLrL8h/JZS7OmJyd Gdlr8AQVvv06A9a79YJNhjmn3Ko5MWWsEAD2TEyN99VSpdAr0sQUQp0YDBqKxS0cZzq1JBEwo0 YfbYWhYr6yy0FrbdTUkFjxeT/lc2KslCeKcn8S71mYNM8546819yeaApsrDvizAPReOpPSQC+7 ALcZ6U+JFiTJJn+GKTYqXKTlayTIhVXKwfVMyXBu0ooVSRtuP/UWI7FT7grcQ+YfC7FMI9TmLi dWo=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.