| Message ID | 44e6c31931f55e79eec713be1a202f2c170667b7.1390424303.git.yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
2014/1/22 Yann E. MORIN <yann.morin.1998@free.fr> > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > 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 makes BR2_EXTERNAL canonical (ie. makes it an absolute path), > and checks the directory exists. > > The manual is updated to discourage using relative paths. > > Reported-by: Jérémy Rosen <jeremy.rosen@openwide.fr> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Romain Naour <romain.naour@openwide.fr> > Acked-by: Samuel Martin <s.martin49@gmail.com> Regards,
Yann, all ----- Mail original ----- > From: "Yann E. MORIN" <yann.morin.1998@free.fr> > > 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 makes BR2_EXTERNAL canonical (ie. makes it an absolute > path), > and checks the directory exists. > > The manual is updated to discourage using relative paths. > > Reported-by: Jérémy Rosen <jeremy.rosen@openwide.fr> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Cc: Peter Korsgaard <jacmet@uclibc.org> > Cc: Romain Naour <romain.naour@openwide.fr> > --- > Makefile | 9 +++++++++ > docs/manual/customize-outside-br.txt | 12 +++++++----- > 2 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/Makefile b/Makefile > index 9dfb1e0..64fa138 100644 > --- a/Makefile > +++ b/Makefile > @@ -118,6 +118,15 @@ ifeq ($(BR2_EXTERNAL),) > override BR2_EXTERNAL = support/dummy-external > $(shell rm -f $(BR2_EXTERNAL_FILE)) > else > + _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) > + ifeq ($(_BR2_EXTERNAL),) > + 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 > + BR2_EXTERNAL := $(_BR2_EXTERNAL) you need an override keyword here. once this is added, the patch works correctly. > $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > > $(BR2_EXTERNAL_FILE)) > endif > > diff --git a/docs/manual/customize-outside-br.txt > b/docs/manual/customize-outside-br.txt > index 19257e6..53d88d7 100644 > --- a/docs/manual/customize-outside-br.txt > +++ b/docs/manual/customize-outside-br.txt > @@ -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: > -- > 1.8.1.2 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Dear Yann E. MORIN, On Wed, 22 Jan 2014 21:59:34 +0100, Yann E. MORIN wrote: > +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. In this patch and in patch 3/3, I don't quite agree with the strong suggestion to use absolute paths. I personally find the usage of absolute paths ugly, and prefer to have in the manual an explanation on how the paths are interpreted, so that the users can understand how to properly use either relative paths or absolute paths as they prefer. To me, a tool that strongly suggests to use absolute paths is broken. Thomas
On 26/01/14 14:53, Thomas Petazzoni wrote: > Dear Yann E. MORIN, > > On Wed, 22 Jan 2014 21:59:34 +0100, Yann E. MORIN wrote: > >> +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. > > In this patch and in patch 3/3, I don't quite agree with the strong > suggestion to use absolute paths. I personally find the usage of > absolute paths ugly, and prefer to have in the manual an explanation on > how the paths are interpreted, so that the users can understand how to > properly use either relative paths or absolute paths as they prefer. > > To me, a tool that strongly suggests to use absolute paths is broken. +1 I have never used anything except relative paths for O= (and I use O= all the time :-). Regards, Arnout
On 22/01/14 21:59, Yann E. MORIN wrote: > + _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) > + ifeq ($(_BR2_EXTERNAL),) > + ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),/) > + $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist) > + else > + $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR)) I don't think the limited explanation "relatively to $(TOPDIR)" warrants the additional complexity of patsubsting stuff. > + endif > + endif > + BR2_EXTERNAL := $(_BR2_EXTERNAL) AFAICS there is no need to have a separate _BR2_EXTERNAL, since we'll anyway error out if it is empty. So, I'd propose: override BR2_EXTERNAL := $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) ifeq ($(BR2_EXTERNAL),) $(error BR2_EXTERNAL directory '$(BR2_EXTERNAL)' does not exist) endif Regards, Arnout
On 2014-01-26 14:53 +0100, Thomas Petazzoni spake thusly: > Dear Yann E. MORIN, > > On Wed, 22 Jan 2014 21:59:34 +0100, Yann E. MORIN wrote: > > > +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. > > In this patch and in patch 3/3, I don't quite agree with the strong > suggestion to use absolute paths. I personally find the usage of > absolute paths ugly, and prefer to have in the manual an explanation on > how the paths are interpreted, so that the users can understand how to > properly use either relative paths or absolute paths as they prefer. > > To me, a tool that strongly suggests to use absolute paths is broken. I am a bit unsure how to handle this situation. On the one hand, I agree with you that not supporting relative paths would be bad. We should work seemlessly with relative paths. On the other hand, the way we handle relative paths is counter-intuitive, since they are interpreted relative to the Buildroot top-dir, not to the current directory [*], which is baiscally the way virtually all other programs handles relative paths, which makes it weird to a new-comer (or even to long-timers, I have to admit). Also, I personnally don't much care what we do or do not usggest either way. I'm just trying to avoid the easy pitfalls. [*] Note that this is not our fault, but make's, since that the way make behaves. I checked the dicumentation, and there is no way to get back the CWD make was run from. Regards, Yann E. MORIN.
On 2014-01-27 18:18 +0100, Arnout Vandecappelle spake thusly: > On 26/01/14 14:53, Thomas Petazzoni wrote: > >Dear Yann E. MORIN, > > > >On Wed, 22 Jan 2014 21:59:34 +0100, Yann E. MORIN wrote: > > > >>+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. > > > >In this patch and in patch 3/3, I don't quite agree with the strong > >suggestion to use absolute paths. I personally find the usage of > >absolute paths ugly, and prefer to have in the manual an explanation on > >how the paths are interpreted, so that the users can understand how to > >properly use either relative paths or absolute paths as they prefer. > > > >To me, a tool that strongly suggests to use absolute paths is broken. > > +1 > > I have never used anything except relative paths for O= (and I use O= all > the time :-). It does work because Buildroot create $(O). At worst, it will not end up where you hoped it to end up in, but the build will not fail because of a mis-configured $(O) (typo, not relative to $(TOPDIR)...) (which, by the way, is a fundamentaly broken behaviour, IMHO.) Regards, Yann E. MORIN.
Arnoud, All, On 2014-01-27 18:26 +0100, Arnout Vandecappelle spake thusly: > On 22/01/14 21:59, Yann E. MORIN wrote: > >+ _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) > >+ ifeq ($(_BR2_EXTERNAL),) > >+ ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),/) > >+ $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist) > >+ else > >+ $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR)) > > I don't think the limited explanation "relatively to $(TOPDIR)" warrants > the additional complexity of patsubsting stuff. Well, we want to tell the user why w edid not find his BR2_EXTERNAL. And since it can be tricky to understand that a relative path is not found, we do want to highlight that fact in the error message. > >+ endif > >+ endif > >+ BR2_EXTERNAL := $(_BR2_EXTERNAL) > > AFAICS there is no need to have a separate _BR2_EXTERNAL, since we'll > anyway error out if it is empty. I used an intermediate, since I want to display the user-supplied path in the error message, not the one we found (or did not find). > So, I'd propose: > > override BR2_EXTERNAL := $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) > ifeq ($(BR2_EXTERNAL),) > $(error BR2_EXTERNAL directory '$(BR2_EXTERNAL)' does not exist) > endif Uh? In this case $(BR2_EXTERNAL) would be empty in the error message, no? Regards, Yann E. MORIN.
On 02/02/14 11:23, Yann E. MORIN wrote: > Arnoud, All, > > On 2014-01-27 18:26 +0100, Arnout Vandecappelle spake thusly: >> On 22/01/14 21:59, Yann E. MORIN wrote: >>> + _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) >>> + ifeq ($(_BR2_EXTERNAL),) >>> + ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),/) >>> + $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist) >>> + else >>> + $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR)) >> >> I don't think the limited explanation "relatively to $(TOPDIR)" warrants >> the additional complexity of patsubsting stuff. > > Well, we want to tell the user why w edid not find his BR2_EXTERNAL. > And since it can be tricky to understand that a relative path is not > found, we do want to highlight that fact in the error message. For me it would be sufficient to make it "relative to the buildroot directory" for both the absolute and relative case. > >>> + endif >>> + endif >>> + BR2_EXTERNAL := $(_BR2_EXTERNAL) >> >> AFAICS there is no need to have a separate _BR2_EXTERNAL, since we'll >> anyway error out if it is empty. > > I used an intermediate, since I want to display the user-supplied path > in the error message, not the one we found (or did not find). > >> So, I'd propose: >> >> override BR2_EXTERNAL := $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) >> ifeq ($(BR2_EXTERNAL),) >> $(error BR2_EXTERNAL directory '$(BR2_EXTERNAL)' does not exist) >> endif > > Uh? In this case $(BR2_EXTERNAL) would be empty in the error message, no? Silly me, of course! Regards, Arnout
Arnoud, All, On 2014-02-02 12:01 +0100, Arnout Vandecappelle spake thusly: > On 02/02/14 11:23, Yann E. MORIN wrote: > >Arnoud, All, > > > >On 2014-01-27 18:26 +0100, Arnout Vandecappelle spake thusly: > >>On 22/01/14 21:59, Yann E. MORIN wrote: > >>>+ _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) > >>>+ ifeq ($(_BR2_EXTERNAL),) > >>>+ ifeq ($(patsubst /%,/,$(BR2_EXTERNAL)),/) > >>>+ $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist) > >>>+ else > >>>+ $(error BR2_EXTERNAL='$(BR2_EXTERNAL)' does not exist, relatively to $(TOPDIR)) > >> > >> I don't think the limited explanation "relatively to $(TOPDIR)" warrants > >>the additional complexity of patsubsting stuff. > > > >Well, we want to tell the user why w edid not find his BR2_EXTERNAL. > >And since it can be tricky to understand that a relative path is not > >found, we do want to highlight that fact in the error message. > > For me it would be sufficient to make it "relative to the buildroot > directory" for both the absolute and relative case. OK, I get it. OK for me. Thanks! Regards, Yann E. MORIN.
diff --git a/Makefile b/Makefile index 9dfb1e0..64fa138 100644 --- a/Makefile +++ b/Makefile @@ -118,6 +118,15 @@ ifeq ($(BR2_EXTERNAL),) override BR2_EXTERNAL = support/dummy-external $(shell rm -f $(BR2_EXTERNAL_FILE)) else + _BR2_EXTERNAL = $(shell cd $(BR2_EXTERNAL) >/dev/null 2>&1 && pwd) + ifeq ($(_BR2_EXTERNAL),) + 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 + BR2_EXTERNAL := $(_BR2_EXTERNAL) $(shell echo BR2_EXTERNAL ?= $(BR2_EXTERNAL) > $(BR2_EXTERNAL_FILE)) endif diff --git a/docs/manual/customize-outside-br.txt b/docs/manual/customize-outside-br.txt index 19257e6..53d88d7 100644 --- a/docs/manual/customize-outside-br.txt +++ b/docs/manual/customize-outside-br.txt @@ -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: