BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

PATCH New Yann E. MORIN 2 participants 6 messages Jan 21, 2014 – Jan 22, 2014 Kbuild

lore

Share & tools

lore

Yann E. MORIN

author

2014-01-21 21:04 UTC #0 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

From: "Yann E. MORIN" <[email protected]>

Using a relative path for BR2_EXTERNAL, and using an external defconfig, such as in (from a Buildroot top-dir):
make O=.. BR2_EXTERNAL=.. foo_defconfig

is broken. It is unclear why the %_defconfig rule recurses in that case, but using relative paths is inherently broken.

This patch first makes BR2_EXTERNAL canonical (ie. makes it an absolute path). Then we check this path exists and contains a valid BR2_EXTERNAL tree, by checking external.mk and Config.in exist.

The manual is updated to discourage using relative paths.

Also, a note is added to the O documentation that relative paths are discouraged as well.

Reported-by J?r?my Rosen <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Peter Korsgaard <[email protected]>

Cc Romain Naour <[email protected]>

---
 Makefile                             | 10 +++++++++-
 docs/manual/common-usage.txt         | 13 ++++++++++---
 docs/manual/customize-outside-br.txt | 12 +++++++-----
 3 files changed, 26 insertions(+), 9 deletions(-)

Makefile +9 -1

index 9dfb1e0..5753f81 100644
@@ -118,7 +118,15 @@ ifeq ($(BR2_EXTERNAL),)
   override BR2_EXTERNAL = support/dummy-external
   $(shell rm -f $(BR2_EXTERNAL_FILE))
 else
