[6/7] Makefile: add check that $(HOST_DIR)/usr is not a directory

Message ID 20170709232123.30120-7-arnout@mind.be
State Rejected
Headers show

Commit Message

Arnout Vandecappelle July 9, 2017, 11:21 p.m.
We create a compatibility symlink for $(HOST_DIR)/usr. However, if
that exists already as a directory, the link can't be created. Make
will not even try since it exists already and has no dependencies. If
it exists as a directory, any post-build script that is still using
$(HOST_DIR)/usr will fail to find what it needs. Therefore, add a
check that it is not a directory.

This check has to be made as part of some PHONY target. We can reuse
the dirs target, on which all packages depend.

Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>
---
Personally I don't think it's important enough to add such a check.
If someone ends up in this situation, they'll be motivated to fix
their post-build scripts :-)
---
 Makefile | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Thomas Petazzoni July 10, 2017, 4:02 p.m. | #1
Hello,

On Mon, 10 Jul 2017 01:21:22 +0200, Arnout Vandecappelle
(Essensium/Mind) wrote:
> We create a compatibility symlink for $(HOST_DIR)/usr. However, if
> that exists already as a directory, the link can't be created. Make
> will not even try since it exists already and has no dependencies. If
> it exists as a directory, any post-build script that is still using
> $(HOST_DIR)/usr will fail to find what it needs. Therefore, add a
> check that it is not a directory.
> 
> This check has to be made as part of some PHONY target. We can reuse
> the dirs target, on which all packages depend.
> 
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> ---
> Personally I don't think it's important enough to add such a check.
> If someone ends up in this situation, they'll be motivated to fix
> their post-build scripts :-)

I also don't like very much adding such checks. In which situation can
our users fall into this problem?

They have an existing Buildroot tree, they do "git pull" and do a
partial rebuild ?

> diff --git a/Makefile b/Makefile
> index 188ce9adc7..a94555d130 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -542,6 +542,11 @@ endif
>  .PHONY: dirs
>  dirs: $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
>  	$(HOST_DIR) $(HOST_DIR)/usr $(BINARIES_DIR)
> +	@if [ -d "$(HOST_DIR)/usr" -a ! -L "$(HOST_DIR)/usr" ]; then \
> +		printf '%s is not allowed to be a directory\n' "$(HOST_DIR)/usr" 1>&2; \
> +		printf 'Please remove this directory and rebuild\n' 1>&2; \

What about saying "Please run make clean all" ?

Thomas
Yann E. MORIN July 10, 2017, 4:12 p.m. | #2
Thomas, All,

On 2017-07-10 18:02 +0200, Thomas Petazzoni spake thusly:
> On Mon, 10 Jul 2017 01:21:22 +0200, Arnout Vandecappelle
> (Essensium/Mind) wrote:
> > We create a compatibility symlink for $(HOST_DIR)/usr. However, if
> > that exists already as a directory, the link can't be created. Make
> > will not even try since it exists already and has no dependencies. If
> > it exists as a directory, any post-build script that is still using
> > $(HOST_DIR)/usr will fail to find what it needs. Therefore, add a
> > check that it is not a directory.
> > 
> > This check has to be made as part of some PHONY target. We can reuse
> > the dirs target, on which all packages depend.
> > 
> > Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> > Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> > ---
> > Personally I don't think it's important enough to add such a check.
> > If someone ends up in this situation, they'll be motivated to fix
> > their post-build scripts :-)
> 
> I also don't like very much adding such checks. In which situation can
> our users fall into this problem?

I've explained it in details when you initially reported the issue of
regenerating the toolchains:

    http://lists.busybox.net/pipermail/buildroot/2017-July/197456.html

> They have an existing Buildroot tree, they do "git pull" and do a
> partial rebuild ?

No, that occurs when a user points HOST_DIR outside, like you do for the
toolchains, and that host-dir already exists, and it already contains a
usr/ directory.

My opinion is that we should not allow using HOST_DIR if it is not empty
at all.

Regards,
Yann E. MORIN.

