Patchwork Standardisation of $(BUILD)/.root name

login
register
mail settings
Submitter Jérôme Pouiller
Date May 14, 2013, 2:30 p.m.
Message ID <1368541851-31089-1-git-send-email-jezz@sysmic.org>
Download mbox | patch
Permalink /patch/243725/
State Superseded
Headers show

Comments

Jérôme Pouiller - May 14, 2013, 2:30 p.m.
To be coherent with stamps used by toolchains, file $(BUILD)/.root is now named
$(STAMPS)/target-skeleton-installed.

Signed-off-by: Jérôme Pouiller <jezz@sysmic.org>
---
 Makefile |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Thomas Petazzoni - May 14, 2013, 3:07 p.m.
Dear Jérôme Pouiller,

On Tue, 14 May 2013 16:30:50 +0200, Jérôme Pouiller wrote:

> -$(BUILD_DIR)/.root:
> +$(STAMP_DIR)/skeleton-target-installed:
>  	mkdir -p $(TARGET_DIR)
>  	rsync -a \
>  		--exclude .empty --exclude .svn --exclude .git \
>  		--exclude .hg --exclude=CVS --exclude '*~' \
>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
>  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> +	mkdir -p $(STAMP_DIR)
>  	touch $@

I'm generally ok with the idea, but I don't like this mkdir. There is
already a mkdir that creates the stamp file directory, in the main
Makefile.

Thomas
Jérôme Pouiller - May 14, 2013, 3:27 p.m.
On Tuesday 14 May 2013 17:07:21 Thomas Petazzoni wrote:
> Dear Jérôme Pouiller,
> 
> On Tue, 14 May 2013 16:30:50 +0200, Jérôme Pouiller wrote:
> > -$(BUILD_DIR)/.root:
> > 
> > +$(STAMP_DIR)/skeleton-target-installed:
> >  	mkdir -p $(TARGET_DIR)
> >  	rsync -a \
> >  	
> >  		--exclude .empty --exclude .svn --exclude .git \
> >  		--exclude .hg --exclude=CVS --exclude '*~' \
> >  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
> >  	
> >  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> > 
> > +	mkdir -p $(STAMP_DIR)
> > 
> >  	touch $@
> 
> I'm generally ok with the idea, but I don't like this mkdir. There is
> already a mkdir that creates the stamp file directory, in the main
> Makefile.
I also dislike this mkdir. My problem is to be sure $(STAMP_DIR) is created 
before $(STAMP_DIR)/skeleton-target-installed. 

I can change order of dependencies in dirs target but this solution is too 
fragile.

I cannot just add a dependency between $(STAMP_DIR)/skeleton-target-installed 
and  $(STAMP_DIR).

I can add a stampfile for $(STAMP_DIR) but it looks a little complicated. 

I can also use an intermediate phony target (like dirs target) but it also 
looks complicated.


Any better idea?
Arnout Vandecappelle - May 14, 2013, 9:52 p.m.
On 14/05/13 17:07, Thomas Petazzoni wrote:
> Dear Jérôme Pouiller,
>
> On Tue, 14 May 2013 16:30:50 +0200, Jérôme Pouiller wrote:
>
>> -$(BUILD_DIR)/.root:
>> +$(STAMP_DIR)/skeleton-target-installed:
>>   	mkdir -p $(TARGET_DIR)
>>   	rsync -a \
>>   		--exclude .empty --exclude .svn --exclude .git \
>>   		--exclude .hg --exclude=CVS --exclude '*~' \
>>   		$(TARGET_SKELETON)/ $(TARGET_DIR)/
>>   	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
>> +	mkdir -p $(STAMP_DIR)
>>   	touch $@
>
> I'm generally ok with the idea, but I don't like this mkdir. There is
> already a mkdir that creates the stamp file directory, in the main
> Makefile.

  I don't have an issue with a few redundant mkdir -p calls.

  I would like to see a $(Q) in front of it though - but then it should 
be in front of the rest as well, and with a $(MESSAGE) call.

  Regards,
  Arnout
Yann E. MORIN - May 14, 2013, 10:06 p.m.
Arnout, All,

