qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Ani Sinha <anisinha@redhat.com>
To: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	"\"Daniel P. Berrangé\"" <berrange@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler
Date: Wed, 28 Jun 2023 17:47:35 +0530	[thread overview]
Message-ID: <7866687C-C29F-4B79-82EA-CB6D0A78119F@redhat.com> (raw)
In-Reply-To: <20230628065132-mutt-send-email-mst@kernel.org>



> 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



      reply	other threads:[~2023-06-28 12:18 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-22 10:30 [PATCH v3] acpi/tests/bios-tables-test: make iasl tool handling simpler Ani Sinha
2023-05-22 10:46 ` Paolo Bonzini
2023-06-26 12:58 ` Michael S. Tsirkin
2023-06-26 13:03   ` Ani Sinha
2023-06-26 13:19     ` Michael S. Tsirkin
2023-06-28  6:35       ` Ani Sinha
2023-06-28 10:54         ` Michael S. Tsirkin
2023-06-28 12:17           ` Ani Sinha [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7866687C-C29F-4B79-82EA-CB6D0A78119F@redhat.com \
    --to=anisinha@redhat.com \
    --cc=alex.bennee@linaro.org \
    --cc=berrange@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=marcandre.lureau@redhat.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).