1489698 - Split js/src/moz.build into multiple files

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Ted Campbell [:tcampbell]]

The jit directory is a large amount of moz.build and it doesn't seem unreasonable to build as a separate group of UNIFIED_SOURCES.

Proposal:
  - Add js/src/jit/moz.build for JIT files
  - Move general CXXFLAGS to js-cxxflags.mozbuild and allow js/src, js/src/jit, js/src/shell to all share
  - Same DEFINES to js-config.mozbuild for same build files

Comment 1

[Ted Campbell [:tcampbell]]

There seems to be some interest (at least from :waldo) about splitting off more than just JIT. Will post patches next week and we can discuss.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Cleanup CXXFLAGS in js/src/moz.build

- Remove CFLAGS definitions since we don't build any non-C++. They were
  misleading cruft, particularly references to fp:precise.

- Split fno-strict-aliasing from warnings list to make it clearer.

MozReview-Commit-ID: JND7TULAzLc

Comment 3

[Ted Campbell [:tcampbell]]

Attachment: Move js/src/moz.build defines to own file

MozReview-Commit-ID: ImcZFI3YlVo

Comment 4

[Ted Campbell [:tcampbell]]

Attachment: Move js/src/moz.build CXXFLAGS to own file

MozReview-Commit-ID: 3A0GNZ1hHeZ

Comment 5

[Ted Campbell [:tcampbell]]

Attachment: Use js-*.mozbuild in js/src subdirectories

MozReview-Commit-ID: zrKnXYGQ5X

Comment 6

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/jit

MozReview-Commit-ID: FKWHR6qFDHv

Comment 7

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/frontend

MozReview-Commit-ID: 4o5bOeYM07O

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/builtin

MozReview-Commit-ID: 3NY33sYghGK

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/gc

MozReview-Commit-ID: DgBvxMYIyUM

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/wasm

MozReview-Commit-ID: 3hfEx1hQ6jd

Comment 11

[Lars T Hansen [:lth]]

It will be highly beneficial if shell/moz.build can share js-config.mozbuild, as it's a constant source of errors that we forget to keep defines in js/src/shell/ in sync with defines in js/src/.  Doing so might be part of your original statement of work; just wanted to emphasize it.

Comment 12

[Ted Campbell [:tcampbell]]

(In reply to Lars T Hansen [:lth] from comment #11)
> It will be highly beneficial if shell/moz.build can share
> js-config.mozbuild, as it's a constant source of errors that we forget to
> keep defines in js/src/shell/ in sync with defines in js/src/.  Doing so
> might be part of your original statement of work; just wanted to emphasize
> it.

Yep, the "Use js-*.mozbuild in js/src subdirectories" patch makes sure to cover those cases.

Comment 13

[Ted Campbell [:tcampbell]]

sfink, are you able to review this sort of thing or is this a build-peer matter?

Comment 14

[Steve Fink [:sfink] [:s:]]

(In reply to Ted Campbell [:tcampbell] from comment #13)
> sfink, are you able to review this sort of thing or is this a build-peer
> matter?

It feels like build-peer territory to me.

Comment 15

[Ted Campbell [:tcampbell]]

Requested build-peer review for the cleanup patches. If they are happy with the approach, we can look at reviewing the actual per-directory splits.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 9007494 [details] [diff] [review]
Cleanup CXXFLAGS in js/src/moz.build

Review of attachment 9007494 [details] [diff] [review]:
-----------------------------------------------------------------

We apparently do build some C files. :)

::: js/src/moz.build
@@ -751,5 @@
>  
>  if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):
> -    # Prevent floating point errors caused by VC++ optimizations
> -    # XXX We should add this to CXXFLAGS, too?
> -    CFLAGS += ['-fp:precise']

Huh, we have dtoa.c, but we apparently build it via #include...wonder if that causes issues...

We have some vtune source files, would this matter there?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 9007495 [details] [diff] [review]
Move js/src/moz.build defines to own file

