iasl-option not working

[Kevin O'Connor]

Hi Michael,

I was running through the SeaBIOS release checks when I found that the
new iasl-option code in the Makefile seems to choke on older versions
of iasl (for example, version 20120123 as shipped with fc13).  Can you
verify if the iasl detection still works properly for you with the
patch below?

-Kevin
> From 51755c3b5ed9dcdfdef8cee56631d68642bde416 Mon Sep 17 00:00:00 2001

From: Kevin O'Connor 
Date: Wed, 29 Aug 2012 21:27:37 -0400
Subject: [PATCH] Make iasl option check work with older versions of iasl.
To: [e-mail]

Signed-off-by: Kevin O'Connor 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 72ee152..45ea48a 100644
--- a/Makefile
+++ b/Makefile
@@ -221,7 +221,7 @@ $(OUT)vgabios.bin: $(OUT)vgabios.bin.raw tools/buildrom.py

################ DSDT build rules

-iasl-option=$(shell if "$(1)" "$(2)" -h > /dev/null 2>&1 \
+iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
     ; then echo "$(2)"; else echo "$(3)"; fi ;)

$(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py

[Michael S. Tsirkin]

On Wed, Aug 29, 2012 at 09:38:00PM -0400, Kevin O'Connor wrote:
> Hi Michael,
>
> I was running through the SeaBIOS release checks when I found that the
> new iasl-option code in the Makefile seems to choke on older versions
> of iasl (for example, version 20120123 as shipped with fc13).  Can you
> verify if the iasl detection still works properly for you with the
> patch below?
>
> -Kevin
> > From 51755c3b5ed9dcdfdef8cee56631d68642bde416 Mon Sep 17 00:00:00 2001
>
> From: Kevin O'Connor 
> Date: Wed, 29 Aug 2012 21:27:37 -0400
> Subject: [PATCH] Make iasl option check work with older versions of iasl.
> To: [e-mail]
> Signed-off-by: Kevin O'Connor
>
> Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 72ee152..45ea48a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -221,7 +221,7 @@ $(OUT)vgabios.bin: $(OUT)vgabios.bin.raw tools/buildrom.py
>
> ################ DSDT build rules
>
> -iasl-option=$(shell if "$(1)" "$(2)" -h > /dev/null 2>&1 \
> +iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
>      ; then echo "$(2)"; else echo "$(3)"; fi ;)
>
> $(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py

Will try. But logic seems fragile here: I am guessing old
iasl versions exit with error if you run them without input,
so this code relies on such to fail silently?
But what if future iasl starts producing
"Warning: no input" or such on stderr?
Seems a sensible thing to do and will break this script.

How about we give it a small dummy input file
instead and check exit status?
>

[Kevin O'Connor]

On Thu, Aug 30, 2012 at 08:38:06AM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 29, 2012 at 09:38:00PM -0400, Kevin O'Connor wrote:
> > ################ DSDT build rules
> >
> > -iasl-option=$(shell if "$(1)" "$(2)" -h > /dev/null 2>&1 \
> > +iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
> >      ; then echo "$(2)"; else echo "$(3)"; fi ;)
> >
> > $(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py
>
> Will try. But logic seems fragile here: I am guessing old
> iasl versions exit with error if you run them without input,
> so this code relies on such to fail silently?
> But what if future iasl starts producing
> "Warning: no input" or such on stderr?
> Seems a sensible thing to do and will break this script.
>
> How about we give it a small dummy input file
> instead and check exit status?

I agree with your statements on fragility, but I'm loathe to
complicate the Makefile that much.  I tested a bunch of iasl versions
and all seem to work with the check above.  For now I'm going to go
with this version.  Should iasl change in such a way that it breaks
this check, we can replace it with a test script in the tools/
directory.

-Kevin

[Michael S. Tsirkin]

On Wed, Aug 29, 2012 at 09:38:00PM -0400, Kevin O'Connor wrote:
> Hi Michael,
>
> I was running through the SeaBIOS release checks when I found that the
> new iasl-option code in the Makefile seems to choke on older versions
> of iasl (for example, version 20120123 as shipped with fc13).  Can you
> verify if the iasl detection still works properly for you with the
> patch below?
>
> -Kevin

So I think using -h is a bad idea: it failed once
it might fail again.
How about the following patch instead?
Tested with latest iasl from git (which needs -Pn)
and with F17 version (which does not).

-->

Makefile: fix iasl option detection

IASL option detection relied on iasl -h exit status which
is not very robust: -h is not expected to be used
from scripts. In particular combination of -h with
any option seems to succeed with some old
versions of iasl (e.g. version 20120123 as shipped with fc13).

Instead, supply a dummy input file and run iasl on it
to test option support.

Signed-off-by: Michael S. Tsirkin

---

diff --git a/Makefile b/Makefile
index 72ee152..0ab9f9f 100644
--- a/Makefile
+++ b/Makefile
@@ -221,14 +221,15 @@ $(OUT)vgabios.bin: $(OUT)vgabios.bin.raw tools/buildrom.py

################ DSDT build rules

-iasl-option=$(shell if "$(1)" "$(2)" -h > /dev/null 2>&1 \
-    ; then echo "$(2)"; else echo "$(3)"; fi ;)
+iasl-option=$(shell cp src/iasl-test.dsl $(1)/iasl-test.dsl; \
+    if "$(2)" "$(3)" $(1)/iasl-test.dsl > /dev/null 2>&1 \
+    ; then echo "$(3)"; else echo "$(4)"; fi ;)

$(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py
    @echo "  Compiling IASL $@"
    $(Q)cpp -P $< > $(OUT)$*.dsl.i.orig
    $(Q)$(PYTHON) ./tools/acpi_extract_preprocess.py $(OUT)$*.dsl.i.orig > $(OUT)$*.dsl.i
-	$(Q)$(IASL) $(call iasl-option,$(IASL),-Pn,) -vs -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
+	$(Q)$(IASL) $(call iasl-option,$(OUT),$(IASL),-Pn,) -vs -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
    $(Q)$(PYTHON) ./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off
    $(Q)cat $(OUT)$*.off > $@

diff --git a/src/iasl-test.dsl b/src/iasl-test.dsl
index e69de29..1a8b95e 100644
--- a/src/iasl-test.dsl
+++ b/src/iasl-test.dsl
@@ -0,0 +1,6 @@
+DefinitionBlock ("iasl-test.aml", "TEST", 0x01, "TSTC", "TESTIASLCMP", 0x1)
+{
+
+        Device (TEST) {
+        }
+}

[Michael S. Tsirkin]

On Thu, Aug 30, 2012 at 12:38:34PM +0300, Michael S. Tsirkin wrote:
> On Wed, Aug 29, 2012 at 09:38:00PM -0400, Kevin O'Connor wrote:
> > Hi Michael,
> >
> > I was running through the SeaBIOS release checks when I found that the
> > new iasl-option code in the Makefile seems to choke on older versions
> > of iasl (for example, version 20120123 as shipped with fc13).  Can you
> > verify if the iasl detection still works properly for you with the
> > patch below?
> >
> > -Kevin
>
> So I think using -h is a bad idea: it failed once
> it might fail again.
> How about the following patch instead?
> Tested with latest iasl from git (which needs -Pn)
> and with F17 version (which does not).

Scratch that, creating a temporary file without a
makefile rule will break parallel build.
Here's a version that has a proper rule.

-->

Makefile: fix iasl option detection

IASL option detection relied on iasl -h exit status which
is not very robust: -h is not expected to be used
from scripts. In particular combination of -h with
any option seems to succeed with some old
versions of iasl (e.g. version 20120123 as shipped with fc13).

Instead, supply a dummy input file and run iasl on it
to test option support.

Signed-off-by: Michael S. Tsirkin

---

diff --git a/Makefile b/Makefile
index 72ee152..33b3e69 100644
--- a/Makefile
+++ b/Makefile
@@ -221,14 +221,17 @@ $(OUT)vgabios.bin: $(OUT)vgabios.bin.raw tools/buildrom.py

################ DSDT build rules

-iasl-option=$(shell if "$(1)" "$(2)" -h > /dev/null 2>&1 \
-    ; then echo "$(2)"; else echo "$(3)"; fi ;)
+iasl-option=$(shell if "$(2)" "$(3)" "$(1)" > /dev/null 2>&1 \
+    ; then echo "$(3)"; else echo "$(4)"; fi ;)
+
+$(OUT)/iasl-test.dsl: src/iasl-test.dsl
+	$(Q)cp -f $< $@

-$(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py
+$(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.py $(OUT)/iasl-test.dsl
    @echo "  Compiling IASL $@"
    $(Q)cpp -P $< > $(OUT)$*.dsl.i.orig
    $(Q)$(PYTHON) ./tools/acpi_extract_preprocess.py $(OUT)$*.dsl.i.orig > $(OUT)$*.dsl.i
-	$(Q)$(IASL) $(call iasl-option,$(IASL),-Pn,) -vs -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
+	$(Q)$(IASL) $(call iasl-option,$(OUT)/iasl-test.dsl,$(IASL),-Pn,) -vs -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
    $(Q)$(PYTHON) ./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off
    $(Q)cat $(OUT)$*.off > $@

diff --git a/src/iasl-test.dsl b/src/iasl-test.dsl
index e69de29..1a8b95e 100644
--- a/src/iasl-test.dsl
+++ b/src/iasl-test.dsl
@@ -0,0 +1,6 @@
+DefinitionBlock ("iasl-test.aml", "TEST", 0x01, "TSTC", "TESTIASLCMP", 0x1)
+{
+
+        Device (TEST) {
+        }
+}