Last Comment Bug 742391 - split config/rules.mk into a hierarchy of makefiles: file batch #1
: split config/rules.mk into a hierarchy of makefiles: file batch #1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Joey Armstrong [:joey]
:
Mentors:
Depends on:
Blocks: 744539 744540 668861
  Show dependency treegraph
 
Reported: 2012-04-04 08:33 PDT by Joey Armstrong [:joey]
Modified: 2012-05-16 19:58 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
file batch #1: breakup config/rules.mk (48.62 KB, patch)
2012-04-04 08:45 PDT, Joey Armstrong [:joey]
ted: review-
Details | Diff | Review
file batch #1: breakup config/rules.mk (37.58 KB, patch)
2012-04-12 06:04 PDT, Joey Armstrong [:joey]
ted: review+
Details | Diff | Review
rules.mk hierarchy split - batch #1 (40.59 KB, patch)
2012-04-30 10:46 PDT, Joey Armstrong [:joey]
ryanvm: checkin+
Details | Diff | Review
split config/rules.mk - file batch #1 (41.72 KB, patch)
2012-05-15 14:40 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Review
split config/rules.mk - file batch #1 (41.72 KB, patch)
2012-05-16 06:32 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Review

Description Joey Armstrong [:joey] 2012-04-04 08:33:58 PDT
+++ This bug was initially created as a clone of Bug #668861 +++

common elements of export, libs and tools in rules.mk have extracted into config/makefiles/{export,libs,tools}.mk

For part two extract all of the workhorse macros and rules that make up exports:: libs::, etc and move them out of rules.mk.
XPIDL goop for exports::
compiling/linking rules for libs::
Comment 1 Joey Armstrong [:joey] 2012-04-04 08:45:06 PDT
Created attachment 612203 [details] [diff] [review]
file batch #1: breakup config/rules.mk

echo* and show* debug targets moved into config/makefiles/debugmake.mk.
a conditional will now parse and load targets only when needed.

xpcshell targets and logic moved into config/makefiles/xpcshell.mk

more lib:: target cleanup.

Suffixed #{, #} on some if/ifdef conditional blocks that were long so editors can easily display enclosing blocks.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-04-09 10:42:01 PDT
Comment on attachment 612203 [details] [diff] [review]
file batch #1: breakup config/rules.mk

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

This mostly looks good, there's just one change I don't think is worth making.

::: config/makefiles/prefs_js_exports.mk
@@ +2,5 @@
> +# vim:set ts=8 sw=8 sts=8 noet:
> +#
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.

I don't think this stuff deserves its own Makefile. Splitting rules.mk down to smaller chunks is fine, but this is a little too fine-grained for my taste. (It's certainly a matter of personal taste, this just crosses the threshold for me.) Let's just leave this in rules.mk for now.

::: config/makefiles/xpcshell.mk
@@ +5,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +#
> +
> +ifndef INCLUDED_TESTS_XPCSHELL_MK #{