> > diff --git a/Makefile b/Makefile
> > index 188ce9adc7..a94555d130 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -542,6 +542,11 @@ endif
> >  .PHONY: dirs
> >  dirs: $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
> >  	$(HOST_DIR) $(HOST_DIR)/usr $(BINARIES_DIR)
> > +	@if [ -d "$(HOST_DIR)/usr" -a ! -L "$(HOST_DIR)/usr" ]; then \
> > +		printf '%s is not allowed to be a directory\n' "$(HOST_DIR)/usr" 1>&2; \
> > +		printf 'Please remove this directory and rebuild\n' 1>&2; \
> 
> What about saying "Please run make clean all" ?
> 
> Thomas
> -- 
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Arnout Vandecappelle July 10, 2017, 7:51 p.m. | #3
On 10-07-17 18:12, Yann E. MORIN wrote:
> Thomas, All,
> 
> On 2017-07-10 18:02 +0200, Thomas Petazzoni spake thusly:
[snip]
>> I also don't like very much adding such checks. In which situation can
>> our users fall into this problem?
> 
> I've explained it in details when you initially reported the issue of
> regenerating the toolchains:
> 
>     http://lists.busybox.net/pipermail/buildroot/2017-July/197456.html
> 
>> They have an existing Buildroot tree, they do "git pull" and do a
>> partial rebuild ?
> 
> No, that occurs when a user points HOST_DIR outside, like you do for the
> toolchains, and that host-dir already exists, and it already contains a
> usr/ directory.

 Actually, I realize now: when people do a git pull, we expect them to do "make
clean all" afterwards. And "make clean" will delete HOST_DIR - so no
HOST_DIR/usr will exist anymore.

 So the original report from Thomas only happened because Thomas didn't do a
"make clean" like he should have...

 Or am I missing something?


> My opinion is that we should not allow using HOST_DIR if it is not empty
> at all.

 Not sure what you mean with "using HOST_DIR". Do you mean "using a non-default
HOST_DIR"? Do you mean that we should force people to do "make clean all" *all
the time* when using a non-default HOST_DIR?

 Regards,
 Arnout
Arnout Vandecappelle July 10, 2017, 8:09 p.m. | #4
On 10-07-17 21:51, Arnout Vandecappelle wrote:
> 
> 
> On 10-07-17 18:12, Yann E. MORIN wrote:
>> Thomas, All,
>>
>> On 2017-07-10 18:02 +0200, Thomas Petazzoni spake thusly:
> [snip]
>>> I also don't like very much adding such checks. In which situation can
>>> our users fall into this problem?
>>
>> I've explained it in details when you initially reported the issue of
>> regenerating the toolchains:
>>
>>     http://lists.busybox.net/pipermail/buildroot/2017-July/197456.html
>>
>>> They have an existing Buildroot tree, they do "git pull" and do a
>>> partial rebuild ?
>>
>> No, that occurs when a user points HOST_DIR outside, like you do for the
>> toolchains, and that host-dir already exists, and it already contains a
>> usr/ directory.
> 
>  Actually, I realize now: when people do a git pull, we expect them to do "make
> clean all" afterwards. And "make clean" will delete HOST_DIR - so no
> HOST_DIR/usr will exist anymore.
> 
>  So the original report from Thomas only happened because Thomas didn't do a
> "make clean" like he should have...
> 
>  Or am I missing something?

 By the way, since I think this patch is useless, I'm not going to respin it.

 Regards,
 Arnout
Thomas Petazzoni July 10, 2017, 9:27 p.m. | #5
Hello,

On Mon, 10 Jul 2017 21:51:50 +0200, Arnout Vandecappelle wrote:

>  Actually, I realize now: when people do a git pull, we expect them to do "make
> clean all" afterwards. And "make clean" will delete HOST_DIR - so no
> HOST_DIR/usr will exist anymore.
> 
>  So the original report from Thomas only happened because Thomas didn't do a
> "make clean" like he should have...

Not correct, each time my output directory and host directories were
empty. The only thing that was different is that the host directory is
not set to the default of $(BASE_DIR)/host, but to /opt/something, and
this /opt/something is created (empty) before I start the build.

So my build was perfectly clean, it is just that I created the host
directory prior to starting the build, which IMO isn't crazy,
especially when such host directory was customized in the Buildroot
configuration.

Thomas
Arnout Vandecappelle July 10, 2017, 9:36 p.m. | #6
On 10-07-17 23:27, Thomas Petazzoni wrote:
> Hello,
> 
> On Mon, 10 Jul 2017 21:51:50 +0200, Arnout Vandecappelle wrote:
> 
>>  Actually, I realize now: when people do a git pull, we expect them to do "make
>> clean all" afterwards. And "make clean" will delete HOST_DIR - so no
>> HOST_DIR/usr will exist anymore.
>>
>>  So the original report from Thomas only happened because Thomas didn't do a
>> "make clean" like he should have...
> 
> Not correct, each time my output directory and host directories were
> empty. The only thing that was different is that the host directory is
> not set to the default of $(BASE_DIR)/host, but to /opt/something, and
> this /opt/something is created (empty) before I start the build.

 The part about a non-standard HOST_DIR is irrelevant, it's doing

