Closed Bug 1082910 Opened 5 years ago Closed 5 years ago

Race condition for sdk/bootstrap.js

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set

Tracking

(firefox34+ fixed, firefox35 unaffected, firefox36 unaffected)

RESOLVED FIXED
mozilla34
Tracking Status
firefox34 + fixed
firefox35 --- unaffected
firefox36 --- unaffected

People

(Reporter: nthomas, Assigned: glandium)

References

Details

Attachments

(1 file)

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:
[cltbld@b-linux64-ix-0010.build.releng.scl3.mozilla.com ~]$ 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.
[Tracking Requested - why for this release]: Causing builds to fail in buildbot, so far one dep build and two release builds.
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.
Flags: needinfo?(nfroyd)
(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?
Flags: needinfo?(nfroyd)
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"
(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.
(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.
Can one of you take this to get a proper patch up and a fix into the trees?
Flags: needinfo?(mshal)
Flags: needinfo?(mh+mozilla)
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
Attachment #8508384 - Flags: review?(mshal)
Flags: needinfo?(mshal)
Attachment #8508384 - Flags: review?(mshal) → review+
<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
Target Milestone: --- → mozilla34
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Summary: Race condition for sdk/bootstrap.js ? → Race condition for sdk/bootstrap.js
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.