diff mbox series

[next,v4,4/6] Makefile: move .NOTPARALLEL statement after including .config file

Message ID 20181114105557.12599-5-thomas.petazzoni@bootlin.com
State Superseded
Headers show
Series Per-package host/target directory support | expand

Commit Message

Thomas Petazzoni Nov. 14, 2018, 10:55 a.m. UTC
In a follow-up commit, we will make the .NOTPARALLEL statement
conditional on a Config.in option, so we need to move it further down.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 Makefile | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Yann E. MORIN Nov. 15, 2018, 9:37 p.m. UTC | #1
Thomas, All,

On 2018-11-14 11:55 +0100, Thomas Petazzoni spake thusly:
> In a follow-up commit, we will make the .NOTPARALLEL statement
> conditional on a Config.in option, so we need to move it further down.

Ultimately, I don't see why we do need a config option to turn top-level
parallel build on/off. It will ultimately be the user's choice to do so
when calling 'make -jN' no?

However, what we do need for now, is a config option that enabled
per-package directories (PPD, formerly known as PPS, per-package
staging). When that is turned off, we must not allow top-level
parallel build.

I was thinking that we would not need that option either, because we
would enable PPD when we detect that the user is doing top-level parllel
build, by inspecting $(MAKEFLAGS) to see if they contained -j.

However, make is our fiend here, because it does not include -j in the
MAKFLAGS variable, but the one that is exported:

    $ cat Makefile
    $(info var MAKEFLAGS='$(MAKEFLAGS)')
    all:
        @echo "env MAKEFLAGS='$${MAKEFLAGS}'"

    $ make -j1
    var MAKEFLAGS=''
    env MAKEFLAGS=''

    $ make -j3
    var MAKEFLAGS=''
    env MAKEFLAGS=' -j --jobserver-fds=3,4'

    $ make -j
    var MAKEFLAGS=''
    env MAKEFLAGS=' -j'

And there is no other variable in make that specifies that either...

So yes, this means we have no way from the Makefile to detect whether
we're parallel or not. Sigh. :-(