make clean
mkdir -p output/host
make

that triggered the issue. Which obviously makes no sense at all in the normal
situation, but does make some kind of twisted sense in the non-standard HOST_DIR
situation.

> So my build was perfectly clean, it is just that I created the host
> directory prior to starting the build, which IMO isn't crazy,
> especially when such host directory was customized in the Buildroot
> configuration.

 Well, it doesn't make *that* much sense to me, because pre-populating that
directory with stuff is kind of fragile, given that it will all be removed again
by "make clean".

 Anyway, I still don't think that this patch is very useful :-)


 Regards,
 Arnout
Yann E. MORIN July 10, 2017, 9:43 p.m. | #7
Arnout, all,

On 2017-07-10 23:36 +0200, Arnout Vandecappelle spake thusly:
> On 10-07-17 23:27, Thomas Petazzoni wrote:
> > Hello,
> > 
> > On Mon, 10 Jul 2017 21:51:50 +0200, Arnout Vandecappelle wrote:
> > 
> >>  Actually, I realize now: when people do a git pull, we expect them to do "make
> >> clean all" afterwards. And "make clean" will delete HOST_DIR - so no
> >> HOST_DIR/usr will exist anymore.
> >>
> >>  So the original report from Thomas only happened because Thomas didn't do a
> >> "make clean" like he should have...
> > 
> > Not correct, each time my output directory and host directories were
> > empty. The only thing that was different is that the host directory is
> > not set to the default of $(BASE_DIR)/host, but to /opt/something, and
> > this /opt/something is created (empty) before I start the build.
> 
>  The part about a non-standard HOST_DIR is irrelevant, it's doing
> 
> make clean
> mkdir -p output/host
> make
> 
> that triggered the issue. Which obviously makes no sense at all in the normal
> situation, but does make some kind of twisted sense in the non-standard HOST_DIR
> situation.
> 
> > So my build was perfectly clean, it is just that I created the host
> > directory prior to starting the build, which IMO isn't crazy,
> > especially when such host directory was customized in the Buildroot
> > configuration.
> 
>  Well, it doesn't make *that* much sense to me, because pre-populating that
> directory with stuff is kind of fragile, given that it will all be removed again
> by "make clean".

It is not about running "make clean" but having stuff pre-isntalled in
there, from a clean build.

I.e. people would do:
  mkdir /my/host-dir
  tar xf mystuff.tar -C /my/host-dir
  make -C buildroot my_toolchain_defconfig
  make -C buildroot toolchain

and then their stuff is poulting the host-dir, which we should try to
avoid.

Granted, there is nothing that would prevent them from extracting it
after the build...

>  Anyway, I still don't think that this patch is very useful :-)

Until we have weird build errors that are reported, because some weirdo
will have already extracted a few toolchains in there. Or tries to
re-use the same host dir to isntall many toolchains in there... ;-)

(Note: I take it that Thomas uses different host dirs for each of the
toolchains, ot that the host dir is empty at the start of each toolchain
build.)

Regards,
Yann E. MORIN.

Patch

diff --git a/Makefile b/Makefile
index 188ce9adc7..a94555d130 100644
--- a/Makefile
+++ b/Makefile
@@ -542,6 +542,11 @@  endif
 .PHONY: dirs
 dirs: $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
 	$(HOST_DIR) $(HOST_DIR)/usr $(BINARIES_DIR)
+	@if [ -d "$(HOST_DIR)/usr" -a ! -L "$(HOST_DIR)/usr" ]; then \
+		printf '%s is not allowed to be a directory\n' "$(HOST_DIR)/usr" 1>&2; \
+		printf 'Please remove this directory and rebuild\n' 1>&2; \
+		exit 1; \
+	fi
 
 $(BUILD_DIR)/buildroot-config/auto.conf: $(BR2_CONFIG)
 	$(MAKE1) $(EXTRAMAKEARGS) HOSTCC="$(HOSTCC_NOCCACHE)" HOSTCXX="$(HOSTCXX_NOCCACHE)" silentoldconfig