diff mbox

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

Message ID 1378416469-17708-2-git-send-email-thomas.petazzoni@free-electrons.com
State Superseded
Headers show

Commit Message

Thomas Petazzoni Sept. 5, 2013, 9:27 p.m. UTC 
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(-)

Comments

Thomas De Schampheleire Sept. 8, 2013, 12:06 p.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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. UTC | #6 
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. UTC | #7 
>>>>> "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. UTC | #8 
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
diff mbox

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