-  $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
+  _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd)
+  ifeq ($(and $(_BR2_EXTERNAL),$(wildcard $(_BR2_EXTERNAL)/external.mk),$(wildcard $(_BR2_EXTERNAL)/Config.in)),)
+    ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),)
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist)
+    else
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR))
+    endif
+  endif
+  $(shell echo BR2_EXTERNAL ?= $(_BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
 endif
 
 # Need that early, before we scan packages

docs/manual/common-usage.txt +10 -3

index 1d15c05..ca44b10 100644
@@ -40,7 +40,8 @@ Or:
  $ cd /tmp/build; make O=$PWD -C path/to/buildroot
 --------------------
 
-All the output files will be located under +/tmp/build+.
+All the output files will be located under +/tmp/build+. If the +O+
+path does not exist, Buildroot will create it.
 
 When using out-of-tree builds, the Buildroot +.config+ and temporary
 files are also stored in the output directory. This means that you can

@@ -48,13 +49,19 @@ safely run multiple builds in parallel using the same source tree as
 long as they use unique output directories.
 
 For ease of use, Buildroot generates a Makefile wrapper in the output
-directory - so after the first run, you no longer need to pass +O=..+
-and +-C ..+, simply run (in the output directory):
+directory - so after the first run, you no longer need to pass +O=<...>+
+and +-C <...>+, simply run (in the output directory):
 
 --------------------
  $ make <target>
 --------------------
 
+The +O+ path can be either an absolute or a relative path, but if it's
+passed as a relative path, it is important to note that it is interpreted
+relative to the main Buildroot source directory, not the current working
+directory. Using relative paths can lead to various broken setups, and
+thus is highly discouraged in favour of using absolute paths.
+
 [[env-vars]]
 
 Environment variables

docs/manual/customize-outside-br.txt +7 -5

index 19257e6..53d88d7 100644
@@ -32,16 +32,18 @@ removed by passing an empty value.
 
 The +BR2_EXTERNAL+ path can be either an absolute or a relative path,
 but if it's passed as a relative path, it is important to note that it
-is interpreted relatively to the main Buildroot source directory, not
-the Buildroot output directory.
+is interpreted relative to the main Buildroot source directory, not
+the Buildroot output directory. Using relative paths can lead to various
+broken setups, and thus is highly discouraged in favour of using
+absolute paths.
 
 Some examples:
 
 -----
- buildroot/ $ make BR2_EXTERNAL=../foobar menuconfig
+ buildroot/ $ make BR2_EXTERNAL=/path/to/foobar menuconfig
 -----
 
-Starting from now on, external definitions from the +../foobar+
+Starting from now on, external definitions from the +/path/to/foobar+
 directory will be used:
 
 -----

@@ -52,7 +54,7 @@ directory will be used:
 We can switch to another external definitions directory at any time:
 
 -----
- buildroot/ $ make BR2_EXTERNAL=../barfoo xconfig
+ buildroot/ $ make BR2_EXTERNAL=/where/we/have/barfoo xconfig
 -----
 
 Or disable the usage of external definitions:

Signature · 1 line

1.8.1.2

Jeremy Rosen

2014-01-22 09:00 UTC #1 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

ok, I tried your patch and I have a couple of problems

I'm still trying my use-case (I'm only half convinced it's uesfull at this point, but it's still supposed to work, which is a good enough reason to try to understand what's going on)

with the tmp/ directory containing only the buildroot subdirectory i get:

rosen at pcrosen:~/tmp/buildroot$ LANG=C make O=.. BR2_EXTERNAL=.. raspberrypi_defconfig Makefile:126: *** BR2_EXTERNAL='..' does not exist, relatively to /home/rosen/tmp/buildroot. Stop.

Which is a confusing error message, the problem is that I don't have a Config.in and external.mk file in tmp /(which are mandatory in BR2_EXTERNAL though that's not stated in the doc afaics)

once that is fixed, that command line still creates the infinite recursion on calling defconfig. So that doesn't solve that particular issue...

----- Mail original -----

Quoted 150 lines

From: "Yann E. MORIN" <[email protected]>
Using a relative path for BR2_EXTERNAL, and using an external defconfig,
such as in (from a Buildroot top-dir):
make O=.. BR2_EXTERNAL=.. foo_defconfig
is broken. It is unclear why the %_defconfig rule recurses in that case,
but using relative paths is inherently broken.
This patch first makes BR2_EXTERNAL canonical (ie. makes it an absolute
path). Then we check this path exists and contains a valid BR2_EXTERNAL
tree, by checking external.mk and Config.in exist.
The manual is updated to discourage using relative paths.
Also, a note is added to the O documentation that relative paths are discouraged as well.
Reported-by J?r?my Rosen <[email protected]>
Signed-off-by "Yann E. MORIN" <[email protected]>
Cc Peter Korsgaard <[email protected]>
Cc Romain Naour <[email protected]>
---
 Makefile                             | 10 +++++++++-
 docs/manual/common-usage.txt         | 13 ++++++++++---
 docs/manual/customize-outside-br.txt | 12 +++++++-----
 3 files changed, 26 insertions(+), 9 deletions(-)
Makefile +2 -1
index 9dfb1e0..5753f81 100644
@@ -118,7 +118,15 @@ ifeq ($(BR2_EXTERNAL),)
   override BR2_EXTERNAL = support/dummy-external
   $(shell rm -f $(BR2_EXTERNAL_FILE))
 else
-  $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) >
 $(BR2_EXTERNAL_FILE))
+  _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd)
+  ifeq ($(and $(_BR2_EXTERNAL),$(wildcard
$(_BR2_EXTERNAL)/external.mk),$(wildcard
$(_BR2_EXTERNAL)/Config.in)),)
Makefile +4 -0
+    ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),)
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist)
+    else
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist,
relatively to $(TOPDIR))
Makefile +3 -0
+    endif
+  endif
+  $(shell echo BR2_EXTERNAL ?= $(_BR2_EXTERNAL) >
$(BR2_EXTERNAL_FILE))
endif
# Need that early, before we scan packages
docs/manual/common-usage.txt +2 -1
index 1d15c05..ca44b10 100644
@@ -40,7 +40,8 @@ Or:
  $ cd /tmp/build; make O=$PWD -C path/to/buildroot
 --------------------
 
-All the output files will be located under +/tmp/build+.
+All the output files will be located under +/tmp/build+. If the +O+
+path does not exist, Buildroot will create it.
 
 When using out-of-tree builds, the Buildroot +.config+ and temporary
 files are also stored in the output directory. This means that you
