Re: [PATCH] acpi/tests/bios-tables-test: add an environment variable for iasl location

[Michael S. Tsirkin]

On Wed, May 17, 2023 at 04:16:47PM +0100, Alex Bennée wrote:
> 
> Ani Sinha <anisinha@redhat.com> writes:
> 
> >> On 17-May-2023, at 8:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >> On Wed, May 17, 2023 at 07:57:53PM +0530, Ani Sinha wrote:
> >>> 
> >>> 
> >>>> On 17-May-2023, at 7:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> 
> >>>> On Wed, May 17, 2023 at 05:37:51PM +0530, Ani Sinha wrote:
> >>>>> Currently the meson based QEMU build process locates the iasl binary 
> >>>>> from the
> >>>>> current PATH and other locations [1] and uses that to set CONFIG_IASL 
> >>>>> which is
> >>>>> then used by the test.
> >>>>> 
> >>>>> This has two disadvantages:
> >>>>> - If iasl was not previously installed in the PATH, one has to install 
> >>>>> iasl
> >>>>>  and rebuild QEMU in order to pick up the iasl location. One cannot 
> >>>>> simply
> >>>>>  use the existing bios-tables-test binary because CONFIG_IASL is only 
> >>>>> set
> >>>>>  during the QEMU build time by meson and then bios-tables-test has to be
> >>>>>  rebuilt with CONFIG_IASL set in order to use iasl.
> 
> Usually we work the other way by checking at configure time and skipping
> the feature if the prerequisites are not in place. We do this with gdb:
> 
>   ../../configure 
> --gdb=/home/alex/src/tools/binutils-gdb.git/builds/all/install/bin/gdb
> 
> which checks gdb is at least new enough to support the features we need:
> 
>   if test -n "$gdb_bin"; then
>       gdb_version=$($gdb_bin --version | head -n 1)
>       if version_ge ${gdb_version##* } 9.1; then
>           echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>           gdb_arches=$("$source_path/scripts/probe-gdb-support.py" $gdb_bin)
>       else
>           gdb_bin=""
>       fi
>   fi
> 
> >>>>> - Sometimes, the stock iasl that comes with distributions is simply not 
> >>>>> good
> >>>>>  enough because it does not support the latest ACPI changes - newly
> >>>>>  introduced tables or new table attributes etc. In order to test ACPI 
> >>>>> code
> >>>>>  in QEMU, one has to clone the latest acpica upstream repository and
> >>>>>  rebuild iasl in order to get support for it. In those cases, one may 
> >>>>> want
> >>>>>  the test to use the iasl binary from a non-standard location.
> 
> I think configure should be checking if iasl is new enough and reporting
> to the user at configure time they need to do something different. We
> don't want to attempt to run tests that will fail unless the user has
> added the right magic to their environment.

iasl is a disassembler we trigger for user convenience in case tests
fail. It will never cause tests to fail.

> >>>>> 
> >>>>> In order to overcome the above two disadvantages, we introduce a new
> >>>>> environment variable IASL_PATH that can be set by the tester pointing 
> >>>>> to an
> >>>>> possibly non-standard iasl binary location. Bios-tables-test then uses 
> >>>>> this
> >>>>> environment variable to set its iasl location, possibly also overriding 
> >>>>> the
> >>>>> location that was pointed to by CONFIG_IASL that was set by meson. This 
> >>>>> way
> >>>>> developers can not only use this new environment variable to set iasl
> >>>>> location to quickly run bios-tables-test but also can point the test to 
> >>>>> a
> >>>>> custom iasl if required.
> >>>>> 
> >>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
> >>>>> 
> >>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> >>>> 
> >>>> Well I think the point was originally that meson can
> >>>> also test the binary in a variety of ways.
> >>>> Never surfaced so maybe never mind.
> >>>> 
> >>>> Would it be easier to just look iasl up on path
> >>> 
> >>> But that’s what meson is also doing, only QEMU build time.
> >> 
> >> 
> >> So you were unhappy it's build time because it is not really
> >> part of build and you want flexibility, right?
> >
> > Hmm, maybe in that case, we might want to resurrect iasl_installed(),
> > basically reverting part of cc8fa0e80836c51ba644d910c.
> >
> > To me its ok if I had to set IASL_PATH=`which iasl` before running the
> > test. I do not have strong opinions.
> 
> I don't think so - we should be using the tools configure found, after
> all that is its job.
> 
> -- 
> Alex Bennée
> Virtualisation Tech Lead @ Linaro


Let's say the whole problem does not seem that important to me either.

-- 
MST