Review of attachment 9007495 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.

::: js/src/js-config.mozbuild
@@ +31,5 @@
> +    DEFINES['JS_HAS_CTYPES'] = True
> +    if not CONFIG['MOZ_SYSTEM_FFI']:
> +        DEFINES['FFI_BUILDING'] = True
> +    DEFINES['DLL_PREFIX'] = '"%s"' % CONFIG['DLL_PREFIX']
> +    DEFINES['DLL_SUFFIX'] = '"%s"' % CONFIG['DLL_SUFFIX']

These two got removed in bug 1399877, please remove them here as well.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 9007494 [details] [diff] [review]
Cleanup CXXFLAGS in js/src/moz.build

Review of attachment 9007494 [details] [diff] [review]:
-----------------------------------------------------------------

tcampbell and I worked out the differences on IRC.  Long story short, fp:precise is the default nowadays and is likely to remain so.

Comment 19

[Ted Campbell [:tcampbell]]

Comment on attachment 9007497 [details] [diff] [review]
Use js-*.mozbuild in js/src subdirectories

This patch tweaks the js/src/fuzz-tests/moz.build file so I thought I'd make sure you are aware.

Comment 20

[Ted Campbell [:tcampbell]]

Attachment: Move js/src/moz.build defines to own file

Comment 21

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/builtin

Comment 22

[Ted Campbell [:tcampbell]]

Comment on attachment 9009717 [details] [diff] [review]
Move js/src/moz.build defines to own file

Rebase past Bug 1399877. Carrying r=froydnj from Comment 17

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36de0f10e7f
Cleanup CXXFLAGS in js/src/moz.build. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeacff3b80fc
Move js/src/moz.build defines to own file. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5859f2671a8
Move js/src/moz.build CXXFLAGS to own file. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/725fbb08ff83
Use js-*.mozbuild in js/src subdirectories. r=froydnj

Comment 24

[Ted Campbell [:tcampbell]]

Landing the cleanup first. Individual directories can come as js-peers approve.

Comment 25

[Christian Holler (:decoder)]

Comment on attachment 9007497 [details] [diff] [review]
Use js-*.mozbuild in js/src subdirectories

Looks good to me and shouldn't cause any issues as long as try is also happy. Thanks for keeping me in the loop.

Comment 26

[Ted Campbell [:tcampbell]]

jsapi-tests is hitting linkage problems on Windows =\. I think I need to explictly add back |DEFINES['EXPORT_JS_API'] = True|.

Comment 27

[Cristina Coroiu [:ccoroiu]]

Backed out for spidermonkey failures 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6cd52c69c74a8769f8d45441498fdcaee5aca1

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=725fbb08ff83be7163a180d1bb47fd2aa1275756&selectedJob=199773065

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199773065&repo=mozilla-inbound&lineNumber=2665

