Re: [PATCH v3 13/19] acpi: Makefile should better tolerate interrupts

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/09/2016 04:03 AM, Jan Beulich wrote:
>>>> On 08.09.16 at 20:51, <boris.ostrovsky@oracle.com> wrote:
>> On 09/08/2016 10:15 AM, Jan Beulich wrote:
>>>>>> On 07.09.16 at 20:59, <boris.ostrovsky@oracle.com> wrote:
>>>> --- a/tools/libacpi/Makefile
>>>> +++ b/tools/libacpi/Makefile
>>>> @@ -21,38 +21,45 @@ MK_DSDT = $(ACPI_BUILD_DIR)/mk_dsdt
>>>>  C_SRC = $(addprefix $(ACPI_BUILD_DIR)/, dsdt_anycpu.c dsdt_15cpu.c  
>> dsdt_anycpu_qemu_xen.c dsdt_pvh.c)
>>>>  H_SRC = $(addprefix $(ACPI_BUILD_DIR)/, ssdt_s3.h ssdt_s4.h ssdt_pm.h 
>> ssdt_tpm.h)
>>>>  
>>>> +ifeq ($(subst all,,$(MAKECMDGOALS)),)
>>>> +TDIR := $(shell mktemp -d --tmpdir=$(TMPDIR) tmp_XXXXXX)
>>>> +endif
>>> How is this (or really the rules using this directory) supposed to work
>>> when other than "all" gets built?
>>
>> I realize it's a somewhat weak argument but only 'all' and 'clean'
>> targets are supposed to be used. In fact I was thinking about explicitly
>> making a check for targets.
> Hmm, that would be quite limiting in case one wants to analyze a
> problem with just a single build step. (On the hypervisor side I,
> every once in a while, find it helpful to be able to compile individual
> files into .o, or even produce the intermediate .i or .s.)

Yes.

I tried to figure out a way for Makefile to always execute a rule when
before it exits but didn't find anything. You can do it as a *first*
rule (with "-include <file>") but not as a final one.

My only defense here (again, pretty weak) is that making libacpi 'all'
target is pretty fast.

>
>>>>  vpath iasl $(PATH)
>>>>  all: $(C_SRC) $(H_SRC)
>>>> +	rm -fr $(TDIR)
>>> And how is the temporary directory going to get cleaned up when
>>> interrupting make? I think you really should use a subdirectory
>>> underneath the build directory, which then can stay there until
>>> "make clean". And then you can also use mv instead of cp below,
>>> or even move-if-changed.
>> The reason I am doing this in /tmp and use tmp_XXXXX as template is
>> because I found that at least one old versions of iasl has a bug where
>> it can't process path that has a '.' in it. It drops anything after the
>> dot, presumably because it thinks it's file suffix.
> That . is a leading one, as in ./path/file.ext? If so, why can't this be
> made path/file.ext? The leading ./ shouldn't be necessary after all.

No, not a leading one. Inside a name:

Expected:

[root@ovs104 libacpi]# mkdir -p /tmp/root/xen.git
[root@ovs104 libacpi]# iasl -vs -p /tmp/root/xen.git/dsdt_anycpu -tc
/tmp/dsdt_anycpu.asl &>/dev/null
[root@ovs104 libacpi]# ls -R /tmp/root/
/tmp/root/:
xen.git

/tmp/root/xen.git:
dsdt_anycpu.aml  dsdt_anycpu.hex
[root@ovs104 libacpi]#


Fedora 12:

[root@ovs104 libacpi]# mkdir -p /tmp/root/xen.git
[root@ovs104 libacpi]# ~/iasl.f12 -vs -p /tmp/root/xen.git/dsdt_anycpu
-tc /tmp/dsdt_anycpu.asl &>/dev/null
[root@ovs104 libacpi]# ls -R /tmp/root/
/tmp/root/:
xen.aml  xen.git  xen.hex

/tmp/root/xen.git:
[root@ovs104 libacpi]#

I tried escaping the dot, putting path in quotes, etc. but without any
success.

>
> Also this part of your answer doesn't address the cleanup aspect at
> all.

Again, not the greatest way to deal with this, but we will leave temp
directory in /tmp on interrupts. (And on error I could have added an
error handling target but decided not to do that so that it's easier to
debug the error).


>  (And btw., thinking about it again I don't see the need for a
> subdirectory. We have ample room to name the intermediate files
> suitably without causing any name collision.)

That, in fact, is what I initially had --- I just added an extra temp
suffix to intermediate files during a build. And right before sending
the series I figured I'd give it a spin on our build server. Big mistake!

So I agree that this is not the best solution but I haven't come up with
anything better.

-boris


>
>> This is true on fedora12 (2009), which is what we still use as build
>> environment. I know that fedora 18 doesn't have this problem but I don't
>> know at which point this got fixed. (Interestingly enough, I tried to
>> build from F12's sources for iasl and could not reproduce this. Now, i
>> built with new tools so perhaps the bug is not in iasl itself but in
>> something like yacc, which is used for the build).
>>
>> I don't think we have any requirement on supported iasl version so I am
>> not sure we can ignore this error.
> I agree.
>
> Jan
>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel