Closed Bug 1478499 Opened 6 years ago Closed 6 years ago

Linux64 Tup build of the Spidermonkey shell

Categories

(Firefox Build System :: General, enhancement)

x86_64
Linux
enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mgaudet, Assigned: chmanchester)

References

Details

Attachments

(6 files)

I managed to build the spidermonkey shell with tup recently and encountered three problems while doing it: 


Versions and Methods: 

Linux 64 (Ubuntu 17.04), grabbed tup with cd ~/.mozbuild && mach artifact toolchain --from-build linux64-tup (https://docs.google.com/presentation/d/1SHb9Vp0aWPbHz1Eo53h9Xbh4-6qmukSknFQXQc2BVqA/edit\#slide=id.g3c628ff6b6\_3\_0) 

Configure and build like this: 

    mkdir js/src/build_tup_DBG.OBJ
    cd js/src/build_tup_DBG.OBJ
    ../configure --enable-warnings-as-errors --enable-tests --disable-optimize --enable-debug --enable-build-backends=Tup


Failure 1:
----------

     [         ETA~=<1s Remaining=19          ]  52%tup error: Unable to insert duplicate tupid 355243
     - [355243] js/src/build_tup_DBG.OBJ/dist/system_wrappers/pixman.h
    * 22) [0.010s] js/src/build_tup_DBG.OBJ/config                                                                                                                                                                                                                                  
    tup error: The output file '../dist/system_wrappers/pixman.h' is listed multiple times in a command.
    tup error: Error parsing Tupfile line 7
      Line was: ': $(MOZ_OBJ_ROOT)/<early-generated-files> |> ^o python /home/mgaudet/mozilla-unified-clean/config/make-system-wrappers.py:ge...'
     [         ETA~=<1s Remaining=18          ]  55%
     *** tup: 1 job failed.


Resolution: 

Edit config/Tupfile, and remove one instance of ../dist/system\_wrappers/pixman.h 

Failure 2:
----------

    * 39) [0.000s] js/src/build_tup_DBG.OBJ/js/src/shell                                                                                                                                                                                                                            
    tup error: Explicitly named file 'js' in subdir 'js/src/build_tup_DBG.OBJ/dist/bin' is scheduled to be deleted (possibly the command that created it has been removed).
    tup error: Error parsing Tupfile line 9
      Line was: ': /home/mgaudet/mozilla-unified-clean/js/src/build_tup_DBG.OBJ/dist/bin/js |> !tup_ln |> $(MOZ_OBJ_ROOT)/js/src/js $(MOZ_OBJ...'
     [         ETA~=<1s Remaining=1           ]  97%
     *** tup: 1 job failed.
    mgaudet@ubuntu:~/mozilla-un

Resolution: 

There's a rule in js/src/shell/Tupfile 

 : /home/mgaudet/mozilla-unified-clean/js/src/build_tup_DBG.OBJ/dist/bin/js |\> !tup\_ln |\> $(MOZ_OBJ_ROOT)/js/src/js $(MOZ_OBJ_ROOT)/\<default\>

Delete it. 