mozmake[3]: *** [z:/task_1537211408/src/config/rules.mk:1123: Unified_cpp_js_src_jsapi-tests2.obj] Error 2
Unified_cpp_js_src_shell0.cpp
z:\task_1537211408\src\js\src\vm/Runtime.h(83): error C2220: warning treated as error - no 'object' file generated
z:\task_1537211408\src\js\src\vm/Runtime.h(83): warning C4273: 'js::ReportOutOfMemory': inconsistent dll linkage
Z:\task_1537211408\src\obj-spider\dist\include\js/AllocPolicy.h(57): note: see previous definition of 'ReportOutOfMemory'
z:\task_1537211408\src\js\src\vm/Runtime.h(93): warning C4273: 'js::ReportOverRecursed': inconsistent dll linkage
z:\task_1537211408\src\js\src\jsfriendapi.h(234): note: see previous definition of 'ReportOverRecursed'
mozmake[3]: *** [z:/task_1537211408/src/config/rules.mk:1123: Unified_cpp_js_src_shell0.obj] Error 2
mozmake[3]: Leaving directory 'Z:/task_1537211408/src/obj-spider/js/src/shell'
mozmake[2]: *** [z:/task_1537211408/src/config/recurse.mk:74: js/src/shell/target] Error 2
Unified_cpp_js_src_jsapi-tests3.cpp
z:\task_1537211408\src\js\src\vm/Runtime.h(83): error C2220: warning treated as error - no 'object' file generated
z:\task_1537211408\src\js\src\vm/Runtime.h(83): warning C4273: 'js::ReportOutOfMemory': inconsistent dll linkage
Z:\task_1537211408\src\obj-spider\dist\include\js/AllocPolicy.h(57): note: see previous definition of 'ReportOutOfMemory'
z:\task_1537211408\src\js\src\vm/Runtime.h(93): warning C4273: 'js::ReportOverRecursed': inconsistent dll linkage
z:\task_1537211408\src\js\src\jsfriendapi.h(234): note: see previous definition of 'ReportOverRecursed'
mozmake[3]: *** [z:/task_1537211408/src/config/rules.mk:1123: Unified_cpp_js_src_jsapi-tests3.obj] Error 2
mozmake[3]: Leaving directory 'Z:/task_1537211408/src/obj-spider/js/src/jsapi-tests'
mozmake[2]: *** [z:/task_1537211408/src/config/recurse.mk:74: js/src/jsapi-tests/target] Error 2
mozmake[2]: Leaving directory 'Z:/task_1537211408/src/obj-spider'
mozmake[1]: *** [z:/task_1537211408/src/config/recurse.mk:34: compile] Error 2
mozmake[1]: Leaving directory 'Z:/task_1537211408/src/obj-spider'
mozmake: *** [z:/task_1537211408/src/config/rules.mk:432: default] Error 2
mozmake: Leaving directory 'Z:/task_1537211408/src/obj-spider'
Traceback (most recent call last):
  File "./src/js/src/devtools/automation/autospider.py", line 413, in <module>
    run_command('%s -w %s' % (MAKE, MAKEFLAGS), shell=True, check=True)
  File "./src/js/src/devtools/automation/autospider.py", line 341, in run_command
    raise subprocess.CalledProcessError(status, command, output=stderr)
subprocess.CalledProcessError: Command 'mozmake -w -j6' returned non-zero exit status 2
+ BUILD_STATUS=1

Comment 28

[Ted Campbell [:tcampbell]]

Attachment: Use js-*.mozbuild in js/src subdirectories

Put the EXPORT_JS_API defines back. They are weird but currently needed to build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=680507faab23893852445fb9e2b6483f37e565b3

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa56a2cab83
Cleanup CXXFLAGS in js/src/moz.build. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a27ee7d892b
Move js/src/moz.build defines to own file. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/56d5a9b4eb9d
Move js/src/moz.build CXXFLAGS to own file. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b883684d4ee6
Use js-*.mozbuild in js/src subdirectories. r=froydnj

Comment 30

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/caa56a2cab83
https://hg.mozilla.org/mozilla-central/rev/6a27ee7d892b
https://hg.mozilla.org/mozilla-central/rev/56d5a9b4eb9d
https://hg.mozilla.org/mozilla-central/rev/b883684d4ee6

Comment 31

[Ted Campbell [:tcampbell]]

Notes to self: Patches for actual per-directory moz.build need to be rebased and perf-tested (sanity check). Review from both a build peer and js peer is a good idea.

Comment 32

[Ted Campbell [:tcampbell]]

Comment on attachment 9009718 [details] [diff] [review]
Add moz.build for js/src/builtin

This is super busted on Windows as we wind up with js/src/builtin/String.h on the search path as string.h (case insensitivity) and the standard library tries to use it...

Comment 33

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9009718 [details] [diff] [review]
Add moz.build for js/src/builtin

