[PATCH] build-sys: fix parallel builds w/setarch links

[PATCH] build-sys: fix parallel builds w/setarch links

[Mike Frysinger]

From: Mike Frysinger <vapier@chromium.org>

The symlink generation tries to write to the sys-utils/ subdir but does
not make sure that dir exists.  This can sometimes lead to parallel build
failures when building out-of-tree like:
...
echo ".so man8/setarch.8" > sys-utils/linux64.8
/bin/bash: sys-utils/linux64.8: No such file or directory
Makefile:11503: recipe for target 'sys-utils/linux64.8' failed
make: *** [sys-utils/linux64.8] Error 1

References: https://bugs.gentoo.org/511812
Signed-off-by: Mike Frysinger <vapier@chromium.org>
---
 sys-utils/Makemodule.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sys-utils/Makemodule.am b/sys-utils/Makemodule.am
index a1dc042..08bb6ce 100644
--- a/sys-utils/Makemodule.am
+++ b/sys-utils/Makemodule.am
@@ -152,6 +152,7 @@ man_MANS += $(SETARCH_MAN_LINKS)
 CLEANFILES += $(SETARCH_MAN_LINKS)
 
 $(SETARCH_MAN_LINKS):
+	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
 	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
 
 install-exec-hook-setarch:
-- 
2.4.0

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Pádraig Brady]

On 04/05/15 04:51, Mike Frysinger wrote:
> From: Mike Frysinger <vapier@chromium.org>
> 
> The symlink generation tries to write to the sys-utils/ subdir but does
> not make sure that dir exists.  This can sometimes lead to parallel build
> failures when building out-of-tree

>  $(SETARCH_MAN_LINKS):
> +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
>  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
>  
>  install-exec-hook-setarch:

The `test -d ... ||` bit is racy and redundant I think

cheers,
Pádraig

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Karel Zak]

On Sun, May 03, 2015 at 11:51:04PM -0400, Mike Frysinger wrote:
>  sys-utils/Makemodule.am | 1 +
>  1 file changed, 1 insertion(+)

 Applied, thanks.

 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

On 05 May 2015 11:37, Pádraig Brady wrote:
> On 04/05/15 04:51, Mike Frysinger wrote:
> > From: Mike Frysinger <vapier@chromium.org>
> > 
> > The symlink generation tries to write to the sys-utils/ subdir but does
> > not make sure that dir exists.  This can sometimes lead to parallel build
> > failures when building out-of-tree
> 
> >  $(SETARCH_MAN_LINKS):
> > +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
> >  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
> >  
> >  install-exec-hook-setarch:
> 
> The `test -d ... ||` bit is racy and redundant I think

i noticed that, but i didn't bother fixing it -- i just copied & pasted from an 
earlier fix in this project which used the same exact line

note: it's really only racy if mkdir itself fails internally, and i'm kind of 
the opinion that mkdir itself should be handling this
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Bruce Dubbs]

Mike Frysinger wrote:
> On 05 May 2015 11:37, Pádraig Brady wrote:
>> On 04/05/15 04:51, Mike Frysinger wrote:
>>> From: Mike Frysinger <vapier@chromium.org>
>>>
>>> The symlink generation tries to write to the sys-utils/ subdir but does
>>> not make sure that dir exists.  This can sometimes lead to parallel build
>>> failures when building out-of-tree
>>
>>>   $(SETARCH_MAN_LINKS):
>>> +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
>>>   	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
>>>
>>>   install-exec-hook-setarch:
>>
>> The `test -d ... ||` bit is racy and redundant I think
>
> i noticed that, but i didn't bother fixing it -- i just copied & pasted from an
> earlier fix in this project which used the same exact line
>
> note: it's really only racy if mkdir itself fails internally, and i'm kind of
> the opinion that mkdir itself should be handling this
> -mike

Why do the test at all?  Why not just a simple `mkdir -p $(dir $@)` ?

   -- Bruce

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Isaac Dunham]

On Tue, May 05, 2015 at 11:37:33AM +0100, Pádraig Brady wrote:
> On 04/05/15 04:51, Mike Frysinger wrote:
> > From: Mike Frysinger <vapier@chromium.org>
> > 
> > The symlink generation tries to write to the sys-utils/ subdir but does
> > not make sure that dir exists.  This can sometimes lead to parallel build
> > failures when building out-of-tree
> 
> >  $(SETARCH_MAN_LINKS):
> > +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
> >  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
> >  
> >  install-exec-hook-setarch:
> 
> The `test -d ... ||` bit is racy and redundant I think

Racy, yes, but no more so than "mkdir -p $DIR && install $FILE $DIR":
it's an inherent limitation of shell scripts.

Redundant, certainly: mkdir -p $DIR is required to succeed if $DIR is
a preexisting directory (though it will fail if part of the path is a
regular file).

Thanks,
Isaac Dunham

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Karel Zak]