On 2013-05-14 23:52 +0200, Arnout Vandecappelle spake thusly:
> On 14/05/13 17:07, Thomas Petazzoni wrote:
> >Dear Jérôme Pouiller,
> >
> >On Tue, 14 May 2013 16:30:50 +0200, Jérôme Pouiller wrote:
> >
> >>-$(BUILD_DIR)/.root:
> >>+$(STAMP_DIR)/skeleton-target-installed:
> >>  	mkdir -p $(TARGET_DIR)
> >>  	rsync -a \
> >>  		--exclude .empty --exclude .svn --exclude .git \
> >>  		--exclude .hg --exclude=CVS --exclude '*~' \
> >>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
> >>  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> >>+	mkdir -p $(STAMP_DIR)
> >>  	touch $@
> >
> >I'm generally ok with the idea, but I don't like this mkdir. There is
> >already a mkdir that creates the stamp file directory, in the main
> >Makefile.
> 
>  I don't have an issue with a few redundant mkdir -p calls.

I concur with Thomas: we should ensure the stamp-dir is already created
in a generic way, it's kind of an internal stuff, which should not be
visible to users (I mean: packages, and stuff like that, not actual
persons).

That we need to sprinkle the code with a few 'mkdir' here and ther "just
in case" is an indication we do not fully masterise the chain of events,
and is really not nice.

The fact that the 'mkdir' is needed in the first place is because of this
rulle in the top-level Makefile:

    dirs: [...] $(TARGET_DIR) [...] $(STAMP_DIR)

which will have 'make' run the $(TARGET_DIR) goal before the
$(STAMP_DIR) goal.

This can be solved in two ways: the nice one, and the not-so-nice one:
  - have an additional rule:
        $(TARGET_DIR): $(STAMP_DIR)                 <- the nice way
  - invert the order of goals in the 'dirs' rule    <- the not-so-nice way

Regards,
Yann E. MORIN.
Yann E. MORIN - May 14, 2013, 10:23 p.m.
Jérôme, All,

On 2013-05-14 17:27 +0200, Jérôme Pouiller spake thusly:
> On Tuesday 14 May 2013 17:07:21 Thomas Petazzoni wrote:
> > Dear Jérôme Pouiller,
> > 
> > On Tue, 14 May 2013 16:30:50 +0200, Jérôme Pouiller wrote:
> > > -$(BUILD_DIR)/.root:
> > > 
> > > +$(STAMP_DIR)/skeleton-target-installed:
> > >  	mkdir -p $(TARGET_DIR)
> > >  	rsync -a \
> > >  	
> > >  		--exclude .empty --exclude .svn --exclude .git \
> > >  		--exclude .hg --exclude=CVS --exclude '*~' \
> > >  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
> > >  	
> > >  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> > > 
> > > +	mkdir -p $(STAMP_DIR)
> > > 
> > >  	touch $@
> > 
> > I'm generally ok with the idea, but I don't like this mkdir. There is
> > already a mkdir that creates the stamp file directory, in the main
> > Makefile.
> I also dislike this mkdir. My problem is to be sure $(STAMP_DIR) is created 
> before $(STAMP_DIR)/skeleton-target-installed. 
> 
> I can change order of dependencies in dirs target but this solution is too 
> fragile.

Agreed, this is not-so-nice.

> I cannot just add a dependency between $(STAMP_DIR)/skeleton-target-installed 
> and  $(STAMP_DIR).

Why not? Like:
    $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)

which to me looks like the sane solution (and not what I previosuly
answered Arnout about $(TARGET_DIR))