Review of attachment 9009718 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, I personally would not mind if we needed to #include "js/src/builtin/String.h", even if we only changed includes incrementally as needed for filename collisions. (Or some other include path would be ok too, though I'd rather not conflate "js/foo.h" coming from js/public/foo.h with "js/builtin/foo.h" coming from js/src/builtin/foo.h" -- at least not until we eliminate all js/src/*.h files.)

But that might involve fixing up check_spidermonkey_style.py, so perhaps renaming the file is the path of least resistance for now?

Comment 34

[Ted Campbell [:tcampbell]]

Comment on attachment 9007498 [details] [diff] [review]
Add moz.build for js/src/jit

Disassembler / BaselineCompiler-x mentions will be removed in the rebase.

Comment 35

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/frontend

Comment 36

[Jan de Mooij [:jandem]]

Comment on attachment 9007498 [details] [diff] [review]
Add moz.build for js/src/jit

Review of attachment 9007498 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 37

[Nathan Froyd [:froydnj]]

Comment on attachment 9020016 [details] [diff] [review]
Add moz.build for js/src/frontend

Review of attachment 9020016 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a17dfbac6b10
Add moz.build for js/src/jit. r=jandem,froydnj

Comment 39

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a17dfbac6b10

Comment 40

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] [:s:] (PTO Nov 5-16) from comment #33)
> FWIW, I personally would not mind if we needed to #include
> "js/src/builtin/String.h", even if we only changed includes incrementally as
> needed for filename collisions.

YES!  Having to think about how to #include stuff is stupid, and topsrcdir-relative paths are *the* one thing that is immediately blindingly obvious and clear and eliminates all need to Think (other than whether the file in question was explicitly exported to the includedir -- which explicit export still seems like a good thing to me).

> (Or some other include path would be ok too,
> though I'd rather not conflate "js/foo.h" coming from js/public/foo.h with
> "js/builtin/foo.h" coming from js/src/builtin/foo.h" -- at least not until
> we eliminate all js/src/*.h files.)

NO!  Changing one complicated non-obvious scheme is basically meaningless.  :-)

But yes, carry on with whatever here that's easy.  :-)

Comment 41

[Ted Campbell [:tcampbell]]

The String.h problem is that *ours* get's included by msvc's other headers. That doesn't get solved by more-qualified include paths in our code.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93ecf65646b9
Add moz.build for js/src/frontend. r=waldo,froydnj

Comment 43

[Mike Hommey [:glandium]]

(In reply to Ted Campbell [:tcampbell] from comment #41)
> The String.h problem is that *ours* get's included by msvc's other headers.
> That doesn't get solved by more-qualified include paths in our code.

It does if you remove the directory containing it from the include paths.

Comment 44

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/93ecf65646b9

Comment 45

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/gc

Comment 46

[Ted Campbell [:tcampbell]]

Attachment: Add moz.build for js/src/wasm

Comment 47

[Nathan Froyd [:froydnj]]

Comment on attachment 9021891 [details] [diff] [review]
Add moz.build for js/src/gc

Review of attachment 9021891 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/moz.build
@@ +43,5 @@
> +    'WeakMapPtr.cpp',
> +    'Zone.cpp',
> +]
> +
> +# StoreBuffer.cpp cannot be built in unified because its template

Nit (this is preexisting though, so...): "...in unified mode"

Comment 48

[Jon Coppeard (:jonco)]

Comment on attachment 9021891 [details] [diff] [review]
Add moz.build for js/src/gc

Review of attachment 9021891 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/298e3182979c
Add moz.build for js/src/gc. r=jonco,froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/520d47760632
Add moz.build for js/src/wasm. r=luke,froydnj

Comment 50

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/298e3182979c
https://hg.mozilla.org/mozilla-central/rev/520d47760632

Comment 51

[Ted Campbell [:tcampbell]]

Main cleanups landed and a number of subdirectories files added. I don't have immediate plans to tackle other directories.