Failure 3:
----------

     1314) [0.073s] js/src/build_tup_DBG.OBJ/modules/fdlibm/src: CXX /home/mgaudet/mozilla-unified-clean/modules/fdlibm/src/e_acos.cpp                                                                                                                                              
    * 1315) js/src/build_tup_DBG.OBJ/js/src/build: LINK libmozjs-63a1.so                                                                                                                                                                                                            
    /usr/bin/ld.gold: error: cannot open symverscript: No such file or directory
    /usr/bin/ld.gold: fatal error: unable to parse version script file symverscript
    clang-5.0: error: linker command failed with exit code 1 (use -v to see invocation)
     *** tup errors ***
     *** Command ID=331290 failed with return value 1
    tup error: Expected to write to file 'libmozjs-63a1.so' from cmd 331290 but didn't
     1316) [4.340s] js/src/build_tup_DBG.OBJ/js/src/build: AR libjs_static.a                                                                                                                                                                                                        
     [                                                              

Resolution: Edit js/src/build/Tupfile, and remove the version script argument 


With the above three failures fixed, it appears we build a working js shell (and jsapi-tests)
To summarize some comments on irc:

Failure 1 seems to be due to processing pixman.h twice in system-headers.mozbuild. This should be trivial to fix.

Failure 2 appears to be due to tup's distaste for rules depending on an output that is generated later in that file. We may be able to work around this until it's fixed in tup upstream.

Failure 3 appears to be a generated file (symverscript) at https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/js/src/build/Makefile.in#13 that needs to be converted to moz.build. I did something similar for the "symverscript" next to libxul in bug 1423815.
Assignee: nobody → cmanchester
I have a set of patches for this, but unfortunately Failure 2 is a little more complicated than I thought. We're attempting to install the js shell executable to $objdir/js/src/js, but many compile invocations will attempt to find headers in a potential directory $objdir/js/src/js (js-confdefs.h, in $objdir/js/src, contains #include "js/RequiredDefines.h"), so tup thinks all of these compilations should depend on the file $objdir/js/src/js.

As far as I can tell the additional symlink to $objdir/js/src/js is a matter of convention, although I don't know how useful this convention is to people in practice, so maybe we can skip this in the tup backend for now.
Attachment #8996144 - Flags: review?(core-build-config-reviews) → review?(nfroyd)
Comment on attachment 8996144 [details]
Bug 1478499 - Do not list pixman.h twice in system-headers.h

https://reviewboard.mozilla.org/r/260370/#review267442
Attachment #8996144 - Flags: review?(nfroyd) → review+
Comment on attachment 8996145 [details]
Bug 1478499 - Skip additional js shell installation via ObjdirFiles in the tup build.

https://reviewboard.mozilla.org/r/260372/#review267638
Attachment #8996145 - Flags: review+
Attachment #8996145 - Flags: review?(core-build-config-reviews)
Comment on attachment 8996146 [details]
Bug 1478499 - Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols.

https://reviewboard.mozilla.org/r/260374/#review267640

::: js/src/build/moz.build:34
(Diff revision 1)
> +    if CONFIG['OS_TARGET'] == 'Linux':
> +        GENERATED_FILES += ['symverscript']
> +        GENERATED_FILES['symverscript'].script = '/toolkit/library/gen_symverscript.py'
> +        GENERATED_FILES['symverscript'].inputs = ['symverscript.in']
> +        GENERATED_FILES['symverscript'].flags = [
> +            CONFIG['JS_LIBRARY_NAME'].replace('-', '_'),

For me locally, the Makefile version produced a symverscript with -DVERSION='js', but this ends up with something like 'mozjs_63a1'. Is that intentional? Or does it not matter?

::: js/src/build/moz.build
(Diff revision 1)
>  DIST_INSTALL = True
>  
> -# Ensure symbol versions of shared library on Linux do not conflict
> -# with those in libxul.
> -if CONFIG['OS_TARGET'] == 'Linux':
> -    LDFLAGS += ['-Wl,-version-script,symverscript']

When using clang locally, this originally added the -Wl,-version-script,symverscript to the command-line for linking libmozjs-63a1.so, but after this patch I no longer see it on the command-line (in either make or tup).

I think for tup we currently don't handle SYMBOLS_FILE at all, and for rules.mk it looks like no flag is added on Linux unless GCC_USE_GNU_LD is set.
Attachment #8996146 - Flags: review?(core-build-config-reviews)
Comment on attachment 8996146 [details]
Bug 1478499 - Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols.

https://reviewboard.mozilla.org/r/260374/#review267640

> For me locally, the Makefile version produced a symverscript with -DVERSION='js', but this ends up with something like 'mozjs_63a1'. Is that intentional? Or does it not matter?

Huh, yeah, it looks like it's expecting to see the `mozjs_$version` form since it replaces dashes with underscores in the make version, but it does always see "js".

> When using clang locally, this originally added the -Wl,-version-script,symverscript to the command-line for linking libmozjs-63a1.so, but after this patch I no longer see it on the command-line (in either make or tup).
> 
> I think for tup we currently don't handle SYMBOLS_FILE at all, and for rules.mk it looks like no flag is added on Linux unless GCC_USE_GNU_LD is set.

Hmm, I wonder if we can just always pass SYMBOLS_FILE to lld, it doesn't complain. We handle SYMBOLS_FILE in the tup backend here: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/python/mozbuild/mozbuild/backend/tup.py#380 I see it added when I try this locally, but you're right that we're not consistent with the make backend about being picky about gnu ld.
Comment on attachment 8996146 [details]
Bug 1478499 - Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols.

https://reviewboard.mozilla.org/r/260374/#review267640

> Huh, yeah, it looks like it's expecting to see the `mozjs_$version` form since it replaces dashes with underscores in the make version, but it does always see "js".

According to the bug that added the symbols files (bug 809430) we were intending to have a version number here, but either this never quite worked as expected or it regressed at some point and nobody noticed.
Comment on attachment 8996146 [details]
Bug 1478499 - Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols.

https://reviewboard.mozilla.org/r/260374/#review267640

> Hmm, I wonder if we can just always pass SYMBOLS_FILE to lld, it doesn't complain. We handle SYMBOLS_FILE in the tup backend here: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/python/mozbuild/mozbuild/backend/tup.py#380 I see it added when I try this locally, but you're right that we're not consistent with the make backend about being picky about gnu ld.

Oh, I didn't notice this before, but for some reason GCC_USE_GNU_LD is set when we use lld, just not under js/src, because we don't AC_SUBST it in js/src/old-configure.in. I'll add this to a "pre" patch.
Comment on attachment 8996146 [details]
Bug 1478499 - Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols.

https://reviewboard.mozilla.org/r/260374/#review267812
Attachment #8996146 - Flags: review+
Attachment #8996146 - Flags: review?(core-build-config-reviews)
Comment on attachment 8996483 [details]
Bug 1478499 - Move symbol version script generation for js shared library to moz.build.

https://reviewboard.mozilla.org/r/260554/#review267814
Attachment #8996483 - Flags: review?(mshal) → review+
Comment on attachment 8996484 [details]
Bug 1478499 - Specify the symbol version script for js with SYMBOLS_FILE.

https://reviewboard.mozilla.org/r/260556/#review267816
Attachment #8996484 - Flags: review?(mshal) → review+
Comment on attachment 8996485 [details]
Bug 1478499 - Consider GCC_USE_GNU_LD when passing a symbols file in the Tup build for consistency with the Make build.

https://reviewboard.mozilla.org/r/260558/#review267818
Attachment #8996485 - Flags: review?(mshal) → review+
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f72358d338a
Do not list pixman.h twice in system-headers.h r=froydnj
https://hg.mozilla.org/integration/autoland/rev/237dc169f486
Skip additional js shell installation via ObjdirFiles in the tup build. r=mshal
https://hg.mozilla.org/integration/autoland/rev/03f68a47b89e
Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols. r=mshal
https://hg.mozilla.org/integration/autoland/rev/6f6e562e2013
Move symbol version script generation for js shared library to moz.build. r=mshal
https://hg.mozilla.org/integration/autoland/rev/3982c14a834e
Specify the symbol version script for js with SYMBOLS_FILE. r=mshal
https://hg.mozilla.org/integration/autoland/rev/9dee6a08a2dd
Consider GCC_USE_GNU_LD when passing a symbols file in the Tup build for consistency with the Make build. r=mshal
Moving gen_symverscript.py under build/ should fix this, but I'll test on try.
Flags: needinfo?(cmanchester)
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b798a2902a46
Do not list pixman.h twice in system-headers.h r=froydnj
https://hg.mozilla.org/integration/autoland/rev/97e20fbd0365
Skip additional js shell installation via ObjdirFiles in the tup build. r=mshal
https://hg.mozilla.org/integration/autoland/rev/844b006cc3e9
Use JS_LIBRARY_NAME rather than LIBRARY_NAME when versioning js symbols. r=mshal
https://hg.mozilla.org/integration/autoland/rev/02c279671108
Move symbol version script generation for js shared library to moz.build. r=mshal
https://hg.mozilla.org/integration/autoland/rev/e815b69d6941
Specify the symbol version script for js with SYMBOLS_FILE. r=mshal
https://hg.mozilla.org/integration/autoland/rev/6d4cbc671448
Consider GCC_USE_GNU_LD when passing a symbols file in the Tup build for consistency with the Make build. r=mshal
See Also: → 1481908
You need to log in before you can comment on or make changes to this bug.