Regards,
Yann E. MORIN.
Jérôme Pouiller - May 15, 2013, 6:19 a.m.
On Wednesday 15 May 2013 00:23:24 Yann E. MORIN wrote:
[...]
> > I cannot just add a dependency between
> > $(STAMP_DIR)/skeleton-target-installed and  $(STAMP_DIR).
> 
> Why not? Like:
>     $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)
> 
> which to me looks like the sane solution (and not what I previosuly
> answered Arnout about $(TARGET_DIR))
Because, $(STAMP_DIR)/skeleton-target-installed will be rebuilt each time a 
file is added or removed from $(STAMP_DIR).
Jérôme Pouiller - May 15, 2013, 6:27 a.m.
On Wednesday 15 May 2013 08:19:10 Jérôme Pouiller wrote:
> On Wednesday 15 May 2013 00:23:24 Yann E. MORIN wrote:
> [...]
> 
> > > I cannot just add a dependency between
> > > $(STAMP_DIR)/skeleton-target-installed and  $(STAMP_DIR).
> > 
> > Why not? Like:
> >     $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)
> > 
> > which to me looks like the sane solution (and not what I previosuly
> > answered Arnout about $(TARGET_DIR))
> 
> Because, $(STAMP_DIR)/skeleton-target-installed will be rebuilt each time a
> file is added or removed from $(STAMP_DIR).
But, we may use an intermediate phony target:
$(STAMP_DIR)_phony: $(STAMP_DIR)
.PHONY: $(STAMP_DIR)_phony
$(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)_phony

(not tested, but should work)
Yann E. MORIN - May 15, 2013, 8:59 p.m.
On 2013-05-15 08:19 +0200, Jérôme Pouiller spake thusly:
> On Wednesday 15 May 2013 00:23:24 Yann E. MORIN wrote:
> [...]
> > > I cannot just add a dependency between
> > > $(STAMP_DIR)/skeleton-target-installed and  $(STAMP_DIR).
> > 
> > Why not? Like:
> >     $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)
> > 
> > which to me looks like the sane solution (and not what I previosuly
> > answered Arnout about $(TARGET_DIR))
> Because, $(STAMP_DIR)/skeleton-target-installed will be rebuilt each time a 
> file is added or removed from $(STAMP_DIR).

Indeed. Sigh. Too short-sighted I was...

Regards,
Yann E. MORIN.
Yann E. MORIN - May 15, 2013, 9:01 p.m.
Jérôme, All,

On 2013-05-15 08:27 +0200, Jérôme Pouiller spake thusly:
> On Wednesday 15 May 2013 08:19:10 Jérôme Pouiller wrote:
> > On Wednesday 15 May 2013 00:23:24 Yann E. MORIN wrote:
> > [...]
> > 
> > > > I cannot just add a dependency between
> > > > $(STAMP_DIR)/skeleton-target-installed and  $(STAMP_DIR).
> > > 
> > > Why not? Like:
> > >     $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)
> > > 
> > > which to me looks like the sane solution (and not what I previosuly
> > > answered Arnout about $(TARGET_DIR))
> > 
> > Because, $(STAMP_DIR)/skeleton-target-installed will be rebuilt each time a
> > file is added or removed from $(STAMP_DIR).
> But, we may use an intermediate phony target:
> $(STAMP_DIR)_phony: $(STAMP_DIR)
> .PHONY: $(STAMP_DIR)_phony
> $(STAMP_DIR)/skeleton-target-installed: $(STAMP_DIR)_phony

Or an order-only rule:

$(STAMP_DIR)/skeleton-target-installed: | $(STAMP_DIR)

(not tested, but it's used in the crosstool-NG backend)

Regards,
Yann E. MORIN.

Patch

diff --git a/Makefile b/Makefile
index a57975e..73a7c01 100644
--- a/Makefile
+++ b/Makefile
@@ -429,16 +429,17 @@  ifeq ($(BR2_ROOTFS_SKELETON_CUSTOM),y)
 TARGET_SKELETON=$(BR2_ROOTFS_SKELETON_CUSTOM_PATH)
 endif
 
-$(BUILD_DIR)/.root:
+$(STAMP_DIR)/skeleton-target-installed:
 	mkdir -p $(TARGET_DIR)
 	rsync -a \
 		--exclude .empty --exclude .svn --exclude .git \
 		--exclude .hg --exclude=CVS --exclude '*~' \
 		$(TARGET_SKELETON)/ $(TARGET_DIR)/
 	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
+	mkdir -p $(STAMP_DIR)
 	touch $@
 
-$(TARGET_DIR): $(BUILD_DIR)/.root
+$(TARGET_DIR): $(STAMP_DIR)/skeleton-target-installed
 
 STRIP_FIND_CMD = find $(TARGET_DIR)
 ifneq (,$(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS)))