diff mbox

Fix in ACPICA tools broke cross compilation of tools/power/acpi

Message ID CAHp75VdJ1Nb_qbRX9b=RfDX-8e2tOUvMcbgYx8aMr4COQN3p8Q@mail.gmail.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Andy Shevchenko Nov. 15, 2016, 3:01 p.m. UTC
On Tue, Nov 15, 2016 at 4:23 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 12:27 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Nov 3, 2016 at 5:04 PM, Zheng, Lv <lv.zheng@intel.com> wrote:
>>>> On Sun, Oct 30, 2016 at 9:04 AM, Zheng, Lv <lv.zheng@intel.com> wrote:

>> But reveals another, the output path is broken:
>>
>> output/host/usr/bin/i586-buildroot-linux-uclibc-gcc -c -D_LINUX -I
>> $OUT/include -I $SRC/tools/power/acpi/../../../drivers/acpi/acpica
>> -Wall -Wst
>> rict-prototypes -Wdeclaration-after-statement -O1 -g -DDEBUG
>> -DACPI_APPLICATION -DACPI_SINGLE_THREAD -DACPI_DEBUGG
>> ER -I. -o $OUT/tools/acpidbg/acpidbg.o acpidbg.c
>> Assembler messages:
>> Fatal error: can't create $OUT/tools/acpidbg/acpidbg.o: No such file
>> or directory
>> ../../Makefile.rules:26: recipe for target '$OUT/tools/acpidbg/acpidbg.o' failed
>>
>> $OUT — path to O=
>> $SRC — path to kernel sources
>
> From latest run
>
> output/host/usr/bin/i586-buildroot-linux-uclibc-gcc -c -D_LINUX -I
> $OUT/include -I $SRC/tools/power/acpi/../../../drivers/acpi/acpica
> -Wall -Wst
> rict-prototypes -Wdeclaration-after-statement -O1 -g -DDEBUG
> -DACPI_APPLICATION -DACPI_SINGLE_THREAD -DACPI_DEBUGG
> ER -I. -o $OUT/tools/acpidbg/acpidbg.o acpidbg.c
>
> Of course it fails since proper folder name should be
> 'tools/power/acpi/tools' instead of 'tools'.

The below + several runs (need to serialize makefile, by default it
races install vs. build)  helped eventually.

Comments

Andy Shevchenko Nov. 15, 2016, 3:02 p.m. UTC | #1
On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> The below + several runs (need to serialize makefile, by default it
> races install vs. build)  helped eventually.

Isn't better just to revert patch I mention and continue from working case?
Rafael J. Wysocki Nov. 15, 2016, 5:37 p.m. UTC | #2
On Tue, Nov 15, 2016 at 4:02 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
>> The below + several runs (need to serialize makefile, by default it
>> races install vs. build)  helped eventually.
>
> Isn't better just to revert patch I mention and continue from working case?

Which one is that, I seem to have lost track of it?

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng Nov. 16, 2016, 8:43 a.m. UTC | #3
Hi, Andy

> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> 

> On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko

> <andy.shevchenko@gmail.com> wrote:

> 

> > The below + several runs (need to serialize makefile, by default it

> > races install vs. build)  helped eventually.

> 

> Isn't better just to revert patch I mention and continue from working case?


It's not possible to revert that patch.
It contains correct header inclusion cleanups.
While this case is in fact a special case, bugs in tools/Makefile are triggered (about OUT/SRC).

OTOH, this case is actually not a very useful case.
We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.
We are just to improve it.

Thanks
Lv
Lv Zheng Nov. 16, 2016, 8:48 a.m. UTC | #4
Hi,

> From: Zheng, Lv

> Subject: RE: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> 

> Hi, Andy

> 

> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> > Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> >

> > On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko

> > <andy.shevchenko@gmail.com> wrote:

> >

> > > The below + several runs (need to serialize makefile, by default it

> > > races install vs. build)  helped eventually.

> >

> > Isn't better just to revert patch I mention and continue from working case?

> 

> It's not possible to revert that patch.

> It contains correct header inclusion cleanups.

> While this case is in fact a special case, bugs in tools/Makefile are triggered (about OUT/SRC).

> 

> OTOH, this case is actually not a very useful case.

> We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.

> We are just to improve it.


We can clearly see that:
1. the commit is correct, it can cleanly sort the inclusion orders.
2. tools/power/acpi build is still working, just have issues with cross toolchains.
3. the strict inclusion order (signal.h) itself is a bug - because toolchain issue and ACPICA trick.
4. tools/power/acpi makefiles are not handling OUT/SRC correctly.
So why do we need to revert correct thing to support a buggy behavior?
We are just finding a way to make tools/power/acpi build more robust for such a buggy behavior.

Thanks
Lv
Lv Zheng Nov. 16, 2016, 9:29 a.m. UTC | #5
Hi, Andy

> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> 

> On Tue, Nov 15, 2016 at 4:23 PM, Andy Shevchenko

> <andy.shevchenko@gmail.com> wrote:

> > On Tue, Nov 15, 2016 at 12:27 PM, Andy Shevchenko

> > <andy.shevchenko@gmail.com> wrote:

> >> On Thu, Nov 3, 2016 at 5:04 PM, Zheng, Lv <lv.zheng@intel.com> wrote:

> >>>> On Sun, Oct 30, 2016 at 9:04 AM, Zheng, Lv <lv.zheng@intel.com> wrote:

> 

> >> But reveals another, the output path is broken:

> >>

> >> output/host/usr/bin/i586-buildroot-linux-uclibc-gcc -c -D_LINUX -I

> >> $OUT/include -I $SRC/tools/power/acpi/../../../drivers/acpi/acpica

> >> -Wall -Wst

> >> rict-prototypes -Wdeclaration-after-statement -O1 -g -DDEBUG

> >> -DACPI_APPLICATION -DACPI_SINGLE_THREAD -DACPI_DEBUGG

> >> ER -I. -o $OUT/tools/acpidbg/acpidbg.o acpidbg.c

> >> Assembler messages:

> >> Fatal error: can't create $OUT/tools/acpidbg/acpidbg.o: No such file

> >> or directory

> >> ../../Makefile.rules:26: recipe for target '$OUT/tools/acpidbg/acpidbg.o' failed

> >>

> >> $OUT — path to O=

> >> $SRC — path to kernel sources

> >

> > From latest run

> >

> > output/host/usr/bin/i586-buildroot-linux-uclibc-gcc -c -D_LINUX -I

> > $OUT/include -I $SRC/tools/power/acpi/../../../drivers/acpi/acpica

> > -Wall -Wst

> > rict-prototypes -Wdeclaration-after-statement -O1 -g -DDEBUG

> > -DACPI_APPLICATION -DACPI_SINGLE_THREAD -DACPI_DEBUGG

> > ER -I. -o $OUT/tools/acpidbg/acpidbg.o acpidbg.c

> >

> > Of course it fails since proper folder name should be

> > 'tools/power/acpi/tools' instead of 'tools'.

> 

> The below + several runs (need to serialize makefile, by default it

> races install vs. build)  helped eventually.


I changed dependency in v4.
Please check if it works now.

Thanks and best regards
Lv

> 

> --- a/tools/power/acpi/Makefile.rules

> +++ b/tools/power/acpi/Makefile.rules

> @@ -8,9 +8,9 @@

> # as published by the Free Software Foundation; version 2

> # of the License.

> 

> -objdir := $(OUTPUT)tools/$(TOOL)/

> +objdir := $(OUTPUT)tools/power/acpi/tools/$(TOOL)/

> toolobjs := $(addprefix $(objdir),$(TOOL_OBJS))

