Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler

[Ani Sinha]

> On 28-Jun-2023, at 4:24 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Wed, Jun 28, 2023 at 12:05:46PM +0530, Ani Sinha wrote:
>> 
>> 
>>> On 26-Jun-2023, at 6:49 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> 
>>> On Mon, Jun 26, 2023 at 06:33:14PM +0530, Ani Sinha wrote:
>>>> 
>>>> 
>>>>> On 26-Jun-2023, at 6:28 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> 
>>>>> On Mon, May 22, 2023 at 04:00:39PM +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 in
>>>>>> config-host.h header.This is then used at compile time by 
>>>>>> bios-tables-test to
>>>>>> set iasl path.
>>>>>> 
>>>>>> This has two disadvantages:
>>>>>> - If iasl was not previously installed in the PATH, one has to install 
>>>>>> iasl
>>>>>> and rebuild QEMU in order to regenerate the header and pick up the found
>>>>>> 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.
>>>>>> - 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.
>>>>>> 
>>>>>> In order to overcome the above two disadvantages, we set a default iasl 
>>>>>> path
>>>>>> as "/usr/bin/iasl". bios-tables-test also checks for the environment 
>>>>>> variable
>>>>>> IASL_PATH that can be set by the developer. IASL_PATH passed from the
>>>>>> environment overrides the default path. This way developers can point
>>>>>> IASL_PATH environment variable to a possibly a non-standard custom build
>>>>>> binary and quickly run bios-tables-test without rebuilding. If the 
>>>>>> default
>>>>>> path of iasl changes, one simply needs to update the default path and 
>>>>>> rebuild
>>>>>> just the test, not whole QEMU.
>>>>>> 
>>>>>> [1] https://mesonbuild.com/Reference-manual_functions.html#find_program
>>>>>> 
>>>>>> CC: alex.bennee@linaro.org
>>>>>> CC: pbonzini@redhat.com
>>>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>>> 
>>>>> I don't much like environment variables since they are
>>>>> not discoverable.
>>>> 
>>>> I do have this:
>>>> 
>>>> +                " Set IASL_PATH environment variable to the path of iasl 
>>>> binary\n"
>>>> +                " if iasl is installed somewhere other than %s.\n",
>>> 
>>> You only see this if there's a diff.
>>> 
>>> And then people stick this in their scripts and are scratching their
>>> heads trying to figure out why is a wrong iasl running.  Or someone
>>> comes up with a different use for IASL_PATH and they conflict.
>> 
>> OK in that case I think its ok to simply remove the environment
>> variable part. If people are going to be changing a header file,
> 
> Not people. configure script
> 
>> they
>> might as well change the DEFAULT_IASL_PATH in the test itself where
>> its easier to find. What additional complication meson provides is
>> that it uses find_program() to find the IASL binary in a list of
>> predefined locations. I do not think this additional tie up with meson
>> is worth it for the niche iasl use case.  Simple is beautiful.
> 
> The just the below then? And we can let it be?

The original problem I had was that I had to rebuild entire QEMU just so that 
it picked up the newly installed iasl binary. This does not seem right and I am 
ok if anyone fixed this in any way they liked. I loved it when previously the 
test “just worked” when transitioning from no installed iasl to iasl installed 
in default path. In my opinion, currently what we do with iasl and meson is way 
too complex and is really not needed for the purpose we use the tool for.

> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index ed1c69cf01..d0e1655d2e 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -102,6 +102,7 @@ typedef struct {
> 
> static char disk[] = "tests/acpi-test-disk-XXXXXX";
> static const char *data_dir = "tests/data/acpi";
> +/* If you want your own path, change the below to iasl = 
> "/home/usr/bin/iasl" */
> #ifdef CONFIG_IASL
> static const char *iasl = CONFIG_IASL;
> #else