Patchwork [RFCv1,01/11] Makefile: make $(BUILD_DIR)/.root rule idempotent

login
register
mail settings
Submitter Thomas Petazzoni
Date Sept. 5, 2013, 9:27 p.m.
Message ID <1378416469-17708-2-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/272972/
State Superseded
Headers show

Comments

Thomas Petazzoni - Sept. 5, 2013, 9:27 p.m.
The $(BUILD_DIR)/.root rule is executed as part of the 'dirs'
target. The 'dirs' target is re-executed at every execution of 'make
external-deps', and make external-deps explicitly tells make to ignore
targets that have already been made (through the -B option). This
means that the $(BUILD_DIR)/.root rule has to be idempotant, which was
not the case this the introduction of the lib32/lib64 symbolic
link.

Running 'make external-deps' three times in a row was sufficient to
trigger an error due to symbolic links being incorrectly created. This
patch fixes that.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
This should probably be taken for 2013.08.1 as a fix of 'make
external-deps'.
---
 Makefile | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
Thomas De Schampheleire - Sept. 8, 2013, 12:06 p.m.
Hi Thomas,

On Thu, Sep 5, 2013 at 11:27 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> The $(BUILD_DIR)/.root rule is executed as part of the 'dirs'
> target. The 'dirs' target is re-executed at every execution of 'make
> external-deps', and make external-deps explicitly tells make to ignore
> targets that have already been made (through the -B option). This
> means that the $(BUILD_DIR)/.root rule has to be idempotant, which was
> not the case this the introduction of the lib32/lib64 symbolic
> link.
>
> Running 'make external-deps' three times in a row was sufficient to
> trigger an error due to symbolic links being incorrectly created. This
> patch fixes that.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> This should probably be taken for 2013.08.1 as a fix of 'make
> external-deps'.
> ---
>  Makefile | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 09faeba..93fc6ea 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -447,9 +447,13 @@ $(BUILD_DIR)/.root:
>                 --exclude .hg --exclude=CVS --exclude '*~' \
>                 $(TARGET_SKELETON)/ $(TARGET_DIR)/
>         cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> -       @ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
> -       @mkdir -p $(TARGET_DIR)/usr
> -       @ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
> +       $(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
> +               ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \
> +       fi
> +       $(Q)mkdir -p $(TARGET_DIR)/usr
> +       $(Q)if [ ! -L $(TARGET_DIR)/usr/$(LIB_SYMLINK) ]; then \
> +               ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK) ; \
> +       fi
>         touch $@
>
>  $(TARGET_DIR): $(BUILD_DIR)/.root
> --


Acked-by: Thomas De Schampheleire <thomas.de.schampheleire@gmail.com>


Thomas, there is a patch in patchwork that renames the
$(BUILD_DIR)/.root stamp file, by Jérôme.
Although it is not directly related to the above, could you have a look at it?
http://patchwork.ozlabs.org/patch/265680/

Thanks,
Thomas
Danomi Manchego - Sept. 8, 2013, 1:13 p.m.
Thomas,

On Thu, Sep 5, 2013 at 5:27 PM, Thomas Petazzoni <
thomas.petazzoni@free-electrons.com> wrote:

> $(TARGET_DIR_WARNING_FILE)
> -       @ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
> -       @mkdir -p $(TARGET_DIR)/usr
> -       @ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
> +       $(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
> +               ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \
> +       fi
> +       $(Q)mkdir -p $(TARGET_DIR)/usr
> +       $(Q)if [ ! -L $(TARGET_DIR)/usr/$(LIB_SYMLINK) ]; then \
> +               ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK) ; \
> +       fi
>         touch $@
>
>  $(TARGET_DIR): $(BUILD_DIR)/.root


Wouldn't just changing the "ln -s" to "ln -snf" be sufficient?  After all,
that's what is being done to make $(STAGING_DIR):

$(STAGING_DIR):
@mkdir -p $(STAGING_DIR)/bin
@mkdir -p $(STAGING_DIR)/lib
@ln -snf lib $(STAGING_DIR)/$(LIB_SYMLINK)
@mkdir -p $(STAGING_DIR)/usr/lib
@ln -snf lib $(STAGING_DIR)/usr/$(LIB_SYMLINK)
   ....

Danomi -
Thomas Petazzoni - Sept. 8, 2013, 1:30 p.m.
Dear Danomi Manchego,

On Sun, 8 Sep 2013 09:13:30 -0400, Danomi Manchego wrote:

