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

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.

- 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

MozReview-Commit-ID: ImcZFI3YlVo

MozReview-Commit-ID: 3A0GNZ1hHeZ

MozReview-Commit-ID: zrKnXYGQ5X

MozReview-Commit-ID: FKWHR6qFDHv

MozReview-Commit-ID: 4o5bOeYM07O

MozReview-Commit-ID: 3NY33sYghGK

MozReview-Commit-ID: DgBvxMYIyUM

MozReview-Commit-ID: 3hfEx1hQ6jd

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.

(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.

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

(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.

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 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 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 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 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 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

Pushed by tcampbell@mozilla.com: 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

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

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.

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

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

Put the EXPORT_JS_API defines back. They are weird but currently needed to build. https://treeherder.mozilla.org/#/jobs?repo=try&revision=680507faab23893852445fb9e2b6483f37e565b3

Pushed by tcampbell@mozilla.com: 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

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

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 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 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 on attachment 9007498 [details] [diff] [review] Add moz.build for js/src/jit Disassembler / BaselineCompiler-x mentions will be removed in the rebase.

Comment on attachment 9007498 [details] [diff] [review] Add moz.build for js/src/jit Review of attachment 9007498 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 9020016 [details] [diff] [review] Add moz.build for js/src/frontend Review of attachment 9020016 [details] [diff] [review]: ----------------------------------------------------------------- \o/

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a17dfbac6b10 Add moz.build for js/src/jit. r=jandem,froydnj

(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. :-)

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.

Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/93ecf65646b9 Add moz.build for js/src/frontend. r=waldo,froydnj

(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 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 on attachment 9021891 [details] [diff] [review] Add moz.build for js/src/gc Review of attachment 9021891 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Pushed by tcampbell@mozilla.com: 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

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

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