On Sun, May 10, 2015 at 01:38:08PM -0700, Isaac Dunham wrote:
> On Tue, May 05, 2015 at 11:37:33AM +0100, Pádraig Brady wrote:
> > On 04/05/15 04:51, Mike Frysinger wrote:
> > > From: Mike Frysinger <vapier@chromium.org>
> > > 
> > > The symlink generation tries to write to the sys-utils/ subdir but does
> > > not make sure that dir exists.  This can sometimes lead to parallel build
> > > failures when building out-of-tree
> > 
> > >  $(SETARCH_MAN_LINKS):
> > > +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
> > >  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
> > >  
> > >  install-exec-hook-setarch:
> > 
> > The `test -d ... ||` bit is racy and redundant I think
> 
> Racy, yes, but no more so than "mkdir -p $DIR && install $FILE $DIR":
> it's an inherent limitation of shell scripts.
> 
> Redundant, certainly: mkdir -p $DIR is required to succeed if $DIR is
> a preexisting directory (though it will fail if part of the path is a
> regular file).

 It seems that 'test || mkdir' is nothing unusual in makefiles. The
 problem is "mkdir -p" portability (at least from autotools point of
 view :-), it seems that the proper way is to use 

    @$(MKDIR_P) $(dir $@)

 rather than directly call mkdir.


    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Peter Cordes]

On Mon, May 11, 2015 at 10:05:28AM +0200, Karel Zak wrote:
> On Sun, May 10, 2015 at 01:38:08PM -0700, Isaac Dunham wrote:
> > On Tue, May 05, 2015 at 11:37:33AM +0100, Pádraig Brady wrote:
> > > On 04/05/15 04:51, Mike Frysinger wrote:
> > > > From: Mike Frysinger <vapier@chromium.org>
> > > > 
> > > > The symlink generation tries to write to the sys-utils/ subdir but does
> > > > not make sure that dir exists.  This can sometimes lead to parallel build
> > > > failures when building out-of-tree
> > > 
> > > >  $(SETARCH_MAN_LINKS):
> > > > +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
> > > >  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
> > > >  
> > > >  install-exec-hook-setarch:
> > > 
> > > The `test -d ... ||` bit is racy and redundant I think
> > 
> > Racy, yes, but no more so than "mkdir -p $DIR && install $FILE $DIR":
> > it's an inherent limitation of shell scripts.

 You mean racy as in multiple mkdir -p invocations could happen?  A C
implementation could shorten the window for the race (because mkdir(2)
would directly follow stat(2), rather than delayed by
fork/exec/dynamic linker startup).  Or handle EEXISTS specially and
test again for existance.  You could do most of that in shell, but it
would make the code a mess.

> > Redundant, certainly: mkdir -p $DIR is required to succeed if $DIR is
> > a preexisting directory (though it will fail if part of the path is a
> > regular file).
>
>  It seems that 'test || mkdir' is nothing unusual in makefiles. The
>  problem is "mkdir -p" portability (at least from autotools point of
>  view :-), it seems that the proper way is to use 
> 
>     @$(MKDIR_P) $(dir $@)
> 
>  rather than directly call mkdir.


 The test || mkdir idiom also has a performance benefit: test is a
shell builtin in most/all shells.  stat(2) is a lot faster than
fork / exec / process startup.  (For portable build systems, this is a
huge deal on cygwin compared to anything else, because the overhead
of running a binary is huge there.)

 Anyway, if checking for existence is going to happen often, then it's
worth doing test || mkdir, rather than relying on GNU mkdir -p.

-- 
#define X(x,y) x##y
Peter Cordes ;  e-mail: X(peter@cor , des.ca)

"The gods confound the man who first found out how to distinguish the hours!
 Confound him, too, who in this place set up a sundial, to cut and hack
 my day so wretchedly into small pieces!" -- Plautus, 200 BC

Re: [PATCH] build-sys: fix parallel builds w/setarch links

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

On 16 May 2015 14:50, Peter Cordes wrote:
> On Mon, May 11, 2015 at 10:05:28AM +0200, Karel Zak wrote:
> > On Sun, May 10, 2015 at 01:38:08PM -0700, Isaac Dunham wrote:
> > > On Tue, May 05, 2015 at 11:37:33AM +0100, Pádraig Brady wrote:
> > > > On 04/05/15 04:51, Mike Frysinger wrote:
> > > > > From: Mike Frysinger <vapier@chromium.org>
> > > > > 
> > > > > The symlink generation tries to write to the sys-utils/ subdir but does
> > > > > not make sure that dir exists.  This can sometimes lead to parallel build
> > > > > failures when building out-of-tree
> > > > 
> > > > >  $(SETARCH_MAN_LINKS):
> > > > > +	$(AM_V_at) test -d $(dir $@) || mkdir -p $(dir $@)
> > > > >  	$(AM_V_GEN)echo ".so man8/setarch.8" > $@
> > > > >  
> > > > >  install-exec-hook-setarch:
> > > > 
> > > > The `test -d ... ||` bit is racy and redundant I think
> > > 
> > > Racy, yes, but no more so than "mkdir -p $DIR && install $FILE $DIR":
> > > it's an inherent limitation of shell scripts.
> 
>  You mean racy as in multiple mkdir -p invocations could happen?  A C
> implementation could shorten the window for the race (because mkdir(2)
> would directly follow stat(2), rather than delayed by
> fork/exec/dynamic linker startup).  Or handle EEXISTS specially and
> test again for existance.  You could do most of that in shell, but it
> would make the code a mess.

shortening the window isn't interesting.  it's either race-free, or racy.  the 
incident rate is irrelevant.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]