So we do need that option for PPD.

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  Makefile | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e675ac26aa..24c803872d 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,22 +105,6 @@ ifneq ($(firstword $(sort $(RUNNING_MAKE_VERSION) $(MIN_MAKE_VERSION))),$(MIN_MA
>  $(error You have make '$(RUNNING_MAKE_VERSION)' installed. GNU make >= $(MIN_MAKE_VERSION) is required)
>  endif
>  
> -# Parallel execution of this Makefile is disabled because it changes
> -# the packages building order, that can be a problem for two reasons:
> -# - If a package has an unspecified optional dependency and that
> -#   dependency is present when the package is built, it is used,
> -#   otherwise it isn't (but compilation happily proceeds) so the end
> -#   result will differ if the order is swapped due to parallel
> -#   building.
> -# - Also changing the building order can be a problem if two packages
> -#   manipulate the same file in the target directory.
> -#
> -# Taking into account the above considerations, if you still want to execute
> -# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
> -# use the -j<jobs> option when building, e.g:
> -#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
> -.NOTPARALLEL:
> -
>  # absolute path
>  TOPDIR := $(CURDIR)
>  CONFIG_CONFIG_IN = Config.in
> @@ -246,6 +230,22 @@ ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
>  -include $(BR2_CONFIG)
>  endif
>  
> +# Parallel execution of this Makefile is disabled because it changes
> +# the packages building order, that can be a problem for two reasons:
> +# - If a package has an unspecified optional dependency and that
> +#   dependency is present when the package is built, it is used,
> +#   otherwise it isn't (but compilation happily proceeds) so the end
> +#   result will differ if the order is swapped due to parallel
> +#   building.
> +# - Also changing the building order can be a problem if two packages
> +#   manipulate the same file in the target directory.
> +#
> +# Taking into account the above considerations, if you still want to execute
> +# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
> +# use the -j<jobs> option when building, e.g:
> +#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
> +.NOTPARALLEL:
> +
>  # timezone and locale may affect build output
>  ifeq ($(BR2_REPRODUCIBLE),y)
>  export TZ = UTC
> -- 
> 2.19.1
>
Thomas Petazzoni Nov. 16, 2018, 8:53 a.m. UTC | #2
Hello,

On Thu, 15 Nov 2018 22:37:58 +0100, Yann E. MORIN wrote:

> Ultimately, I don't see why we do need a config option to turn top-level
> parallel build on/off. It will ultimately be the user's choice to do so
> when calling 'make -jN' no?
> 
> However, what we do need for now, is a config option that enabled
> per-package directories (PPD, formerly known as PPS, per-package
> staging). When that is turned off, we must not allow top-level
> parallel build.
> 
> I was thinking that we would not need that option either, because we
> would enable PPD when we detect that the user is doing top-level parllel
> build, by inspecting $(MAKEFLAGS) to see if they contained -j.
> 
> However, make is our fiend here, because it does not include -j in the
> MAKFLAGS variable, but the one that is exported:
> 
>     $ cat Makefile
>     $(info var MAKEFLAGS='$(MAKEFLAGS)')
>     all:
>         @echo "env MAKEFLAGS='$${MAKEFLAGS}'"
> 
>     $ make -j1
>     var MAKEFLAGS=''
>     env MAKEFLAGS=''
> 
>     $ make -j3
>     var MAKEFLAGS=''
>     env MAKEFLAGS=' -j --jobserver-fds=3,4'
> 
>     $ make -j
>     var MAKEFLAGS=''
>     env MAKEFLAGS=' -j'
> 
> And there is no other variable in make that specifies that either...
> 
> So yes, this means we have no way from the Makefile to detect whether
> we're parallel or not. Sigh. :-(
> 
> So we do need that option for PPD.
> 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>  
> 
> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Thanks for this research and the review. I think there is another
reason to have an option, at least for the moment: I'm pretty sure a
number of people not aware of top-level parallel build issues use
Buildroot with "make -jN" today, as they have learned that using make
-jN will "speed up their build" (I mean in general, not specifically
for Buildroot).

If we were to use make -jN as the criteria to decide if PPD should be
used or not, then suddenly all such users would start using PPD, which
is still way too experimental to expose it without the user's explicit
consent.

My plan is rather the following:

 (1) Have it under an explicit option for now.

 (2) Once we consider it stable, drop the option entirely, and always
     use PPD, even if the user doesn't use top-level parallel build.
     There's a slight additional cost, but there's also a huge benefit
     in terms of dependency checking/isolation.

Note that we might stay on step (1) for a little while, I think it will
take time to really make it fully usable.

Best regards,

Thomas
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index e675ac26aa..24c803872d 100644
--- a/Makefile
+++ b/Makefile
@@ -105,22 +105,6 @@  ifneq ($(firstword $(sort $(RUNNING_MAKE_VERSION) $(MIN_MAKE_VERSION))),$(MIN_MA
 $(error You have make '$(RUNNING_MAKE_VERSION)' installed. GNU make >= $(MIN_MAKE_VERSION) is required)
 endif
 
-# Parallel execution of this Makefile is disabled because it changes
-# the packages building order, that can be a problem for two reasons:
-# - If a package has an unspecified optional dependency and that
-#   dependency is present when the package is built, it is used,
-#   otherwise it isn't (but compilation happily proceeds) so the end
-#   result will differ if the order is swapped due to parallel
-#   building.
-# - Also changing the building order can be a problem if two packages
-#   manipulate the same file in the target directory.
-#
-# Taking into account the above considerations, if you still want to execute
-# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
-# use the -j<jobs> option when building, e.g:
-#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
-.NOTPARALLEL:
-
 # absolute path
 TOPDIR := $(CURDIR)
 CONFIG_CONFIG_IN = Config.in
@@ -246,6 +230,22 @@  ifeq ($(filter $(noconfig_targets),$(MAKECMDGOALS)),)
 -include $(BR2_CONFIG)
 endif
 
+# Parallel execution of this Makefile is disabled because it changes
+# the packages building order, that can be a problem for two reasons:
+# - If a package has an unspecified optional dependency and that
+#   dependency is present when the package is built, it is used,
+#   otherwise it isn't (but compilation happily proceeds) so the end
+#   result will differ if the order is swapped due to parallel
+#   building.
+# - Also changing the building order can be a problem if two packages
+#   manipulate the same file in the target directory.
+#
+# Taking into account the above considerations, if you still want to execute
+# this top-level Makefile in parallel comment the ".NOTPARALLEL" line and
+# use the -j<jobs> option when building, e.g:
+#      make -j$((`getconf _NPROCESSORS_ONLN`+1))
+.NOTPARALLEL:
+
 # timezone and locale may affect build output
 ifeq ($(BR2_REPRODUCIBLE),y)
 export TZ = UTC