742391 - split config/rules.mk into a hierarchy of makefiles: file batch #1

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

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

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

[(not currently active) Ted Mielczarek]

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

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

Attachment: 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]]

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

ping on code review

Comment 7

[(not currently active) Ted Mielczarek]

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

Attachment: 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8ae0c81d32

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c8ae0c81d32
https://hg.mozilla.org/mozilla-central/rev/e8d147cc71b0

Comment 12

[Joey Armstrong [:joey]]

Attachment: split config/rules.mk - file batch #1

Comment 13

[Joey Armstrong [:joey]]

Attachment: split config/rules.mk - file batch #1

Comment 14

[Joey Armstrong [:joey]]

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

r=ted carried forward

Comment 16

[Joey Armstrong [:joey]]

Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/98ca9bd48170

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98ca9bd48170