Closed
Bug 742391
Opened 12 years ago
Closed 12 years ago
split config/rules.mk into a hierarchy of makefiles: file batch #1
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla15
People
(Reporter: joey, Assigned: joey)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 4 obsolete files)
41.72 KB,
patch
|
Details | Diff | Splinter Review |
+++ 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::
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
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.
Attachment #612203 -
Flags: review?(ted.mielczarek)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → joey
Comment 2•12 years ago
|
||
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
Attachment #612203 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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 ]
Attachment #612203 -
Attachment is obsolete: true
Attachment #614345 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
ping on code review
Comment 7•12 years ago
|
||
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.
Attachment #614345 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 8•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #619613 -
Flags: checkin+
Comment 10•12 years ago
|
||
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
Target Milestone: mozilla15 → ---
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2c8ae0c81d32 https://hg.mozilla.org/mozilla-central/rev/e8d147cc71b0
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #614345 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #619613 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #624198 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
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.
No longer depends on: 755327
Assignee | ||
Comment 15•12 years ago
|
||
r=ted carried forward
Assignee | ||
Comment 16•12 years ago
|
||
Inbound push: https://hg.mozilla.org/integration/mozilla-inbound/rev/98ca9bd48170
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98ca9bd48170
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•