can
docs/manual/common-usage.txt +0 -0
@@ -48,13 +49,19 @@ safely run multiple builds in parallel using the
same source tree as
long as they use unique output directories.
For ease of use, Buildroot generates a Makefile wrapper in the output
docs/manual/common-usage.txt +5 -2
-directory - so after the first run, you no longer need to pass
+O=..+
-and +-C ..+, simply run (in the output directory):
+directory - so after the first run, you no longer need to pass
+O=<...>+
+and +-C <...>+, simply run (in the output directory):
 
 --------------------
  $ make <target>
 --------------------
 
+The +O+ path can be either an absolute or a relative path, but if
it's
+passed as a relative path, it is important to note that it is interpreted
+relative to the main Buildroot source directory, not the current working
+directory. Using relative paths can lead to various broken setups, and
docs/manual/common-usage.txt +2 -0
+thus is highly discouraged in favour of using absolute paths.
+
 [[env-vars]]
 
 Environment variables
docs/manual/customize-outside-br.txt +0 -1
index 19257e6..53d88d7 100644
@@ -32,16 +32,18 @@ removed by passing an empty value.
 
 The +BR2_EXTERNAL+ path can be either an absolute or a relative
 path,
 but if it's passed as a relative path, it is important to note that
 it
-is interpreted relatively to the main Buildroot source directory,
not
docs/manual/customize-outside-br.txt +2 -1
-the Buildroot output directory.
+is interpreted relative to the main Buildroot source directory, not
+the Buildroot output directory. Using relative paths can lead to
various
docs/manual/customize-outside-br.txt +6 -4
+broken setups, and thus is highly discouraged in favour of using
+absolute paths.
 
 Some examples:
 
 -----
- buildroot/ $ make BR2_EXTERNAL=../foobar menuconfig
+ buildroot/ $ make BR2_EXTERNAL=/path/to/foobar menuconfig
 -----
 
-Starting from now on, external definitions from the +../foobar+
+Starting from now on, external definitions from the
+/path/to/foobar+
 directory will be used:
 
 -----
@@ -52,7 +54,7 @@ directory will be used:
 We can switch to another external definitions directory at any time:
 
 -----
- buildroot/ $ make BR2_EXTERNAL=../barfoo xconfig
+ buildroot/ $ make BR2_EXTERNAL=/where/we/have/barfoo xconfig
 -----
 
 Or disable the usage of external definitions:
--
1.8.1.2

yann.morin.1998 at free.fr

author

2014-01-22 10:21 UTC #2 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

J?r?my, All,

----- Original Message -----
From: "Jeremy Rosen" <[email protected]>
To: "Yann E. MORIN" <[email protected]>
Cc: "Peter Korsgaard" <[email protected]>, "Romain Naour" <[email protected]>, buildroot at busybox.net
Sent: Wednesday, January 22, 2014 10:00:40 AM
Subject: Re: [PATCH] BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

ok, I tried your patch and I have a couple of problems

[--SNIP--]

Quoted 7 lines

Makefile +0 -0

index 9dfb1e0..5753f81 100644
@@ -118,7 +118,15 @@ ifeq ($(BR2_EXTERNAL),)
   override BR2_EXTERNAL = support/dummy-external
   $(shell rm -f $(BR2_EXTERNAL_FILE))

Makefile ifeq ($(BR2_EXTERNAL),) · line 128

Quoted 10 lines

Makefile +8 -1

 else
-  $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
+  _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd)
+  ifeq ($(and $(_BR2_EXTERNAL),$(wildcard $(_BR2_EXTERNAL)/external.mk),$(wildcard $(_BR2_EXTERNAL)/Config.in)),)
+    ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),)
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist)
+    else
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR))
+    endif
+  endif

Doh, I forgot to commit with this line:

    override BR2_EXTERNAL := $(_BR2_EXTERNAL)

Makefile ifeq ($(BR2_EXTERNAL),) · line 130

Quoted 2 lines

Makefile +1 -0