We should really figure out a way to merge the contents of this file (which implements targets to run xpcshell tests on a per-directory basis) with the targets in testsuite-targets.mk, which lets you run all xpcshell tests from the top-level:
http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk
Comment 3 Joey Armstrong [:joey] 2012-04-11 12:29:11 PDT
(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 612203 [details] [diff] [review]
> file batch #1: breakup config/rules.mk
> 
> Review of attachment 612203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This mostly looks good, there's just one change I don't think is worth
> making.
> 
> ::: config/makefiles/prefs_js_exports.mk
> @@ +2,5 @@
> > +# vim:set ts=8 sw=8 sts=8 noet:
> > +#
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> I don't think this stuff deserves its own Makefile. Splitting rules.mk down
> to smaller chunks is fine, but this is a little too fine-grained for my
> taste. (It's certainly a matter of personal taste, this just crosses the
> threshold for me.) Let's just leave this in rules.mk for now.

No problem, if the hierarchy shakes out this can always be revisited later on.


> ::: config/makefiles/xpcshell.mk
> @@ +5,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > +# You can obtain one at http://mozilla.org/MPL/2.0/.
> > +#
> > +
> > +ifndef INCLUDED_TESTS_XPCSHELL_MK #{
> 
> We should really figure out a way to merge the contents of this file (which
> implements targets to run xpcshell tests on a per-directory basis) with the
> targets in testsuite-targets.mk, which lets you run all xpcshell tests from
> the top-level:
> http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk

I'll open another bug so it can be handled in a followup patch.
Comment 4 Joey Armstrong [:joey] 2012-04-12 06:04:43 PDT
Created attachment 614345 [details] [diff] [review]
file batch #1: breakup config/rules.mk

Patch deltas from the last submission:
  o removed (js/src/)?config/makefiles/prefs_js_exports.mk
  o removed (js/src/)?config/makefiles/target_libs.mk [ cosmetic edits ]
Comment 5 Joey Armstrong [:joey] 2012-04-12 06:07:07 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7fd36eef73d5
  - patch was tested with prefs_js_exports.mk and target_libs.mk in the try submission but were removed in the uploaded patch.  target_libs contained only cosmetic edits.  rules.mk was modified to no longer include prefs_js_exports but the file had not yet been deleted from the patch.
Comment 6 Joey Armstrong [:joey] 2012-04-16 08:13:05 PDT
ping on code review
Comment 7 Ted Mielczarek [:ted.mielczarek] 2012-04-23 05:47:58 PDT
Comment on attachment 614345 [details] [diff] [review]
file batch #1: breakup config/rules.mk

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

::: config/makefiles/xpcshell.mk
@@ +20,5 @@
> +endef # do not remove the blank line!
> +
> +SOLO_FILE ?= $(error Specify a test filename in SOLO_FILE when using check-interactive or check-one)
> +
> +libs :: libs-xpcshell-tests

Minor formatting nit: the :: should be snuggled to the target name.

::: config/rules.mk
@@ +1771,5 @@
>  	$(LOOP_OVER_DIRS)
>  
> +ifndef INCLUDED_DEBUGMAKE_MK #{
> +  ## Only parse when an echo* or show* target is requested
> +  ## Todo[743243] - isTargetStem(echo, show)

Clever! I guess we should get that other patch landed.
Comment 8 Joey Armstrong [:joey] 2012-04-30 10:46:38 PDT
Created attachment 619613 [details] [diff] [review]
rules.mk hierarchy split - batch #1

r=ted carried forward.
Fixed nit, removed whitespace between target and double-colon.

Minor enhancement: bug 743243 - isTargetStem patch landed.  Replaced string check for echo/show with a call to the library function.  isTargetStem function modified to accept a list of arguments.  Unit test updated to verify behavior.
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-05-04 19:52:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8ae0c81d32
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-05-04 20:05:16 PDT
http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

Backed out due to check-sync-dirs.py bustage.
TEST-UNEXPECTED-FAIL | check-sync-dirs.py | build file copies are not in sync
TEST-INFO | check-sync-dirs.py | file(s) found in:               /builds/slave/m-in-lnx/build/js/src/config
TEST-INFO | check-sync-dirs.py | differ from their originals in: /builds/slave/m-in-lnx/build/config
TEST-INFO | check-sync-dirs.py | differing file:                 ./makefiles/makeutils.mk
In general, the files in '/builds/slave/m-in-lnx/build/js/src/config'
should always be exact copies of originals in '/builds/slave/m-in-
lnx/build/config'.  A change made to one should also be made to the other.
See 'check-sync-dirs.py' for more details.
make[1]: *** [check-sync-dirs-config] Error 1

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d147cc71b0
Comment 12 Joey Armstrong [:joey] 2012-05-15 14:40:11 PDT
Created attachment 624198 [details] [diff] [review]
split config/rules.mk - file batch #1
Comment 13 Joey Armstrong [:joey] 2012-05-16 06:32:19 PDT
Created attachment 624365 [details] [diff] [review]
split config/rules.mk - file batch #1
Comment 14 Joey Armstrong [:joey] 2012-05-16 06:35:13 PDT
https://tbpl.mozilla.org/?tree=Try&rev=ba9d6d4bb843

Full re-merge with m-c.  Added -Imozbuild and --[a-z] args added in rules.mk.  nits fixed and check-sync-dirs reported no problems.
Comment 15 Joey Armstrong [:joey] 2012-05-16 06:35:47 PDT
r=ted carried forward
Comment 16 Joey Armstrong [:joey] 2012-05-16 11:48:44 PDT
Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/98ca9bd48170
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:58:01 PDT
https://hg.mozilla.org/mozilla-central/rev/98ca9bd48170

Note You need to log in before you can comment on or make changes to this bug.