> > $(TARGET_DIR_WARNING_FILE)
> > -       @ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
> > -       @mkdir -p $(TARGET_DIR)/usr
> > -       @ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
> > +       $(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
> > +               ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \
> > +       fi
> > +       $(Q)mkdir -p $(TARGET_DIR)/usr
> > +       $(Q)if [ ! -L $(TARGET_DIR)/usr/$(LIB_SYMLINK) ]; then \
> > +               ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK) ; \
> > +       fi
> >         touch $@
> >
> >  $(TARGET_DIR): $(BUILD_DIR)/.root
> 
> 
> Wouldn't just changing the "ln -s" to "ln -snf" be sufficient?  After
> all, that's what is being done to make $(STAGING_DIR):

I think, when I tried it, it wasn't working: at the second invocation,
it creates a symbolic link *within* the lib directory. Example:

thomas@skate:/tmp$ mkdir target
thomas@skate:/tmp$ cd target/
thomas@skate:/tmp/target$ ls
thomas@skate:/tmp/target$ mkdir lib
thomas@skate:/tmp/target$ ls lib/

# lib/ is empty

thomas@skate:/tmp/target$ ln -sf lib lib32
thomas@skate:/tmp/target$ ls -l
total 0
drwxrwxr-x 2 thomas thomas 40 sept.  8 15:27 lib
lrwxrwxrwx 1 thomas thomas  3 sept.  8 15:28 lib32 -> lib
thomas@skate:/tmp/target$ ls lib/

# we have the symbolic link, lib/ is still empty. Now we will create
# the symbolic link again

thomas@skate:/tmp/target$ ln -sf lib lib32
thomas@skate:/tmp/target$ ls -l
total 0
drwxrwxr-x 2 thomas thomas 60 sept.  8 15:28 lib
lrwxrwxrwx 1 thomas thomas  3 sept.  8 15:28 lib32 -> lib
thomas@skate:/tmp/target$ ls -l lib/
total 0
lrwxrwxrwx 1 thomas thomas 3 sept.  8 15:28 lib -> lib

# A stale symbolic link was created in lib/

Best regards,

Thomas
Danomi Manchego - Sept. 8, 2013, 4:59 p.m.
Thomas,

On Sun, Sep 8, 2013 at 9:30 AM, Thomas Petazzoni <
thomas.petazzoni@free-electrons.com> wrote:
>
> > Wouldn't just changing the "ln -s" to "ln -snf" be sufficient?  After
> > all, that's what is being done to make $(STAGING_DIR):
>
> I think, when I tried it, it wasn't working: at the second invocation,
> it creates a symbolic link *within* the lib directory. Example:
>
> thomas@skate:/tmp$ mkdir target
> thomas@skate:/tmp$ cd target/
> thomas@skate:/tmp/target$ ls
> thomas@skate:/tmp/target$ mkdir lib
> thomas@skate:/tmp/target$ ls lib/
>
> # lib/ is empty
>
> thomas@skate:/tmp/target$ ln -sf lib lib32
> thomas@skate:/tmp/target$ ls -l
> total 0
> drwxrwxr-x 2 thomas thomas 40 sept.  8 15:27 lib
> lrwxrwxrwx 1 thomas thomas  3 sept.  8 15:28 lib32 -> lib


 "ln -sf" is not sufficient for a symlink to a directory - you need "ln
-snf" - the -n treats the directory symlink as a file, which prevents this
very problem.

Example based on your example:

/tmp$ mkdir -p target/lib
/tmp$ cd target
/tmp/target$ ls lib/

# lib/ is empty

/tmp/target$ ln -snf lib lib32
/tmp/target$ ls -l
total 4
drwxrwxr-x 2 dmanchego dmanchego 4096 Sep  8 12:49 lib
lrwxrwxrwx 1 dmanchego dmanchego    3 Sep  8 12:50 lib32 -> lib
/tmp/target$ ls lib/

# we have the symbolic link, lib/ is empty

/tmp/target$ ln -snf lib lib32
/tmp/target$ ls -l
total 4
drwxrwxr-x 2 dmanchego dmanchego 4096 Sep  8 12:49 lib
lrwxrwxrwx 1 dmanchego dmanchego    3 Sep  8 12:50 lib32 -> lib
/tmp/target$ ls lib/

# still good!

/tmp/target$ ln -snf lib lib32
/tmp/target$ ls -l
total 4
drwxrwxr-x 2 dmanchego dmanchego 4096 Sep  8 12:49 lib
lrwxrwxrwx 1 dmanchego dmanchego    3 Sep  8 12:50 lib32 -> lib
/tmp/target$ ls lib/

# still good!

Using the "ln -snf" lets you drop the if statements, so it seems desirable.

Danomi -
Thomas Petazzoni - Sept. 8, 2013, 5:31 p.m.
Dear Danomi Manchego,

On Sun, 8 Sep 2013 12:59:23 -0400, Danomi Manchego wrote:

>  "ln -sf" is not sufficient for a symlink to a directory - you need "ln
> -snf" - the -n treats the directory symlink as a file, which prevents this
> very problem.