> -$(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE

> +$(objdir)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE

>        $(ECHO) "  LD      " $(subst $(OUTPUT),,$@)

>        $(QUIET) $(LD) $(CFLAGS) $(LDFLAGS) $(toolobjs) -L$(OUTPUT) -o $@

>        $(ECHO) "  STRIP   " $(subst $(OUTPUT),,$@)

> @@ -26,21 +26,21 @@ $(objdir)%.o: %.c

>        $(ECHO) "  CC      " $(subst $(OUTPUT),,$@)

>        $(QUIET) $(CC) -c $(CFLAGS) -o $@ $<

> 

> -all: $(OUTPUT)$(TOOL)

> +all: $(objdir)$(TOOL)

> clean:

>        $(ECHO) "  RMOBJ   " $(subst $(OUTPUT),,$(objdir))

>        $(QUIET) find $(objdir) \( -not -type d \)\

>                 -and \( -name '*~' -o -name '*.[oas]' \)\

>                 -type f -print | xargs rm -f

>        $(ECHO) "  RM      " $(TOOL)

> -       $(QUIET) rm -f $(OUTPUT)$(TOOL)

> +       $(QUIET) rm -f $(objdir)$(TOOL)

>        $(ECHO) "  RMINC   " $(subst $(OUTPUT),,$(KERNEL_INCLUDE))

>        $(QUIET) rm -rf $(KERNEL_INCLUDE)

> 

> install-tools:

>        $(ECHO) "  INST    " $(TOOL)

>        $(QUIET) $(INSTALL) -d $(DESTDIR)$(sbindir)

> -       $(QUIET) $(INSTALL_PROGRAM) $(OUTPUT)$(TOOL) $(DESTDIR)$(sbindir)

> +       $(QUIET) $(INSTALL_PROGRAM) $(objdir)$(TOOL) $(DESTDIR)$(sbindir)

> uninstall-tools:

>        $(ECHO) "  UNINST  " $(TOOL)

>        $(QUIET) rm -f $(DESTDIR)$(sbindir)/$(TOOL)

> 

> 

> --

> With Best Regards,

> Andy Shevchenko
Rafael J. Wysocki Nov. 16, 2016, 12:58 p.m. UTC | #6
On Wed, Nov 16, 2016 at 9:43 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Andy
>
>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi
>>
>> On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>
>> > The below + several runs (need to serialize makefile, by default it
>> > races install vs. build)  helped eventually.
>>
>> Isn't better just to revert patch I mention and continue from working case?
>
> It's not possible to revert that patch.

It surely is possible to revert things, but it may not be a good idea.
So I think you wanted to say that reverting this particular one would
not be a good idea.

> It contains correct header inclusion cleanups.

Cleanups are not a good enough reason for breaking builds.

> While this case is in fact a special case, bugs in tools/Makefile are triggered (about OUT/SRC).

I'm not sure what you mean.

> OTOH, this case is actually not a very useful case.

In your opinion I guess?

> We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.

I guess the issue is that it was not broken before and it is broken
after your changes.  That's quite a bit of a difference.

> We are just to improve it.

That's nice, but see above.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng Nov. 17, 2016, 3:10 a.m. UTC | #7
Hi, Rafael

> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki

> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> 

> On Wed, Nov 16, 2016 at 9:43 AM, Zheng, Lv <lv.zheng@intel.com> wrote:

> > Hi, Andy

> >

> >> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> >> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> >>

> >> On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko

> >> <andy.shevchenko@gmail.com> wrote:

> >>

> >> > The below + several runs (need to serialize makefile, by default it

> >> > races install vs. build)  helped eventually.

> >>

> >> Isn't better just to revert patch I mention and continue from working case?

> >

> > It's not possible to revert that patch.

> 

> It surely is possible to revert things, but it may not be a good idea.

> So I think you wanted to say that reverting this particular one would

> not be a good idea.


Yes, it's not a good idea.
The cleanup is correct.

> 

> > It contains correct header inclusion cleanups.

> 

> Cleanups are not a good enough reason for breaking builds.


It doesn't break, build is OK in my environment.

> 

> > While this case is in fact a special case, bugs in tools/Makefile are triggered (about OUT/SRC).

> 

> I'm not sure what you mean.


I'm wrong here.
The only issue triggered is:
The following line in the acenv.h need to be commented out:
#include <signal.h>

> 

> > OTOH, this case is actually not a very useful case.

> 

> In your opinion I guess?


Yes. IMO,
The above breakage looks much more strange than the commit itself.
It's better to make the build successful without commenting this line out.

Such kind of regressions bothered me a lot.
It seems we can easily fell into such old traps.

If I cannot fix the new regression in time, I will sure suggest to revert breakage commit before a better solution can be offered after investigation.
However, for this issue:
1. There is actually no serious breakage (only toolchains built with incompliant kernel headers)
2. We have fixed the issue, but the fix need to be improved.
So reverting the correct commit isn't a good idea.
Reverting it may trigger more serious issues because of wrong header inclusion orders.
Let me improve it.

> 

> > We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.

> 

> I guess the issue is that it was not broken before and it is broken

> after your changes.  That's quite a bit of a difference.


IMO, the non-updated toolchains/or ACPICA's sX/uX definitions are actually buggy while the commit isn't.

> 

> > We are just to improve it.

> 

> That's nice, but see above.


Yes, we have fixed it.
And Yisheng Xie has feedback that this issue has been fixed.

However, the v1 fix contains problem:
There is one line dependency not correct:
 acpidbg acpidump ec: include/acpi FORCE
I failed to correct it in v2/v3:
 $(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
Which was criticized by Andy, complaining new breaks for parallel builds.
Now in v4, it is corrected:
 $(objdir)%.o: %.c $(KERNEL_INCLUDE)

However, in v2/v3, I cleaned up OUTPUT related code and changed its behavior of build with "O=".
In v2/v3, build "O=" from tools/power/acpi folder passes, but build from tools folder fails.
In v4, build "O=" from tools folder passes, but build from tools/power/acpi folder fails.

There doesn't seem to be a solution to make both cases working.
Because the "descend" macro in tools/scripts/Makefile.include cannot handle both cases.
Stopping using "descend" macro in tools/power/acpi/Makefile could be the only possible fix...
So we only need one of them working and IMO v4 is suitable for Andy's needs.

As a conclusion, v4 should be OK now.

Thanks and best regards
Lv
Rafael J. Wysocki Nov. 17, 2016, 3:02 p.m. UTC | #8
On Thu, Nov 17, 2016 at 4:10 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi, Rafael
>
>> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of Rafael J. Wysocki
>> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi
>>
>> On Wed, Nov 16, 2016 at 9:43 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>> > Hi, Andy
>> >
>> >> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>> >> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi
>> >>
>> >> On Tue, Nov 15, 2016 at 5:01 PM, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> wrote:
>> >>
>> >> > The below + several runs (need to serialize makefile, by default it
>> >> > races install vs. build)  helped eventually.
>> >>
>> >> Isn't better just to revert patch I mention and continue from working case?
>> >
>> > It's not possible to revert that patch.
>>
>> It surely is possible to revert things, but it may not be a good idea.
>> So I think you wanted to say that reverting this particular one would
>> not be a good idea.
>
> Yes, it's not a good idea.
> The cleanup is correct.
>
>>
>> > It contains correct header inclusion cleanups.
>>
>> Cleanups are not a good enough reason for breaking builds.
>
> It doesn't break, build is OK in my environment.
>
>>
>> > While this case is in fact a special case, bugs in tools/Makefile are triggered (about OUT/SRC).
>>
>> I'm not sure what you mean.
>
> I'm wrong here.
> The only issue triggered is:
> The following line in the acenv.h need to be commented out:
> #include <signal.h>
>
>>
>> > OTOH, this case is actually not a very useful case.
>>
>> In your opinion I guess?
>
> Yes. IMO,
> The above breakage looks much more strange than the commit itself.
> It's better to make the build successful without commenting this line out.
>
> Such kind of regressions bothered me a lot.
> It seems we can easily fell into such old traps.
>
> If I cannot fix the new regression in time, I will sure suggest to revert breakage commit before a better solution can be offered after investigation.
> However, for this issue:
> 1. There is actually no serious breakage (only toolchains built with incompliant kernel headers)
> 2. We have fixed the issue, but the fix need to be improved.
> So reverting the correct commit isn't a good idea.
> Reverting it may trigger more serious issues because of wrong header inclusion orders.
> Let me improve it.
>
>>
>> > We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.
>>
>> I guess the issue is that it was not broken before and it is broken
>> after your changes.  That's quite a bit of a difference.
>
> IMO, the non-updated toolchains/or ACPICA's sX/uX definitions are actually buggy while the commit isn't.
>
>>
>> > We are just to improve it.
>>
>> That's nice, but see above.
>
> Yes, we have fixed it.
> And Yisheng Xie has feedback that this issue has been fixed.
>
> However, the v1 fix contains problem:
> There is one line dependency not correct:
>  acpidbg acpidump ec: include/acpi FORCE
> I failed to correct it in v2/v3:
>  $(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
> Which was criticized by Andy, complaining new breaks for parallel builds.
> Now in v4, it is corrected:
>  $(objdir)%.o: %.c $(KERNEL_INCLUDE)
>
> However, in v2/v3, I cleaned up OUTPUT related code and changed its behavior of build with "O=".
> In v2/v3, build "O=" from tools/power/acpi folder passes, but build from tools folder fails.
> In v4, build "O=" from tools folder passes, but build from tools/power/acpi folder fails.
>
> There doesn't seem to be a solution to make both cases working.
> Because the "descend" macro in tools/scripts/Makefile.include cannot handle both cases.
> Stopping using "descend" macro in tools/power/acpi/Makefile could be the only possible fix...
> So we only need one of them working and IMO v4 is suitable for Andy's needs.
>
> As a conclusion, v4 should be OK now.

OK, I've queued it up and I'm going to push it for -rc6 tomorrow.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Dec. 12, 2016, 8:58 p.m. UTC | #9
On Thu, Nov 17, 2016 at 5:10 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>>
>> > It contains correct header inclusion cleanups.
>>
>> Cleanups are not a good enough reason for breaking builds.
>
> It doesn't break, build is OK in my environment.

You know, before you did that first clean up for MSVC it worked for me.

Now I got the following

 DESCEND  power/acpi
 DESCEND  tools/acpidbg
 DESCEND  tools/acpidump
 DESCEND  tools/ec
 MKDIR    include
 INST     acpidbg
 MKDIR    include
 INST     ec
 MKDIR    include
 CP       include
 CP       include
/usr/bin/install: cannot stat '.../power/acpi/acpidbg': No such file
or directory
 INST     acpidump
../../Makefile.rules:41: recipe for target 'install-tools' failed
make[6]: *** [install-tools] Error 1
/usr/bin/install: make[6]: *** Waiting for unfinished jobs....
cannot stat '.../power/acpi/ec': No such file or directory
../../Makefile.rules:41: recipe for target 'install-tools' failed
make[6]: *** [install-tools] Error 1
make[6]: *** Waiting for unfinished jobs....
 INST     acpidump.8
 CP       include
Makefile:23: recipe for target 'acpidbg_install' failed
make[5]: *** [acpidbg_install] Error 2
make[5]: *** Waiting for unfinished jobs....
/usr/bin/install: cannot stat '.../power/acpi/acpidump': No such file
or directory
../../Makefile.rules:41: recipe for target 'install-tools' failed
make[6]: *** [install-tools] Error 1
make[6]: *** Waiting for unfinished jobs....
Makefile:23: recipe for target 'ec_install' failed
make[5]: *** [ec_install] Error 2
Makefile:23: recipe for target 'acpidump_install' failed
make[5]: *** [acpidump_install] Error 2
Makefile:95: recipe for target 'acpi_install' failed
make[4]: *** [acpi_install] Error 2

.../Makefile:1613: recipe for target 'tools/acpi_install' failed

>>> Just to notice that above Makefile comes from the source tree not the O= variant
The rest of ... related to O=/my/output/path.

make[3]: *** [tools/acpi_install] Error 2
Makefile:150: recipe for target 'sub-make' failed
make[2]: *** [sub-make] Error 2
Makefile:24: recipe for target '__sub-make' failed
make[1]: *** [__sub-make] Error 2

So, it doesn't even try to build anything.

> Yes. IMO,
> The above breakage looks much more strange than the commit itself.
> It's better to make the build successful without commenting this line out.
>
> Such kind of regressions bothered me a lot.
> It seems we can easily fell into such old traps.
>
> If I cannot fix the new regression in time, I will sure suggest to revert breakage commit before a better solution can be offered after investigation.
> However, for this issue:
> 1. There is actually no serious breakage (only toolchains built with incompliant kernel headers)
> 2. We have fixed the issue, but the fix need to be improved.
> So reverting the correct commit isn't a good idea.
> Reverting it may trigger more serious issues because of wrong header inclusion orders.
> Let me improve it.
>
>>
>> > We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.
>>
>> I guess the issue is that it was not broken before and it is broken
>> after your changes.  That's quite a bit of a difference.
>
> IMO, the non-updated toolchains/or ACPICA's sX/uX definitions are actually buggy while the commit isn't.
>
>>
>> > We are just to improve it.
>>
>> That's nice, but see above.
>
> Yes, we have fixed it.
> And Yisheng Xie has feedback that this issue has been fixed.
>
> However, the v1 fix contains problem:
> There is one line dependency not correct:
>  acpidbg acpidump ec: include/acpi FORCE
> I failed to correct it in v2/v3:
>  $(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
> Which was criticized by Andy, complaining new breaks for parallel builds.
> Now in v4, it is corrected:
>  $(objdir)%.o: %.c $(KERNEL_INCLUDE)
>
> However, in v2/v3, I cleaned up OUTPUT related code and changed its behavior of build with "O=".
> In v2/v3, build "O=" from tools/power/acpi folder passes, but build from tools folder fails.
> In v4, build "O=" from tools folder passes, but build from tools/power/acpi folder fails.
>
> There doesn't seem to be a solution to make both cases working.
> Because the "descend" macro in tools/scripts/Makefile.include cannot handle both cases.
> Stopping using "descend" macro in tools/power/acpi/Makefile could be the only possible fix...
> So we only need one of them working and IMO v4 is suitable for Andy's needs.
>
> As a conclusion, v4 should be OK now.

See above.

This is how make started. $(KERNEL_TOOLS_TARGET) is a list of folders
under tools (for now just "acpi gpio turbostat"). $(1) either
"install" or "clean".
KERNEL_SRC the output folder of kernel build (it might be the same as
sources, but not my case, I used O=). The rest from the list are
compiled and installed nicely. This *was* the case for acpi tools.

+       $(Q)for t in $(KERNEL_TOOLS_TARGET); do                 \
+               $(MAKE) -C $(KERNEL_SRC)                        \
+                       CROSS_COMPILE="$(TARGET_CROSS)"         \
+                       DESTDIR="$(TARGET_DIR)"                 \
+                       tools/$${t}_$(1);                       \
+       done

So. there is no magic here.

Can you add this simple test to your test cases?
Andy Shevchenko Dec. 12, 2016, 9:10 p.m. UTC | #10
On Mon, Dec 12, 2016 at 10:58 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 17, 2016 at 5:10 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
>>>
>>> > It contains correct header inclusion cleanups.
>>>
>>> Cleanups are not a good enough reason for breaking builds.
>>
>> It doesn't break, build is OK in my environment.
>
> You know, before you did that first clean up for MSVC it worked for me.
>
> Now I got the following

I serialized make in my example to be make -j1 and got the following

 DESCEND  power/acpi
 DESCEND  tools/acpidbg
 CC       tools/acpidbg/acpidbg.o
Assembler messages:
Fatal error: can't create .../power/acpi/tools/acpidbg/acpidbg.o: No
such file or dir
ectory
../../Makefile.rules:26: recipe for target
'.../power/acpi/tools/acpidbg/acpidbg.o' f
ailed

As you may already notice from the below there is a wrong folder used.
The hierarchy is

% ls -R tools/power/
tools/power/:
acpi  x86

tools/power/acpi:
tools

tools/power/acpi/tools:
acpidbg  acpidump  ec

tools/power/acpi/tools/acpidbg:

tools/power/acpi/tools/acpidump:

tools/power/acpi/tools/ec:

tools/power/x86:
turbostat


>
>  DESCEND  power/acpi
>  DESCEND  tools/acpidbg
>  DESCEND  tools/acpidump
>  DESCEND  tools/ec
>  MKDIR    include
>  INST     acpidbg
>  MKDIR    include
>  INST     ec
>  MKDIR    include
>  CP       include
>  CP       include
> /usr/bin/install: cannot stat '.../power/acpi/acpidbg': No such file
> or directory
>  INST     acpidump
> ../../Makefile.rules:41: recipe for target 'install-tools' failed
> make[6]: *** [install-tools] Error 1
> /usr/bin/install: make[6]: *** Waiting for unfinished jobs....
> cannot stat '.../power/acpi/ec': No such file or directory
> ../../Makefile.rules:41: recipe for target 'install-tools' failed
> make[6]: *** [install-tools] Error 1
> make[6]: *** Waiting for unfinished jobs....
>  INST     acpidump.8
>  CP       include
> Makefile:23: recipe for target 'acpidbg_install' failed
> make[5]: *** [acpidbg_install] Error 2
> make[5]: *** Waiting for unfinished jobs....
> /usr/bin/install: cannot stat '.../power/acpi/acpidump': No such file
> or directory
> ../../Makefile.rules:41: recipe for target 'install-tools' failed
> make[6]: *** [install-tools] Error 1
> make[6]: *** Waiting for unfinished jobs....
> Makefile:23: recipe for target 'ec_install' failed
> make[5]: *** [ec_install] Error 2
> Makefile:23: recipe for target 'acpidump_install' failed
> make[5]: *** [acpidump_install] Error 2
> Makefile:95: recipe for target 'acpi_install' failed
> make[4]: *** [acpi_install] Error 2
>
> .../Makefile:1613: recipe for target 'tools/acpi_install' failed
>
>>>> Just to notice that above Makefile comes from the source tree not the O= variant
> The rest of ... related to O=/my/output/path.
>
> make[3]: *** [tools/acpi_install] Error 2
> Makefile:150: recipe for target 'sub-make' failed
> make[2]: *** [sub-make] Error 2
> Makefile:24: recipe for target '__sub-make' failed
> make[1]: *** [__sub-make] Error 2
>
> So, it doesn't even try to build anything.
>
>> Yes. IMO,
>> The above breakage looks much more strange than the commit itself.
>> It's better to make the build successful without commenting this line out.
>>
>> Such kind of regressions bothered me a lot.
>> It seems we can easily fell into such old traps.
>>
>> If I cannot fix the new regression in time, I will sure suggest to revert breakage commit before a better solution can be offered after investigation.
>> However, for this issue:
>> 1. There is actually no serious breakage (only toolchains built with incompliant kernel headers)
>> 2. We have fixed the issue, but the fix need to be improved.
>> So reverting the correct commit isn't a good idea.
>> Reverting it may trigger more serious issues because of wrong header inclusion orders.
>> Let me improve it.
>>
>>>
>>> > We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.
>>>
>>> I guess the issue is that it was not broken before and it is broken
>>> after your changes.  That's quite a bit of a difference.
>>
>> IMO, the non-updated toolchains/or ACPICA's sX/uX definitions are actually buggy while the commit isn't.
>>
>>>
>>> > We are just to improve it.
>>>
>>> That's nice, but see above.
>>
>> Yes, we have fixed it.
>> And Yisheng Xie has feedback that this issue has been fixed.
>>
>> However, the v1 fix contains problem:
>> There is one line dependency not correct:
>>  acpidbg acpidump ec: include/acpi FORCE
>> I failed to correct it in v2/v3:
>>  $(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
>> Which was criticized by Andy, complaining new breaks for parallel builds.
>> Now in v4, it is corrected:
>>  $(objdir)%.o: %.c $(KERNEL_INCLUDE)
>>
>> However, in v2/v3, I cleaned up OUTPUT related code and changed its behavior of build with "O=".
>> In v2/v3, build "O=" from tools/power/acpi folder passes, but build from tools folder fails.
>> In v4, build "O=" from tools folder passes, but build from tools/power/acpi folder fails.
>>
>> There doesn't seem to be a solution to make both cases working.
>> Because the "descend" macro in tools/scripts/Makefile.include cannot handle both cases.
>> Stopping using "descend" macro in tools/power/acpi/Makefile could be the only possible fix...
>> So we only need one of them working and IMO v4 is suitable for Andy's needs.
>>
>> As a conclusion, v4 should be OK now.
>
> See above.
>
> This is how make started. $(KERNEL_TOOLS_TARGET) is a list of folders
> under tools (for now just "acpi gpio turbostat"). $(1) either
> "install" or "clean".
> KERNEL_SRC the output folder of kernel build (it might be the same as
> sources, but not my case, I used O=). The rest from the list are
> compiled and installed nicely. This *was* the case for acpi tools.
>
> +       $(Q)for t in $(KERNEL_TOOLS_TARGET); do                 \
> +               $(MAKE) -C $(KERNEL_SRC)                        \

$(MAKE) is something like make -jXX here

> +                       CROSS_COMPILE="$(TARGET_CROSS)"         \
> +                       DESTDIR="$(TARGET_DIR)"                 \
> +                       tools/$${t}_$(1);                       \
> +       done
>
> So. there is no magic here.
>
> Can you add this simple test to your test cases?
Lv Zheng Dec. 15, 2016, 3:25 a.m. UTC | #11
SGksIEFuZHkNCg0KR29vZCB0byBkaXNjdXNzL3N5bmNocm9uaXplIHdpdGggeW91IGluIGRldGFp
bHMgYXJvdW5kIHRoaXMgaXNzdWUuDQoNCj4gRnJvbTogQW5keSBTaGV2Y2hlbmtvIFttYWlsdG86
YW5keS5zaGV2Y2hlbmtvQGdtYWlsLmNvbV0NCj4gU3ViamVjdDogUmU6IEZpeCBpbiBBQ1BJQ0Eg
dG9vbHMgYnJva2UgY3Jvc3MgY29tcGlsYXRpb24gb2YgdG9vbHMvcG93ZXIvYWNwaQ0KPiANCj4g
T24gVGh1LCBOb3YgMTcsIDIwMTYgYXQgNToxMCBBTSwgWmhlbmcsIEx2IDxsdi56aGVuZ0BpbnRl
bC5jb20+IHdyb3RlOg0KPiA+Pg0KPiA+PiA+IEl0IGNvbnRhaW5zIGNvcnJlY3QgaGVhZGVyIGlu
Y2x1c2lvbiBjbGVhbnVwcy4NCj4gPj4NCj4gPj4gQ2xlYW51cHMgYXJlIG5vdCBhIGdvb2QgZW5v
dWdoIHJlYXNvbiBmb3IgYnJlYWtpbmcgYnVpbGRzLg0KPiA+DQo+ID4gSXQgZG9lc24ndCBicmVh
aywgYnVpbGQgaXMgT0sgaW4gbXkgZW52aXJvbm1lbnQuDQo+IA0KPiBZb3Uga25vdywgYmVmb3Jl
IHlvdSBkaWQgdGhhdCBmaXJzdCBjbGVhbiB1cCBmb3IgTVNWQyBpdCB3b3JrZWQgZm9yIG1lLg0K
PiANCj4gTm93IEkgZ290IHRoZSBmb2xsb3dpbmcNCj4gDQo+ICBERVNDRU5EICBwb3dlci9hY3Bp
DQo+ICBERVNDRU5EICB0b29scy9hY3BpZGJnDQo+ICBERVNDRU5EICB0b29scy9hY3BpZHVtcA0K
PiAgREVTQ0VORCAgdG9vbHMvZWMNCj4gIE1LRElSICAgIGluY2x1ZGUNCj4gIElOU1QgICAgIGFj
cGlkYmcNCj4gIE1LRElSICAgIGluY2x1ZGUNCj4gIElOU1QgICAgIGVjDQo+ICBNS0RJUiAgICBp
bmNsdWRlDQo+ICBDUCAgICAgICBpbmNsdWRlDQo+ICBDUCAgICAgICBpbmNsdWRlDQo+IC91c3Iv
YmluL2luc3RhbGw6IGNhbm5vdCBzdGF0ICcuLi4vcG93ZXIvYWNwaS9hY3BpZGJnJzogTm8gc3Vj
aCBmaWxlDQo+IG9yIGRpcmVjdG9yeQ0KPiAgSU5TVCAgICAgYWNwaWR1bXANCj4gLi4vLi4vTWFr
ZWZpbGUucnVsZXM6NDE6IHJlY2lwZSBmb3IgdGFyZ2V0ICdpbnN0YWxsLXRvb2xzJyBmYWlsZWQN
Cj4gbWFrZVs2XTogKioqIFtpbnN0YWxsLXRvb2xzXSBFcnJvciAxDQo+IC91c3IvYmluL2luc3Rh
bGw6IG1ha2VbNl06ICoqKiBXYWl0aW5nIGZvciB1bmZpbmlzaGVkIGpvYnMuLi4uDQo+IGNhbm5v
dCBzdGF0ICcuLi4vcG93ZXIvYWNwaS9lYyc6IE5vIHN1Y2ggZmlsZSBvciBkaXJlY3RvcnkNCj4g
Li4vLi4vTWFrZWZpbGUucnVsZXM6NDE6IHJlY2lwZSBmb3IgdGFyZ2V0ICdpbnN0YWxsLXRvb2xz
JyBmYWlsZWQNCj4gbWFrZVs2XTogKioqIFtpbnN0YWxsLXRvb2xzXSBFcnJvciAxDQo+IG1ha2Vb
Nl06ICoqKiBXYWl0aW5nIGZvciB1bmZpbmlzaGVkIGpvYnMuLi4uDQo+ICBJTlNUICAgICBhY3Bp
ZHVtcC44DQo+ICBDUCAgICAgICBpbmNsdWRlDQo+IE1ha2VmaWxlOjIzOiByZWNpcGUgZm9yIHRh
cmdldCAnYWNwaWRiZ19pbnN0YWxsJyBmYWlsZWQNCj4gbWFrZVs1XTogKioqIFthY3BpZGJnX2lu
c3RhbGxdIEVycm9yIDINCj4gbWFrZVs1XTogKioqIFdhaXRpbmcgZm9yIHVuZmluaXNoZWQgam9i
cy4uLi4NCj4gL3Vzci9iaW4vaW5zdGFsbDogY2Fubm90IHN0YXQgJy4uLi9wb3dlci9hY3BpL2Fj
cGlkdW1wJzogTm8gc3VjaCBmaWxlDQo+IG9yIGRpcmVjdG9yeQ0KPiAuLi8uLi9NYWtlZmlsZS5y
dWxlczo0MTogcmVjaXBlIGZvciB0YXJnZXQgJ2luc3RhbGwtdG9vbHMnIGZhaWxlZA0KPiBtYWtl
WzZdOiAqKiogW2luc3RhbGwtdG9vbHNdIEVycm9yIDENCj4gbWFrZVs2XTogKioqIFdhaXRpbmcg
Zm9yIHVuZmluaXNoZWQgam9icy4uLi4NCj4gTWFrZWZpbGU6MjM6IHJlY2lwZSBmb3IgdGFyZ2V0
ICdlY19pbnN0YWxsJyBmYWlsZWQNCj4gbWFrZVs1XTogKioqIFtlY19pbnN0YWxsXSBFcnJvciAy
DQo+IE1ha2VmaWxlOjIzOiByZWNpcGUgZm9yIHRhcmdldCAnYWNwaWR1bXBfaW5zdGFsbCcgZmFp
bGVkDQo+IG1ha2VbNV06ICoqKiBbYWNwaWR1bXBfaW5zdGFsbF0gRXJyb3IgMg0KPiBNYWtlZmls
ZTo5NTogcmVjaXBlIGZvciB0YXJnZXQgJ2FjcGlfaW5zdGFsbCcgZmFpbGVkDQo+IG1ha2VbNF06
ICoqKiBbYWNwaV9pbnN0YWxsXSBFcnJvciAyDQo+IA0KPiAuLi4vTWFrZWZpbGU6MTYxMzogcmVj
aXBlIGZvciB0YXJnZXQgJ3Rvb2xzL2FjcGlfaW5zdGFsbCcgZmFpbGVkDQo+IA0KPiA+Pj4gSnVz
dCB0byBub3RpY2UgdGhhdCBhYm92ZSBNYWtlZmlsZSBjb21lcyBmcm9tIHRoZSBzb3VyY2UgdHJl
ZSBub3QgdGhlIE89IHZhcmlhbnQNCj4gVGhlIHJlc3Qgb2YgLi4uIHJlbGF0ZWQgdG8gTz0vbXkv
b3V0cHV0L3BhdGguDQo+IA0KPiBtYWtlWzNdOiAqKiogW3Rvb2xzL2FjcGlfaW5zdGFsbF0gRXJy
b3IgMg0KPiBNYWtlZmlsZToxNTA6IHJlY2lwZSBmb3IgdGFyZ2V0ICdzdWItbWFrZScgZmFpbGVk
DQo+IG1ha2VbMl06ICoqKiBbc3ViLW1ha2VdIEVycm9yIDINCj4gTWFrZWZpbGU6MjQ6IHJlY2lw
ZSBmb3IgdGFyZ2V0ICdfX3N1Yi1tYWtlJyBmYWlsZWQNCj4gbWFrZVsxXTogKioqIFtfX3N1Yi1t
YWtlXSBFcnJvciAyDQo+IA0KPiBTbywgaXQgZG9lc24ndCBldmVuIHRyeSB0byBidWlsZCBhbnl0
aGluZy4NCg0KVGhlIHJlc3VsdCBkZXBlbmRzIG9uIHdoZXJlIHlvdSBzdGFydCB5b3VyIE89IGNv
bW1hbmQuDQpJbiB5b3VyIHByZXZpb3VzIHJlcG9ydCwgeW91IHByZWZlcnJlZCB0b29scyBmb2xk
ZXIuDQpXaGlsZSB5b3UgcmVwb3J0ZWQgZmFpbHVyZXMgYWdhaW5zdCBteSBvbGQgcGF0Y2ggd2hp
Y2ggYWxsb3dzIE89IGNvbW1hbmQgZnJvbSB0b29scy9wb3dlci9hY3BpDQpDYW4geW91IHRlbGwg
bWUgeW91ciBjdXJyZW50IGRpcmVjdG9yeSBpbiB0aGlzIGNhc2U/DQoNCj4gDQo+ID4gWWVzLiBJ
TU8sDQo+ID4gVGhlIGFib3ZlIGJyZWFrYWdlIGxvb2tzIG11Y2ggbW9yZSBzdHJhbmdlIHRoYW4g
dGhlIGNvbW1pdCBpdHNlbGYuDQo+ID4gSXQncyBiZXR0ZXIgdG8gbWFrZSB0aGUgYnVpbGQgc3Vj
Y2Vzc2Z1bCB3aXRob3V0IGNvbW1lbnRpbmcgdGhpcyBsaW5lIG91dC4NCj4gPg0KPiA+IFN1Y2gg
a2luZCBvZiByZWdyZXNzaW9ucyBib3RoZXJlZCBtZSBhIGxvdC4NCj4gPiBJdCBzZWVtcyB3ZSBj
YW4gZWFzaWx5IGZlbGwgaW50byBzdWNoIG9sZCB0cmFwcy4NCj4gPg0KPiA+IElmIEkgY2Fubm90
IGZpeCB0aGUgbmV3IHJlZ3Jlc3Npb24gaW4gdGltZSwgSSB3aWxsIHN1cmUgc3VnZ2VzdCB0byBy
ZXZlcnQgYnJlYWthZ2UgY29tbWl0IGJlZm9yZSBhDQo+IGJldHRlciBzb2x1dGlvbiBjYW4gYmUg
b2ZmZXJlZCBhZnRlciBpbnZlc3RpZ2F0aW9uLg0KPiA+IEhvd2V2ZXIsIGZvciB0aGlzIGlzc3Vl
Og0KPiA+IDEuIFRoZXJlIGlzIGFjdHVhbGx5IG5vIHNlcmlvdXMgYnJlYWthZ2UgKG9ubHkgdG9v
bGNoYWlucyBidWlsdCB3aXRoIGluY29tcGxpYW50IGtlcm5lbCBoZWFkZXJzKQ0KPiA+IDIuIFdl
IGhhdmUgZml4ZWQgdGhlIGlzc3VlLCBidXQgdGhlIGZpeCBuZWVkIHRvIGJlIGltcHJvdmVkLg0K
PiA+IFNvIHJldmVydGluZyB0aGUgY29ycmVjdCBjb21taXQgaXNuJ3QgYSBnb29kIGlkZWEuDQo+
ID4gUmV2ZXJ0aW5nIGl0IG1heSB0cmlnZ2VyIG1vcmUgc2VyaW91cyBpc3N1ZXMgYmVjYXVzZSBv
ZiB3cm9uZyBoZWFkZXIgaW5jbHVzaW9uIG9yZGVycy4NCj4gPiBMZXQgbWUgaW1wcm92ZSBpdC4N
Cj4gPg0KPiA+Pg0KPiA+PiA+IFdlIGNhbiBzZWUgbWFueSB0b29scyBidWlsZCBicm9rZW4gZm9y
IGNyb3NzLWNvbXBpbGVycywgYW5kIHRvb2xzL3Bvd2VyL2FjcGkgaXMgbm90IHRoZSBvbmx5IG9u
ZS4NCj4gPj4NCj4gPj4gSSBndWVzcyB0aGUgaXNzdWUgaXMgdGhhdCBpdCB3YXMgbm90IGJyb2tl
biBiZWZvcmUgYW5kIGl0IGlzIGJyb2tlbg0KPiA+PiBhZnRlciB5b3VyIGNoYW5nZXMuICBUaGF0
J3MgcXVpdGUgYSBiaXQgb2YgYSBkaWZmZXJlbmNlLg0KPiA+DQo+ID4gSU1PLCB0aGUgbm9uLXVw
ZGF0ZWQgdG9vbGNoYWlucy9vciBBQ1BJQ0EncyBzWC91WCBkZWZpbml0aW9ucyBhcmUgYWN0dWFs
bHkgYnVnZ3kgd2hpbGUgdGhlIGNvbW1pdA0KPiBpc24ndC4NCj4gPg0KPiA+Pg0KPiA+PiA+IFdl
IGFyZSBqdXN0IHRvIGltcHJvdmUgaXQuDQo+ID4+DQo+ID4+IFRoYXQncyBuaWNlLCBidXQgc2Vl
IGFib3ZlLg0KPiA+DQo+ID4gWWVzLCB3ZSBoYXZlIGZpeGVkIGl0Lg0KPiA+IEFuZCBZaXNoZW5n
IFhpZSBoYXMgZmVlZGJhY2sgdGhhdCB0aGlzIGlzc3VlIGhhcyBiZWVuIGZpeGVkLg0KPiA+DQo+
ID4gSG93ZXZlciwgdGhlIHYxIGZpeCBjb250YWlucyBwcm9ibGVtOg0KPiA+IFRoZXJlIGlzIG9u
ZSBsaW5lIGRlcGVuZGVuY3kgbm90IGNvcnJlY3Q6DQo+ID4gIGFjcGlkYmcgYWNwaWR1bXAgZWM6
IGluY2x1ZGUvYWNwaSBGT1JDRQ0KPiA+IEkgZmFpbGVkIHRvIGNvcnJlY3QgaXQgaW4gdjIvdjM6
DQo+ID4gICQoT1VUUFVUKSQoVE9PTCk6ICQoS0VSTkVMX0lOQ0xVREUpICQodG9vbG9ianMpIEZP
UkNFDQo+ID4gV2hpY2ggd2FzIGNyaXRpY2l6ZWQgYnkgQW5keSwgY29tcGxhaW5pbmcgbmV3IGJy
ZWFrcyBmb3IgcGFyYWxsZWwgYnVpbGRzLg0KPiA+IE5vdyBpbiB2NCwgaXQgaXMgY29ycmVjdGVk
Og0KPiA+ICAkKG9iamRpciklLm86ICUuYyAkKEtFUk5FTF9JTkNMVURFKQ0KPiA+DQo+ID4gSG93
ZXZlciwgaW4gdjIvdjMsIEkgY2xlYW5lZCB1cCBPVVRQVVQgcmVsYXRlZCBjb2RlIGFuZCBjaGFu
Z2VkIGl0cyBiZWhhdmlvciBvZiBidWlsZCB3aXRoICJPPSIuDQo+ID4gSW4gdjIvdjMsIGJ1aWxk
ICJPPSIgZnJvbSB0b29scy9wb3dlci9hY3BpIGZvbGRlciBwYXNzZXMsIGJ1dCBidWlsZCBmcm9t
IHRvb2xzIGZvbGRlciBmYWlscy4NCj4gPiBJbiB2NCwgYnVpbGQgIk89IiBmcm9tIHRvb2xzIGZv
bGRlciBwYXNzZXMsIGJ1dCBidWlsZCBmcm9tIHRvb2xzL3Bvd2VyL2FjcGkgZm9sZGVyIGZhaWxz
Lg0KPiA+DQo+ID4gVGhlcmUgZG9lc24ndCBzZWVtIHRvIGJlIGEgc29sdXRpb24gdG8gbWFrZSBi
b3RoIGNhc2VzIHdvcmtpbmcuDQo+ID4gQmVjYXVzZSB0aGUgImRlc2NlbmQiIG1hY3JvIGluIHRv
b2xzL3NjcmlwdHMvTWFrZWZpbGUuaW5jbHVkZSBjYW5ub3QgaGFuZGxlIGJvdGggY2FzZXMuDQo+
ID4gU3RvcHBpbmcgdXNpbmcgImRlc2NlbmQiIG1hY3JvIGluIHRvb2xzL3Bvd2VyL2FjcGkvTWFr
ZWZpbGUgY291bGQgYmUgdGhlIG9ubHkgcG9zc2libGUgZml4Li4uDQo+ID4gU28gd2Ugb25seSBu
ZWVkIG9uZSBvZiB0aGVtIHdvcmtpbmcgYW5kIElNTyB2NCBpcyBzdWl0YWJsZSBmb3IgQW5keSdz
IG5lZWRzLg0KPiA+DQo+ID4gQXMgYSBjb25jbHVzaW9uLCB2NCBzaG91bGQgYmUgT0sgbm93Lg0K
PiANCj4gU2VlIGFib3ZlLg0KPiANCj4gVGhpcyBpcyBob3cgbWFrZSBzdGFydGVkLiAkKEtFUk5F
TF9UT09MU19UQVJHRVQpIGlzIGEgbGlzdCBvZiBmb2xkZXJzDQo+IHVuZGVyIHRvb2xzIChmb3Ig
bm93IGp1c3QgImFjcGkgZ3BpbyB0dXJib3N0YXQiKS4gJCgxKSBlaXRoZXINCj4gImluc3RhbGwi
IG9yICJjbGVhbiIuDQo+IEtFUk5FTF9TUkMgdGhlIG91dHB1dCBmb2xkZXIgb2Yga2VybmVsIGJ1
aWxkIChpdCBtaWdodCBiZSB0aGUgc2FtZSBhcw0KPiBzb3VyY2VzLCBidXQgbm90IG15IGNhc2Us
IEkgdXNlZCBPPSkuIFRoZSByZXN0IGZyb20gdGhlIGxpc3QgYXJlDQo+IGNvbXBpbGVkIGFuZCBp
bnN0YWxsZWQgbmljZWx5LiBUaGlzICp3YXMqIHRoZSBjYXNlIGZvciBhY3BpIHRvb2xzLg0KPiAN
Cj4gKyAgICAgICAkKFEpZm9yIHQgaW4gJChLRVJORUxfVE9PTFNfVEFSR0VUKTsgZG8gICAgICAg
ICAgICAgICAgIFwNCj4gKyAgICAgICAgICAgICAgICQoTUFLRSkgLUMgJChLRVJORUxfU1JDKSAg
ICAgICAgICAgICAgICAgICAgICAgIFwNCj4gKyAgICAgICAgICAgICAgICAgICAgICAgQ1JPU1Nf
Q09NUElMRT0iJChUQVJHRVRfQ1JPU1MpIiAgICAgICAgIFwNCj4gKyAgICAgICAgICAgICAgICAg
ICAgICAgREVTVERJUj0iJChUQVJHRVRfRElSKSIgICAgICAgICAgICAgICAgIFwNCj4gKyAgICAg
ICAgICAgICAgICAgICAgICAgdG9vbHMvJCR7dH1fJCgxKTsgICAgICAgICAgICAgICAgICAgICAg
IFwNCj4gKyAgICAgICBkb25lDQo+IA0KPiBTby4gdGhlcmUgaXMgbm8gbWFnaWMgaGVyZS4NCg0K
U28gaXQgc2VlbXMgeW91IHN0YXkgaW4gb2JqdHJlZSB0byBkbyB0aGlzIHRlc3QuDQpXaGljaCBp
cyBub3QgaGFuZGxlZCBieSB0aGUgcHJldmlvdXMgY29tbWl0Lg0KVGhhdCBjb21taXQgb25seSBo
YW5kbGVzIG1ha2UgaW4gInRvb2xzIiBmb2xkZXIuDQoNCj4gDQo+IENhbiB5b3UgYWRkIHRoaXMg
c2ltcGxlIHRlc3QgdG8geW91ciB0ZXN0IGNhc2VzPw0KDQpUaGUgcHJvYmxlbSBpcyB0aGUgYnVn
Z3kgZGVzY2VuZCBtYWNybywgd2hpY2ggaGFzIHNvbWUgY29uZmxpY3RzIGFnYWluc3QgdGhlIG5l
ZWRzIG9mIHB1dHRpbmcgdGhlIG5ldyBpbmNsdWRlIGZvbGRlciB0byBvYmp0cmVlLg0KVGhlIGRl
c2NlbmQgbWFjcm8gaXMgdXNlZCBpbiB0b29scy9NYWtlZmlsZSwgYW5kIGluIG9yZGVyIHRvIGF2
b2lkIHVzaW5nIGV4cG9ydCAod2hpY2ggbG9va3MgdmVyeSBiYWQgaW4gTWFrZWZpbGVzKSwgd2Ug
cmV1c2VkIGl0IGluIHRvb2xzL3Bvd2VyL2FjcGkvTWFrZWZpbGUuDQphY3BpZGJnIGFjcGlkdW1w
IGVjOiBGT1JDRQ0KCSQoY2FsbCBkZXNjZW5kLHRvb2xzLyRALGFsbCkNCi4uLg0KDQpJdCB3b3Jr
ZWQgcHJldmlvdXNseSBiZWNhdXNlIHdlIGhhdmUgbWFnaWMgaW4gdG9vbHMvcG93ZXIvYWNwaS9N
YWtlZmlsZSB0byB3b3JrIHRoZSBkZXNjZW5kIG1hY3JvIGFyb3VuZC4NClRoZSBuZXcgYWNwaSBt
YWtlZmlsZXMgYWN0dWFsbHkgdXNlcyBkaWZmZXJlbnQgb3V0cHV0IGZvbGRlciBmcm9tIGRpZmZl
cmVudCBjdXJyZW50IGRpcmVjdG9yeSBhbmQgZGlmZmVyZW50IE89IGFzc2lnbm1lbnRzLg0KDQpX
aGlsZSB3ZSBuZWVkIGEgc3RhdGlvbmFyeSBkaXJlY3RvcnkgZm9yIHRoZSBuZXcgaW5jbHVkZSBm
b2xkZXIgdG8gYmUgdXNlZCB3aXRoIC1JIHBhcmFtZXRlci4NCklmIHRoZSBuZXcgaW5jbHVkZSBp
cyBpbiAic3JjdHJlZSIgZm9sZGVycywgdGhlcmUgd2lsbCBiZSBubyBwcm9ibGVtLCBob3dldmVy
IGl0IHBvbGx1dGVzIHNyY3RyZWUuDQooVGhhdCdzIHdoeSBJIGFza2VkIHlvdXIgbmVlZCAtIGRv
IHlvdSB3YW50IHRvIHB1dCB0aGUgaW5jbHVkZSBmb2xkZXIgdG8gb2JqdHJlZS4gQnV0IHlvdSBk
aWRuJ3QgcmVwbHkuKQ0KSWYgdGhlIG5ldyBpbmNsdWRlIGlzIGluICJvYmp0cmVlIiBmb2xkZXJz
LCBJJ20gbm90IGFibGUgdG8gZmluZCBhIHdheSB0byBtYWtlIGl0IHN0YXRpb25hcnkgYmVjYXVz
ZSBvZiBhYm92ZSBtYWdpYy4NCg0KVGhlIHByb2JsZW0gaXMgaW4gdG9vbHMvc2NyaXB0cy9NYWtl
ZmlsZS5pbmNsdWRlIHJlbGF0ZWQgdG8gb2JqdHJlZToNCg0KaWZuZXEgKCQoTyksKQ0KaWZlcSAo
JChvcmlnaW4gTyksIGNvbW1hbmQgbGluZSkNCglkdW1teSA6PSAkKGlmICQoc2hlbGwgdGVzdCAt
ZCAkKE8pIHx8IGVjaG8gJChPKSksJChlcnJvciBPPSQoTykgZG9lcyBub3QgZXhpc3QpLCkNCglB
QlNPTFVURV9PIDo9ICQoc2hlbGwgY2QgJChPKSA7IHB3ZCkNCglPVVRQVVQgOj0gJChBQlNPTFVU
RV9PKS8kKGlmICQoc3ViZGlyKSwkKHN1YmRpcikvKQ0KCUNPTU1BTkRfTyA6PSBPPSQoQUJTT0xV
VEVfTykNCmlmZXEgKCQob2JqdHJlZSksKQ0KCW9ianRyZWUgOj0gJChPKQ0KZW5kaWYNCmVuZGlm
DQplbmRpZg0KDQojIGNoZWNrIHRoYXQgdGhlIG91dHB1dCBkaXJlY3RvcnkgYWN0dWFsbHkgZXhp
c3RzDQppZm5lcSAoJChPVVRQVVQpLCkNCk9VVERJUiA6PSAkKHNoZWxsIGNkICQoT1VUUFVUKSAm
JiAvYmluL3B3ZCkNCiQoaWYgJChPVVRESVIpLCwgJChlcnJvciBvdXRwdXQgZGlyZWN0b3J5ICIk
KE9VVFBVVCkiIGRvZXMgbm90IGV4aXN0KSkNCkVuZGlmDQoNCjEuIFdoZW4gd2Ugc3RheSBpbiAi
dG9vbHMiIGZvbGRlciwgYW5kIE8gaXMgb21pdC4NCiAgIERlc2NlbmQgaW52b2tlZCBmcm9tIHRv
b2xzL01ha2VmaWxlIGNhdXNlczogb2JqdHJlZT10b29scy4NCjIuIFdoZW4gd2Ugc3RheSBpbiAi
dG9vbHMiIGZvbGRlciwgYW5kIE8gaXMgIi90bXAiLg0KICAgRGVzY2VuZCBpbnZva2VkIGZyb20g
dG9vbHMvTWFrZWZpbGUgY2F1c2VzOiBvYmp0cmVlPS90bXAuDQozLiBXaGVuIHdlIHN0YXkgaW4g
InRvb2xzL3Bvd2VyL2FjcGkiIGZvbGRlciwgYW5kIE8gaXMgb21pdC4NCiAgIERlc2NlbmQgaW52
b2tlZCBmcm9tIHRvb2xzL3Bvd2VyL2FjcGkvTWFrZWZpbGUgY2F1c2VzOiBvYmp0cmVlPXRvb2xz
L3Bvd2VyL2FjcGkuDQo0LiBXaGVuIHdlIHN0YXkgaW4gInRvb2xzL3Bvd2VyL2FjcGkiIGZvbGRl
ciwgYW5kIE8gaXMgb21pdC4NCiAgIERlc2NlbmQgaW52b2tlZCBmcm9tIHRvb2xzL3Bvd2VyL2Fj
cGkvTWFrZWZpbGUgY2F1c2VzOiBvYmp0cmVlPS90bXAuDQo1LiBOb3cgeW91IHdhbnQgdG8gc3Rh
eSBpbiBPIGZvbGRlciwgYW5kIE8gaXMgL3RtcC4NCiAgIERlc2NlbmQgaW52b2tlZCBmcm9tIHRv
b2xzL01ha2VmaWxlIGNhdXNlczogb2JqdHJlZT0vdG1wLg0KDQpTbyBpZiB3ZSB3YW50IHRvIHB1
dCB0aGUgaW5jbHVkZSBmb2xkZXIgdG8gb2JqdHJlZSwgd2hhdCBzaG91bGQgaXQgYmU/DQpGb3Ig
ZXhhbXBsZSwgaWYgd2UgcHV0IGluY2x1ZGUgZm9sZGVyIGluIG9ianRyZWUvcG93ZXIvYWNwaS9p
bmNsdWRlLCB0aGUgaXQgcHJvYmFibHkgd2lsbCBmYWlsIGNhc2UgNC4NCkFub3RoZXIgZXhhbXBs
ZSBpcywgaWYgd2UgcHV0IGluY2x1ZGUgZm9sZGVyIGluIG9ianRyZWUvaW5jbHVkZSwgdGhlbiAi
bWFrZSBjbGVhbiIgcHJvYmFibHkgd2lsbCBkZWxldGUgdGhlIHNvdXJjZSB0cmVlIGluIGNhc2Ug
MS4NCg0KVGhlIE89IGhhbmRsaW5nIGluIHRvb2xzL3NjcmlwdHMvTWFrZWZpbGUuaW5jbHVkZSBp
cyB0aGUgcm9vdCBjYXVzZSBvZiB0aGUgaXNzdWUgcmVwb3J0ZWQgYnkgeW91LCBJTU8uDQpEbyB5
b3UgaGF2ZSBzdWdnZXN0aW9ucyBhYm91dCBob3cgY2FuIHdlIGdldCBhbGwgY2FzZXMgd29ya2lu
ZyB3aGlsZSBzdGlsbCBjYW4gcHV0IGluY2x1ZGUgZm9sZGVyIGluIG9ianRyZWU/DQpJJ2xsIGFs
c28gdHJ5IGEgZGlmZmVyZW50IGFwcHJvYWNoIHRvIHNlZSBpZiB0aGUgcHV6emxlIGNhbiBiZSBz
b2x2ZWQuDQoNClRoYW5rcyBhbmQgYmVzdCByZWdhcmRzDQpMdg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng Dec. 15, 2016, 3:30 a.m. UTC | #12
Hi, Andy

For parallel build, it should be handled by the previous fix.
The only problem is the O handling IMO.
See my previous reply.
Maybe I should stop using descend in tools/power/acpi/Makefile.

Thanks and best regards
Lv

> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> Subject: Re: Fix in ACPICA tools broke cross compilation of tools/power/acpi

> 

> On Mon, Dec 12, 2016 at 10:58 PM, Andy Shevchenko

> <andy.shevchenko@gmail.com> wrote:

> > On Thu, Nov 17, 2016 at 5:10 AM, Zheng, Lv <lv.zheng@intel.com> wrote:

> >>>

> >>> > It contains correct header inclusion cleanups.

> >>>

> >>> Cleanups are not a good enough reason for breaking builds.

> >>

> >> It doesn't break, build is OK in my environment.

> >

> > You know, before you did that first clean up for MSVC it worked for me.

> >

> > Now I got the following

> 

> I serialized make in my example to be make -j1 and got the following

> 

>  DESCEND  power/acpi

>  DESCEND  tools/acpidbg

>  CC       tools/acpidbg/acpidbg.o

> Assembler messages:

> Fatal error: can't create .../power/acpi/tools/acpidbg/acpidbg.o: No

> such file or dir

> ectory

> ../../Makefile.rules:26: recipe for target

> '.../power/acpi/tools/acpidbg/acpidbg.o' f

> ailed

> 

> As you may already notice from the below there is a wrong folder used.

> The hierarchy is

> 

> % ls -R tools/power/

> tools/power/:

> acpi  x86

> 

> tools/power/acpi:

> tools

> 

> tools/power/acpi/tools:

> acpidbg  acpidump  ec

> 

> tools/power/acpi/tools/acpidbg:

> 

> tools/power/acpi/tools/acpidump:

> 

> tools/power/acpi/tools/ec:

> 

> tools/power/x86:

> turbostat

> 

> 

> >

> >  DESCEND  power/acpi

> >  DESCEND  tools/acpidbg

> >  DESCEND  tools/acpidump

> >  DESCEND  tools/ec

> >  MKDIR    include

> >  INST     acpidbg

> >  MKDIR    include

> >  INST     ec

> >  MKDIR    include

> >  CP       include

> >  CP       include

> > /usr/bin/install: cannot stat '.../power/acpi/acpidbg': No such file

> > or directory

> >  INST     acpidump

> > ../../Makefile.rules:41: recipe for target 'install-tools' failed

> > make[6]: *** [install-tools] Error 1

> > /usr/bin/install: make[6]: *** Waiting for unfinished jobs....

> > cannot stat '.../power/acpi/ec': No such file or directory

> > ../../Makefile.rules:41: recipe for target 'install-tools' failed

> > make[6]: *** [install-tools] Error 1

> > make[6]: *** Waiting for unfinished jobs....

> >  INST     acpidump.8

> >  CP       include

> > Makefile:23: recipe for target 'acpidbg_install' failed

> > make[5]: *** [acpidbg_install] Error 2

> > make[5]: *** Waiting for unfinished jobs....

> > /usr/bin/install: cannot stat '.../power/acpi/acpidump': No such file

> > or directory

> > ../../Makefile.rules:41: recipe for target 'install-tools' failed

> > make[6]: *** [install-tools] Error 1

> > make[6]: *** Waiting for unfinished jobs....

> > Makefile:23: recipe for target 'ec_install' failed

> > make[5]: *** [ec_install] Error 2

> > Makefile:23: recipe for target 'acpidump_install' failed

> > make[5]: *** [acpidump_install] Error 2

> > Makefile:95: recipe for target 'acpi_install' failed

> > make[4]: *** [acpi_install] Error 2

> >

> > .../Makefile:1613: recipe for target 'tools/acpi_install' failed

> >

> >>>> Just to notice that above Makefile comes from the source tree not the O= variant

> > The rest of ... related to O=/my/output/path.

> >

> > make[3]: *** [tools/acpi_install] Error 2

> > Makefile:150: recipe for target 'sub-make' failed

> > make[2]: *** [sub-make] Error 2

> > Makefile:24: recipe for target '__sub-make' failed

> > make[1]: *** [__sub-make] Error 2

> >

> > So, it doesn't even try to build anything.

> >

> >> Yes. IMO,

> >> The above breakage looks much more strange than the commit itself.

> >> It's better to make the build successful without commenting this line out.

> >>

> >> Such kind of regressions bothered me a lot.

> >> It seems we can easily fell into such old traps.

> >>

> >> If I cannot fix the new regression in time, I will sure suggest to revert breakage commit before a

> better solution can be offered after investigation.

> >> However, for this issue:

> >> 1. There is actually no serious breakage (only toolchains built with incompliant kernel headers)

> >> 2. We have fixed the issue, but the fix need to be improved.

> >> So reverting the correct commit isn't a good idea.

> >> Reverting it may trigger more serious issues because of wrong header inclusion orders.

> >> Let me improve it.

> >>

> >>>

> >>> > We can see many tools build broken for cross-compilers, and tools/power/acpi is not the only one.

> >>>

> >>> I guess the issue is that it was not broken before and it is broken

> >>> after your changes.  That's quite a bit of a difference.

> >>

> >> IMO, the non-updated toolchains/or ACPICA's sX/uX definitions are actually buggy while the commit

> isn't.

> >>

> >>>

> >>> > We are just to improve it.

> >>>

> >>> That's nice, but see above.

> >>

> >> Yes, we have fixed it.

> >> And Yisheng Xie has feedback that this issue has been fixed.

> >>

> >> However, the v1 fix contains problem:

> >> There is one line dependency not correct:

> >>  acpidbg acpidump ec: include/acpi FORCE

> >> I failed to correct it in v2/v3:

> >>  $(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE

> >> Which was criticized by Andy, complaining new breaks for parallel builds.

> >> Now in v4, it is corrected:

> >>  $(objdir)%.o: %.c $(KERNEL_INCLUDE)

> >>

> >> However, in v2/v3, I cleaned up OUTPUT related code and changed its behavior of build with "O=".

> >> In v2/v3, build "O=" from tools/power/acpi folder passes, but build from tools folder fails.

> >> In v4, build "O=" from tools folder passes, but build from tools/power/acpi folder fails.

> >>

> >> There doesn't seem to be a solution to make both cases working.

> >> Because the "descend" macro in tools/scripts/Makefile.include cannot handle both cases.

> >> Stopping using "descend" macro in tools/power/acpi/Makefile could be the only possible fix...

> >> So we only need one of them working and IMO v4 is suitable for Andy's needs.

> >>

> >> As a conclusion, v4 should be OK now.

> >

> > See above.

> >

> > This is how make started. $(KERNEL_TOOLS_TARGET) is a list of folders

> > under tools (for now just "acpi gpio turbostat"). $(1) either

> > "install" or "clean".

> > KERNEL_SRC the output folder of kernel build (it might be the same as

> > sources, but not my case, I used O=). The rest from the list are

> > compiled and installed nicely. This *was* the case for acpi tools.

> >

> > +       $(Q)for t in $(KERNEL_TOOLS_TARGET); do                 \

> > +               $(MAKE) -C $(KERNEL_SRC)                        \

> 

> $(MAKE) is something like make -jXX here

> 

> > +                       CROSS_COMPILE="$(TARGET_CROSS)"         \

> > +                       DESTDIR="$(TARGET_DIR)"                 \

> > +                       tools/$${t}_$(1);                       \

> > +       done

> >

> > So. there is no magic here.

> >

> > Can you add this simple test to your test cases?

> 

> 

> --

> With Best Regards,

> Andy Shevchenko
diff mbox

Patch

--- a/tools/power/acpi/Makefile.rules
+++ b/tools/power/acpi/Makefile.rules
@@ -8,9 +8,9 @@ 
# as published by the Free Software Foundation; version 2
# of the License.

-objdir := $(OUTPUT)tools/$(TOOL)/
+objdir := $(OUTPUT)tools/power/acpi/tools/$(TOOL)/
toolobjs := $(addprefix $(objdir),$(TOOL_OBJS))
-$(OUTPUT)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
+$(objdir)$(TOOL): $(KERNEL_INCLUDE) $(toolobjs) FORCE
       $(ECHO) "  LD      " $(subst $(OUTPUT),,$@)
       $(QUIET) $(LD) $(CFLAGS) $(LDFLAGS) $(toolobjs) -L$(OUTPUT) -o $@
       $(ECHO) "  STRIP   " $(subst $(OUTPUT),,$@)
@@ -26,21 +26,21 @@  $(objdir)%.o: %.c
       $(ECHO) "  CC      " $(subst $(OUTPUT),,$@)
       $(QUIET) $(CC) -c $(CFLAGS) -o $@ $<

-all: $(OUTPUT)$(TOOL)
+all: $(objdir)$(TOOL)
clean:
       $(ECHO) "  RMOBJ   " $(subst $(OUTPUT),,$(objdir))
       $(QUIET) find $(objdir) \( -not -type d \)\
                -and \( -name '*~' -o -name '*.[oas]' \)\
                -type f -print | xargs rm -f
       $(ECHO) "  RM      " $(TOOL)
-       $(QUIET) rm -f $(OUTPUT)$(TOOL)
+       $(QUIET) rm -f $(objdir)$(TOOL)
       $(ECHO) "  RMINC   " $(subst $(OUTPUT),,$(KERNEL_INCLUDE))
       $(QUIET) rm -rf $(KERNEL_INCLUDE)

install-tools:
       $(ECHO) "  INST    " $(TOOL)
       $(QUIET) $(INSTALL) -d $(DESTDIR)$(sbindir)
-       $(QUIET) $(INSTALL_PROGRAM) $(OUTPUT)$(TOOL) $(DESTDIR)$(sbindir)
+       $(QUIET) $(INSTALL_PROGRAM) $(objdir)$(TOOL) $(DESTDIR)$(sbindir)
uninstall-tools:
       $(ECHO) "  UNINST  " $(TOOL)
       $(QUIET) rm -f $(DESTDIR)$(sbindir)/$(TOOL)