packages: fix and improve support for top-level parallel makefile

PATCH Superseded Fabio Porcedda 2 participants 5 messages Apr 6, 2015 – Apr 21, 2015 Kbuild

Share & tools

Fabio Porcedda

author

2015-04-06 13:50 UTC #0 lore raw

PATCH 1/1 packages: fix and improve support for top-level parallel makefile

To fix packages that fail to build when PARALLEL_JOBS is empty instead of using an empty PARALLEL_JOBS just avoid to using it in the MAKE variable.

Check the MAKEFLAGS variable to know automatically if the -j option is being used, but use the "=" operator instead of the ":=" operator because it can be checked only in a "recursively expanded variable". Use "override" because otherwise it's impossible to change the automatic variable "MAKE".

Signed-off-by Fabio Porcedda <[email protected]>

---
 Makefile            | 4 ++--
 package/Makefile.in | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

Makefile +2 -2

index 8d09725..6c2dcca 100644
@@ -58,8 +58,8 @@ export HOSTARCH := $(shell uname -m | \
 #
 # Taking into account the above considerations, if you still want to execute
 # this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# build using the following command:
-#	make BR2_JLEVEL= -j$((`getconf _NPROCESSORS_ONLN`+1))
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 
 # absolute path

package/Makefile.in +2 -1

index fc57427..ed68e35 100644
@@ -18,7 +18,8 @@ PARALLEL_JOBS := $(BR2_JLEVEL)
 endif
 
 MAKE1 := $(HOSTMAKE) -j1
-MAKE := $(HOSTMAKE) $(if $(PARALLEL_JOBS),-j$(PARALLEL_JOBS))
+override MAKE = $(HOSTMAKE) \
+	$(if $(findstring j,$(filter-out --%,$(MAKEFLAGS))),,-j$(PARALLEL_JOBS))
 
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 TARGET_VENDOR = $(call qstrip,$(BR2_TOOLCHAIN_BUILDROOT_VENDOR))

Signature · 1 line

2.3.4

Fabio Porcedda

author

2015-04-20 03:27 UTC #1 lore raw

PATCH 1/1 packages: fix and improve support for top-level parallel makefile

Quoted 46 lines · Fabio Porcedda

To fix packages that fail to build when PARALLEL_JOBS is empty instead of using an empty PARALLEL_JOBS just avoid to using it in the MAKE variable.
Check the MAKEFLAGS variable to know automatically if the -j option is being used, but use the "=" operator instead of the ":=" operator because it can be checked only in a "recursively expanded variable". Use "override" because otherwise it's impossible to change the automatic variable "MAKE".
Signed-off-by Fabio Porcedda <[email protected]>
---
 Makefile            | 4 ++--
 package/Makefile.in | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
Makefile +2 -2
index 8d09725..6c2dcca 100644
@@ -58,8 +58,8 @@ export HOSTARCH := $(shell uname -m | \
 #
 # Taking into account the above considerations, if you still want to execute
 # this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# build using the following command:
-#      make BR2_JLEVEL= -j$((`getconf _NPROCESSORS_ONLN`+1))
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 
 # absolute path
package/Makefile.in +2 -2
index fc57427..ed68e35 100644
@@ -18,7 +18,8 @@ PARALLEL_JOBS := $(BR2_JLEVEL)
 endif
 
 MAKE1 := $(HOSTMAKE) -j1
-MAKE := $(HOSTMAKE) $(if $(PARALLEL_JOBS),-j$(PARALLEL_JOBS))
+override MAKE = $(HOSTMAKE) \
+       $(if $(findstring j,$(filter-out --%,$(MAKEFLAGS))),,-j$(PARALLEL_JOBS))
 
 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 TARGET_VENDOR = $(call qstrip,$(BR2_TOOLCHAIN_BUILDROOT_VENDOR))
--

Quoted 2 lines

2.3.4

Ping?

Signature · 1 line

Fabio Porcedda

Arnout Vandecappelle

2015-04-20 21:35 UTC #2 lore raw

PATCH 1/1 packages: fix and improve support for top-level parallel makefile

In reply to Fabio Porcedda

Quoted 36 lines · Fabio Porcedda

To fix packages that fail to build when PARALLEL_JOBS is empty instead of using an empty PARALLEL_JOBS just avoid to using it in the MAKE variable.
Check the MAKEFLAGS variable to know automatically if the -j option is being used, but use the "=" operator instead of the ":=" operator because it can be checked only in a "recursively expanded variable". Use "override" because otherwise it's impossible to change the automatic variable "MAKE".
Signed-off-by Fabio Porcedda <[email protected]>
---
 Makefile            | 4 ++--
 package/Makefile.in | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
Makefile +2 -2
index 8d09725..6c2dcca 100644
@@ -58,8 +58,8 @@ export HOSTARCH := $(shell uname -m | \
 #
 # Taking into account the above considerations, if you still want to execute
 # this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# build using the following command:
-#	make BR2_JLEVEL= -j$((`getconf _NPROCESSORS_ONLN`+1))
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 
 # absolute path
package/Makefile.in +0 -0
index fc57427..ed68e35 100644

package/Makefile.in PARALLEL_JOBS := $(BR2_JLEVEL) · line 22

Quoted 6 lines

package/Makefile.in +2 -1

@@ -18,7 +18,8 @@ PARALLEL_JOBS := $(BR2_JLEVEL)
 endif
 
 MAKE1 := $(HOSTMAKE) -j1
-MAKE := $(HOSTMAKE) $(if $(PARALLEL_JOBS),-j$(PARALLEL_JOBS))
+override MAKE = $(HOSTMAKE) \
+	$(if $(findstring j,$(filter-out --%,$(MAKEFLAGS))),,-j$(PARALLEL_JOBS))

Wouldn't it be much simpler to surround the definition of PARALLEL_JOBS at the
beginning of this file with

ifeq ($(findstring j,$(filter-out --%,$(MAKEFLAGS))),)
...
endif

Regards,
Arnout

Quoted 4 lines

package/Makefile.in +0 -0

 ifeq ($(BR2_TOOLCHAIN_BUILDROOT),y)
 TARGET_VENDOR = $(call qstrip,$(BR2_TOOLCHAIN_BUILDROOT_VENDOR))

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

Fabio Porcedda

author

2015-04-21 03:01 UTC #3 lore raw

PATCH 1/1 packages: fix and improve support for top-level parallel makefile

Quoted 47 lines · Arnout Vandecappelle

Quoted 36 lines · Fabio Porcedda
To fix packages that fail to build when PARALLEL_JOBS is empty instead of using an empty PARALLEL_JOBS just avoid to using it in the MAKE variable.
Check the MAKEFLAGS variable to know automatically if the -j option is being used, but use the "=" operator instead of the ":=" operator because it can be checked only in a "recursively expanded variable". Use "override" because otherwise it's impossible to change the automatic variable "MAKE".
Signed-off-by Fabio Porcedda <[email protected]>
---
 Makefile            | 4 ++--
 package/Makefile.in | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
Makefile +2 -2
index 8d09725..6c2dcca 100644
@@ -58,8 +58,8 @@ export HOSTARCH := $(shell uname -m | \
 #
 # Taking into account the above considerations, if you still want to execute
 # this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# build using the following command:
-#    make BR2_JLEVEL= -j$((`getconf _NPROCESSORS_ONLN`+1))
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 
 # absolute path
package/Makefile.in +0 -0
index fc57427..ed68e35 100644
package/Makefile.in PARALLEL_JOBS := $(BR2_JLEVEL) · line 22
Quoted 6 lines
package/Makefile.in +2 -1
@@ -18,7 +18,8 @@ PARALLEL_JOBS := $(BR2_JLEVEL)
 endif
 
 MAKE1 := $(HOSTMAKE) -j1
-MAKE := $(HOSTMAKE) $(if $(PARALLEL_JOBS),-j$(PARALLEL_JOBS))
+override MAKE = $(HOSTMAKE) \
+     $(if $(findstring j,$(filter-out --%,$(MAKEFLAGS))),,-j$(PARALLEL_JOBS))
Wouldn't it be much simpler to surround the definition of PARALLEL_JOBS at the
beginning of this file with

Quoted 5 lines

ifeq ($(findstring j,$(filter-out --%,$(MAKEFLAGS))),)
...
endif
?

I don't know the reason but the MAKEFLAGS variable is empty if is checked inside the "ifeq" directive, so the only way I've found to check it is using the "$(if ,,)" function like what is done for the QUIET variable.

Also some packages, like boost and jack2 use the $(PARALLEL_JOBS) variable, if that variable isn't defined they fail to build.

In the first version of this patch set [1] I've modified those packages in order to don't use the PARALLEL_JOBS if is empty, but as Thomas P. suggested:

Quoted 5 lines

In addition, even when top-level parallel build is used, using purely sequential build in each package may not be the good solution. Imagine you have a number of packages to build, and one of them is particularly
long: if you don't build this package in parallel, the build time may be increased.

[1]:
http://patchwork.ozlabs.org/patch/457112/
http://patchwork.ozlabs.org/patch/455728/

Thanks for reviewing

Signature · 1 line

Fabio Porcedda

Arnout Vandecappelle

2015-04-21 20:00 UTC #4 lore raw

PATCH 1/1 packages: fix and improve support for top-level parallel makefile

Thanks for your explanation, I see now why my alternative won't work.

Quoted 9 lines · Fabio Porcedda

Quoted 8 lines · Arnout Vandecappelle
Quoted 7 lines · Fabio Porcedda
To fix packages that fail to build when PARALLEL_JOBS is empty instead of using an empty PARALLEL_JOBS just avoid to using it in the MAKE variable.
Check the MAKEFLAGS variable to know automatically if the -j option is being used, but use the "=" operator instead of the ":=" operator because it can be checked only in a "recursively expanded variable".

I don't understand this part though. For QUIET we use :=, so why will it not
work for MAKE? Oh, of course, it's because -j does not appear in the global MAKEFLAGS :-(

Quoted 43 lines

Quoted 43 lines
Quoted 29 lines
Use "override" because otherwise it's impossible to change the automatic variable "MAKE".
Signed-off-by Fabio Porcedda <[email protected]>
---
 Makefile            | 4 ++--
 package/Makefile.in | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)
Makefile +2 -2
index 8d09725..6c2dcca 100644
@@ -58,8 +58,8 @@ export HOSTARCH := $(shell uname -m | \
 #
 # Taking into account the above considerations, if you still want to execute
 # this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# build using the following command:
-#    make BR2_JLEVEL= -j$((`getconf _NPROCESSORS_ONLN`+1))
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
 .NOTPARALLEL:
 
 # absolute path
package/Makefile.in +0 -0
index fc57427..ed68e35 100644
package/Makefile.in PARALLEL_JOBS := $(BR2_JLEVEL) · line 22
Quoted 6 lines
package/Makefile.in +2 -1
@@ -18,7 +18,8 @@ PARALLEL_JOBS := $(BR2_JLEVEL)
 endif
 
 MAKE1 := $(HOSTMAKE) -j1
-MAKE := $(HOSTMAKE) $(if $(PARALLEL_JOBS),-j$(PARALLEL_JOBS))
+override MAKE = $(HOSTMAKE) \
+     $(if $(findstring j,$(filter-out --%,$(MAKEFLAGS))),,-j$(PARALLEL_JOBS))
Wouldn't it be much simpler to surround the definition of PARALLEL_JOBS at the
beginning of this file with
ifeq ($(findstring j,$(filter-out --%,$(MAKEFLAGS))),)
...
endif

Quoted 6 lines

Quoted 1 line
?
I don't know the reason but the MAKEFLAGS variable is empty if is checked inside the "ifeq" directive, so the only way I've found to check it is using the "$(if ,,)" function like what is done for the QUIET variable.

Right! It's actually not empty, but the -j flag is removed from it. I've done a
few tests and found no way to detect the -j flag at global scope.

Quoted 2 lines

Also some packages, like boost and jack2 use the $(PARALLEL_JOBS) variable, if that variable isn't defined they fail to build.

Right, I forgot about that.

So, I tried the solution proposed in this patch extensively on a simple
Makefile to find out if everything is propagated correctly, and it seems to work well. Therefore:

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Regards,
Arnout

[snip]

Signature · 6 lines

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F