+  $(shell echo BR2_EXTERNAL ?= $(_BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
 endif

Care to test again, please?

Regards,
Yann E. MORIN.

Jeremy Rosen

2014-01-22 12:43 UTC #3 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

Ok, thx Yann

my remark about the error message still not being clean still stands

and the doc is not explicit about Config.in and external.mk being required

if you don't want to add these to your patch I can post a separate one, but for the technical part :

Tested-by J?r?my Rosen <[email protected]>

Cordialement

J?r?my Rosen
+33 (0)1 42 68 28 04

fight key loggers : write some perl using vim

Open Wide Ingenierie

23, rue Daviel
75012 Paris - France
www.openwide.fr

----- Mail original -----

Quoted 49 lines

J?r?my, All,
----- Original Message -----
From: "Jeremy Rosen" <[email protected]>
To: "Yann E. MORIN" <[email protected]>
Cc: "Peter Korsgaard" <[email protected]>, "Romain Naour" <[email protected]>, buildroot at busybox.net
Sent: Wednesday, January 22, 2014 10:00:40 AM
Subject: Re: [PATCH] BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it
ok, I tried your patch and I have a couple of problems
[--SNIP--]
Quoted 20 lines
Makefile +2 -1
index 9dfb1e0..5753f81 100644
@@ -118,7 +118,15 @@ ifeq ($(BR2_EXTERNAL),)
   override BR2_EXTERNAL = support/dummy-external
   $(shell rm -f $(BR2_EXTERNAL_FILE))
 else
-  $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) >
 $(BR2_EXTERNAL_FILE))
+  _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 &&
 pwd)
+  ifeq ($(and $(_BR2_EXTERNAL),$(wildcard
$(_BR2_EXTERNAL)/external.mk),$(wildcard
$(_BR2_EXTERNAL)/Config.in)),)
Makefile +4 -0
+    ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),)
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist)
+    else
+      $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist,
relatively to $(TOPDIR))
Makefile ifeq ($(BR2_EXTERNAL),) · line 128
Quoted 2 lines
Makefile +2 -0
+    endif
+  endif
Doh, I forgot to commit with this line:
    override BR2_EXTERNAL := $(_BR2_EXTERNAL)
Quoted 3 lines
+ $(shell echo BR2_EXTERNAL ?= $(_BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE))
endif
Care to test again, please?
Regards,
Yann E. MORIN.

Yann E. MORIN

author

2014-01-22 21:10 UTC #4 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

J?r?my, All,

On 2014-01-22 13:43 +0100, Jeremy Rosen spake thusly:

Quoted 4 lines

my remark about the error message still not being clean still stands
and the doc is not explicit about Config.in and external.mk being required

Hopefully, my new series will address those two points.

Quoted 4 lines

if you don't want to add these to your patch I can post a separate one, but for the technical part :
Tested-by J?r?my Rosen <[email protected]>

Thank you!

I haven't included your Tested-by in the new series, though, since I've modified it more than a bit. Care to have a quick look?

Thanks for the feedback! :-)

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Yann E. MORIN

author

2014-01-22 18:55 UTC #5 lore raw

PATCH 1/1 BR2_EXTERNAL: check it is a valid BR2_EXTERNAL dir before using it

In reply to Jeremy Rosen

J?r?my, All,

On 2014-01-22 10:00 +0100, Jeremy Rosen spake thusly:

Quoted 1 line

ok, I tried your patch and I have a couple of problems

[--SNIP--]

Quoted 2 lines

rosen at pcrosen:~/tmp/buildroot$ LANG=C make O=.. BR2_EXTERNAL=.. raspberrypi_defconfig Makefile:126: *** BR2_EXTERNAL='..' does not exist, relatively to /home/rosen/tmp/buildroot. Stop.

OK, that one's fixed.

Quoted 3 lines

Which is a confusing error message, the problem is that I don't have a Config.in and external.mk file in tmp /(which are mandatory in BR2_EXTERNAL though that's not stated in the doc afaics)

Ah, right. They do be mandatory. I'll update the doc accordingly, so it is explicit one has to provide both in an br2-external tree.

Thanks for the review and testing! :-)

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'