1082910 - Race condition for sdk/bootstrap.js

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

make[7]: Leaving directory `/builds/slave/rel-m-beta-l64_bld-00000000000/build/obj-firefox/addon-sdk'
make[6]: Leaving directory `/builds/slave/rel-m-beta-l64_bld-00000000000/build/obj-firefox/addon-sdk'
cannot make symbolic link /builds/slave/rel-m-beta-l64_bld-00000000000/build/obj-firefox/dist/bin/modules/sdk/bootstrap.js: File exists
make[7]: *** [../dist/bin/modules/sdk/bootstrap.js] Error 1
make[7]: *** Deleting file `../dist/bin/modules/sdk/bootstrap.js'
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [libs] Error 2
make[5]: *** [addon-sdk/libs] Error 2

Full log at http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/34.0b1-candidates/build2/logs/release-mozilla-beta-linux64_build-bm82-build1-build10.txt.gz

But:
[[e-mail] ~]$ ls -l /builds/slave/rel-m-beta-l64_bld-00000000000/build/obj-firefox/dist/bin/modules/sdk/bootstrap.js
ls: cannot access /builds/slave/rel-m-beta-l64_bld-00000000000/build/obj-firefox/dist/bin/modules/sdk/bootstrap.js: No such file or directory

The parent directory exists and is empty.

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

That was while building 34.0b1 on http://hg.mozilla.org/releases/mozilla-beta/rev/7a12de89326b

Comment 2

[Nick Thomas [:nthomas] (UTC+12)]

And again in http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/34.0b1-candidates/build2/logs/release-mozilla-beta-linux_build-bm86-build1-build12.txt.gz

Changes since we didn't hit this in 34.0b1 build1:
 http://hg.mozilla.org/releases/mozilla-beta/rev/35b93d3f5d65
 http://hg.mozilla.org/releases/mozilla-beta/rev/9e9e53d15011
 http://hg.mozilla.org/releases/mozilla-beta/rev/00cf6dd17a10

I don't see anything relevant there.

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

[Tracking Requested - why for this release]: Causing builds to fail in buildbot, so far one dep build and two release builds.

Comment 4

[Dave Townsend [:mossop]]

The code that copies that file is new to 34 from bug 1044162, maybe a bug in it? A little worried by that bug having an unreviewed "fixup patch" in it. I defer to build peers.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Dave Townsend [:mossop] from comment #4)
> The code that copies that file is new to 34 from bug 1044162, maybe a bug in
> it? A little worried by that bug having an unreviewed "fixup patch" in it. I
> defer to build peers.

I would have expected a bug from that to show up long before now.

I see six reviewed patches in bug 1044162; are you looking at a different bug?

Comment 6

[Mike Hommey [:glandium]]

The bug is that the Makefile.in does a make -f copy_source.mk libs, and that triggers the rules for what was added in bug 1044162. So there's a race between that happening and the current invocation of the makefile triggering that rule as well.

Something like the following should fix it (patch edited by hand because I don't have a beta tree ready to use and that code is gone on trunk):

--- a/addon-sdk/Makefile.in
+++ b/addon-sdk/Makefile.in
@@ -7,17 +7,17 @@
 ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
 MOZ_B2G=1
 else
 MOZ_B2G=0
 endif
 
 libs::
 	$(PYTHON) $(srcdir)/copy_source.py $(topsrcdir) $(srcdir)/source/lib $(FINAL_TARGET)/modules/commonjs $(MOZ_B2G) >copy_source.mk
-	$(MAKE) -f copy_source.mk libs
+	$(MAKE) -f copy_source.mk copy_source
 
 include $(topsrcdir)/config/rules.mk
 
 TEST_FILES = \
   source/app-extension \
   source/bin \
   source/python-lib \
   source/test \
--- a/addon-sdk/copy_source.py
+++ b/addon-sdk/copy_source.py
@@ -53,11 +53,12 @@
         ]:
         continue
     varname = "COMMONJS%s" % relative.replace('/', '_')
     print "%s_FILES = \\" % varname
     for name in filenames:
         print "  %s/%s \\" % (dirpath, name)
     print "  $(NULL)"
     print "%s_DEST = %s%s" % (varname, target_dir, relative)
+    print "%s_TARGET = copy_source" % varname
     print "INSTALL_TARGETS += %s\n" % varname
 
 print "include $(topsrcdir)/config/rules.mk"

Comment 7

[Dave Townsend [:mossop]]

(In reply to Nathan Froyd (:froydnj) from comment #5)
> (In reply to Dave Townsend [:mossop] from comment #4)
> > The code that copies that file is new to 34 from bug 1044162, maybe a bug in
> > it? A little worried by that bug having an unreviewed "fixup patch" in it. I
> > defer to build peers.
> 
> I would have expected a bug from that to show up long before now.
> 
> I see six reviewed patches in bug 1044162; are you looking at a different
> bug?

Sorry yes reviewed, just no indication it landed.

Comment 8

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #6)
> The bug is that the Makefile.in does a make -f copy_source.mk libs, and that
> triggers the rules for what was added in bug 1044162. So there's a race
> between that happening and the current invocation of the makefile triggering
> that rule as well.
> 
> Something like the following should fix it (patch edited by hand because I
> don't have a beta tree ready to use and that code is gone on trunk):
> 
> --- a/addon-sdk/Makefile.in
> +++ b/addon-sdk/Makefile.in
> @@ -7,17 +7,17 @@
>  ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
>  MOZ_B2G=1
>  else
>  MOZ_B2G=0
>  endif
>  
>  libs::
>  	$(PYTHON) $(srcdir)/copy_source.py $(topsrcdir) $(srcdir)/source/lib
> $(FINAL_TARGET)/modules/commonjs $(MOZ_B2G) >copy_source.mk
> -	$(MAKE) -f copy_source.mk libs
> +	$(MAKE) -f copy_source.mk copy_source
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  TEST_FILES = \
>    source/app-extension \
>    source/bin \
>    source/python-lib \
>    source/test \
> --- a/addon-sdk/copy_source.py
> +++ b/addon-sdk/copy_source.py
> @@ -53,11 +53,12 @@
>          ]:
>          continue
>      varname = "COMMONJS%s" % relative.replace('/', '_')
>      print "%s_FILES = \\" % varname
>      for name in filenames:
>          print "  %s/%s \\" % (dirpath, name)
>      print "  $(NULL)"
>      print "%s_DEST = %s%s" % (varname, target_dir, relative)
> +    print "%s_TARGET = copy_source" % varname
>      print "INSTALL_TARGETS += %s\n" % varname
>  
>  print "include $(topsrcdir)/config/rules.mk"

I was able to reproduce this locally in beta by adding a sleep(1) in nsinstall.c:

@@ -419,6 +420,7 @@ main(int argc, char **argv)
                (void) (S_ISDIR(tosb.st_mode) ? rmdir : unlink)(toname);
                exists = 0;
            }
+           sleep(1);
            if (!exists && symlink(name, toname) < 0)
                fail("cannot make symbolic link %s", toname);

(Without this I wasn't able to get a double nsinstall on bootstrap.js, even with hundreds of builds - I guess it is just much more rare on my machine configuration).

In any case, I can confirm that glandium's patch fixes the issue.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Can one of you take this to get a proper patch up and a fix into the trees?

Comment 10

[Mike Hommey [:glandium]]

Attachment: Avoid race condition during libs in addon-sdk by using a different target for copy_source

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

<lmandel> nthomas: This is build config. I'm happy for you to land this. If you want approval, I'm also happy to give the a+.

So I landed a=lmandel: https://hg.mozilla.org/releases/mozilla-beta/rev/8c63e1286d75