Aah, good!

> Using the "ln -snf" lets you drop the if statements, so it seems desirable.

Right, makes sense. I'll fix this up for the next iteration of the
patch series.

Thanks!

Thomas
Luca Ceresoli - Sept. 9, 2013, 8:54 a.m.
Thomas Petazzoni wrote:
> The $(BUILD_DIR)/.root rule is executed as part of the 'dirs'
> target. The 'dirs' target is re-executed at every execution of 'make
> external-deps', and make external-deps explicitly tells make to ignore
> targets that have already been made (through the -B option). This
> means that the $(BUILD_DIR)/.root rule has to be idempotant, which was
> not the case this the introduction of the lib32/lib64 symbolic

"not the case {this =>since}"?

I would also cite the original commit SHA1 for easier reference:
5628776c4a4d29d07.

Apart these minor details:
Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
[Tested with a few minimal configs]

Luca
Peter Korsgaard - Sept. 9, 2013, 9:38 p.m.
>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 Thomas> The $(BUILD_DIR)/.root rule is executed as part of the 'dirs'
 Thomas> target. The 'dirs' target is re-executed at every execution of 'make
 Thomas> external-deps', and make external-deps explicitly tells make to ignore
 Thomas> targets that have already been made (through the -B option). This
 Thomas> means that the $(BUILD_DIR)/.root rule has to be idempotant, which was
 Thomas> not the case this the introduction of the lib32/lib64 symbolic
 Thomas> link.

 Thomas> Running 'make external-deps' three times in a row was sufficient to
 Thomas> trigger an error due to symbolic links being incorrectly created. This
 Thomas> patch fixes that.

 Thomas> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
 Thomas> ---
 Thomas> This should probably be taken for 2013.08.1 as a fix of 'make
 Thomas> external-deps'.
 Thomas> ---
 Thomas>  Makefile | 10 +++++++---
 Thomas>  1 file changed, 7 insertions(+), 3 deletions(-)

 Thomas> diff --git a/Makefile b/Makefile
 Thomas> index 09faeba..93fc6ea 100644
 Thomas> --- a/Makefile
 Thomas> +++ b/Makefile
 Thomas> @@ -447,9 +447,13 @@ $(BUILD_DIR)/.root:
 Thomas>  		--exclude .hg --exclude=CVS --exclude '*~' \
 Thomas>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
 Thomas>  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
 Thomas> -	@ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
 Thomas> -	@mkdir -p $(TARGET_DIR)/usr
 Thomas> -	@ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
 Thomas> +	$(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
 Thomas> +		ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \

Can't you just do ln -sf like we do elsewhere?
Thomas Petazzoni - Sept. 10, 2013, 7:23 a.m.
Dear Peter Korsgaard,

On Mon, 09 Sep 2013 23:38:44 +0200, Peter Korsgaard wrote:

>  Thomas> diff --git a/Makefile b/Makefile
>  Thomas> index 09faeba..93fc6ea 100644
>  Thomas> --- a/Makefile
>  Thomas> +++ b/Makefile
>  Thomas> @@ -447,9 +447,13 @@ $(BUILD_DIR)/.root:
>  Thomas>  		--exclude .hg --exclude=CVS --exclude '*~' \
>  Thomas>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
>  Thomas>  	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
>  Thomas> -	@ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
>  Thomas> -	@mkdir -p $(TARGET_DIR)/usr
>  Thomas> -	@ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
>  Thomas> +	$(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
>  Thomas> +		ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \
> 
> Can't you just do ln -sf like we do elsewhere?

ln -sf no, but ln -snf yes. I'll fix up the patch and resend.

Thanks,

Thomas

Patch

diff --git a/Makefile b/Makefile
index 09faeba..93fc6ea 100644
--- a/Makefile
+++ b/Makefile
@@ -447,9 +447,13 @@  $(BUILD_DIR)/.root:
 		--exclude .hg --exclude=CVS --exclude '*~' \
 		$(TARGET_SKELETON)/ $(TARGET_DIR)/
 	cp support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
-	@ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK)
-	@mkdir -p $(TARGET_DIR)/usr
-	@ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK)
+	$(Q)if [ ! -L $(TARGET_DIR)/$(LIB_SYMLINK) ]; then \
+		ln -s lib $(TARGET_DIR)/$(LIB_SYMLINK) ; \
+	fi
+	$(Q)mkdir -p $(TARGET_DIR)/usr
+	$(Q)if [ ! -L $(TARGET_DIR)/usr/$(LIB_SYMLINK) ]; then \
+		ln -s lib $(TARGET_DIR)/usr/$(LIB_SYMLINK) ; \
+	fi
 	touch $@
 
 $(TARGET_DIR): $(BUILD_DIR)/.root