Closed Bug 575283 Opened 15 years ago Closed 14 years ago

Cleanup mozconfig files on all platforms

Categories

(Firefox Build System :: General, enhancement, P5)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: atzaus, Assigned: emorley)

References

Details

Attachments

(1 file, 9 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6 Build Identifier: Cleaning up mozconfig files Win32 Removed --enable-libxul Removed --enable-tests Replaced MOZ_DEBUG_SYMBOLS=1 with --enable-debug-symbols Linux/Linux64/MacOSX/MacOSX64 Removed --enable-libxul Removed --enable-tests Removed MOZ_DEBUG_SYMBOLS=1 Removed CFLAGS="-gdwarf-2" and CXXFLAGS="-gdwarf-2" (Now using --enable-debug-symbols="-gdwarf-2") Changed all MOZ_MAKE_FLAGS to -j4 for consistency Added to MOZ_MAKE_FLAGS="-j4" debug builds Linux/Linux64 Removed # more options for 1.9 vs mozilla-central perf comparisons # shouldn't do anything - we don't do profiled builds on linux mk_add_options PROFILE_GEN_SCRIPT=@TOPSRCDIR@/build/profile_pageloader.pl Reproducible: Always
Attached patch Win32 Patch (obsolete) — Splinter Review
Attached patch Linux Patch (obsolete) — Splinter Review
Attached patch Linux64 Patch (obsolete) — Splinter Review
Attached patch MacOSX Patch (obsolete) — Splinter Review
Attached patch MacOSX64 Patch (obsolete) — Splinter Review
Attached patch Combined Patch (obsolete) — Splinter Review
Is this in support of something or is it for good housekeeping?
Severity: normal → enhancement
Priority: -- → P5
wx24 has been talking with ted last week about all these mozconfig changes. Some of the changes like MOZ_DEBUG_SYMBOLS are to be taken and the other changes are removing the lines that are now by default like --enable-tests. Ted is this a patch we could ask your blessings or shall one of us review it first?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yeah, I can review these.
Assignee: nobody → atzaus
Attachment #454527 - Flags: review?(ted.mielczarek)
Comment on attachment 454527 [details] [diff] [review] Combined Patch Looks good to me. Thanks!
Attachment #454527 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 454527 [details] [diff] [review] Combined Patch >diff -r 14478e93fbc3 mozilla2/macosx/mozilla-central/nightly/mozconfig ( and release & xulrunner) >@@ -1,17 +1,8 @@ >-. $topsrcdir/build/macosx/universal/mozconfig >- ... >+ >+. $topsrcdir/build/macosx/universal/mozconfig Is moving this to the end really safe ? What if we want to override anything set in the universal/mozconfig ? How confident do you feel about these changes ? Should we be taking them for a spin through our staging system ? Please note that this should not land without signoff from RelEng.
Regarding . $topsrcdir/build/macosx/universal/mozconfig If you need to override something in the mozconfig it makes sense to keep it on top. I just followed the win32 mozconfig which have this at the bottom. Should I just switch to bottom for all platforms?
The universal mozconfig and choose-make-flags serve pretty different purposes though. The former is a long file that sets up the complicated cross-compile environment for a two-pass Mac build, used by anyone wanting to do that type of build. The latter is hack for MoCo build slaves to disable parallel make on some of our machines, where it causes hangs. I think we should preserve the status quo - universal mozconfig at the top, win32 make flags at the bottom. Note to RelEng: We'll need to map these changes to other branches where we have copies rather than symlinks, ie mozilla-2.0, electrolysis, tracemonkey, try, 3x twigs
Attached patch Combined Patch 2.0 (obsolete) — Splinter Review
Attachment #454522 - Attachment is obsolete: true
Attachment #454523 - Attachment is obsolete: true
Attachment #454524 - Attachment is obsolete: true
Attachment #454525 - Attachment is obsolete: true
Attachment #454526 - Attachment is obsolete: true
Attachment #454527 - Attachment is obsolete: true
642570 is not really a blocker, just related stuff. Not sure what the best way to flag that is. There is also the recent change for debug symbols. The build configs have --------------------------------------------- export CFLAGS="-gdwarf-2" export CXXFLAGS="-gdwarf-2" # For NSS symbols export MOZ_DEBUG_SYMBOLS=1 ac_add_options --enable-debug-symbols="-gdwarf-2" --------------------------------------------- They have no comments on why we need dwarf2, but if we don't we can remove this as debug symbols are now on by default.
We don't really need -gdwarf-2, since -g == -gdwarf-2 on the platforms we're building. I just added it to be explicit (and since at one time on OS X it was not the default). If we're going to use the defaults, then they're fine.
Do the mobile mozconfigs need changing? Or does "all platforms" mean "all Firefox desktop platforms" ?
(In reply to comment #16) > We don't really need -gdwarf-2, since -g == -gdwarf-2 on the platforms we're > building. I just added it to be explicit (and since at one time on OS X it was > not the default). If we're going to use the defaults, then they're fine. According to the manual, the default dwarf version used by gcc 4.5 is 3. Is that OK?
That's fine, I recall jimb saying there was almost no difference between 2 and 3.
atzaus, I've been working on doing something similar to that in this bug and already have a patch (applies to the now in-tree mozconfigs). Would you object to me taking this bug, or would you like to continue working on it? Thanks :-)
Assignee: atzaus → nobody
Component: Release Engineering → Build Config
Product: mozilla.org → Core
QA Contact: release → build-config
Version: other → Trunk
Attached patch Cleanup in-tree mozconfigs (obsolete) — Splinter Review
After the previous comment, I spotted the date on the prior patch/dates of comments above, so have decided to upload the patch I had already created/take assignment, since it's been 14 months since any significant activity. Hope that's ok :-) Removed the following from the now in-tree mozconfigs... Redundant options, since they are now the default: ac_add_options --disable-debug ac_add_options --enable-optimize ac_add_options --enable-debug-symbols (bug 636695) export MOZ_DEBUG_SYMBOLS=1 ac_add_options --enable-application=browser (bug 644861) Invalid options; no longer exist in configure: ac_add_options --disable-install-strip ac_add_options --disable-javaxpcom (bug 648593) ac_add_options --enable-accessibility ac_add_options --enable-debugger-info-modules (bug 636695) ac_add_options --enable-libxul (bug 638429) ac_add_options --enable-updater ac_add_options --enable-ipc (bug 638755) ac_add_options --enable-tests Redundant if |ac_add_options --enable-debug-symbols="-gdwarf-2"| present: export CFLAGS="-gdwarf-2" export CXXFLAGS="-gdwarf-2" Jemalloc is force disabled if |--enable-trace-malloc| is used (http://mxr.mozilla.org/mozilla-central/source/configure.in#6947), so I removed |--enable-jemalloc| if both present. I decided against rearranging the options in each mozconfig for now, to make the diff easier to follow.
Assignee: nobody → bmo
Attachment #459555 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #565847 - Flags: review?(khuey)
Correction to the above comment (thanks Callek for spotting), the following belong under the redundant options section, rather than invalid; but are still being removed, since they are the defaults: --disable-install-strip --enable-accessibility --enable-updater --enable-tests I also moved this bug to Core::Build Config since now the mozconfigs are in-tree, releng doesn't have the correct milestones to mark as mozilla10 etc. I can still request dual build peer/releng team reviews if needed. Thanks!
(In reply to Ed Morley [:edmorley] from comment #22) > Correction to the above comment (thanks Callek for spotting), the following > belong under the redundant options section, rather than invalid; but are > still being removed, since they are the defaults: > --disable-install-strip This is not the default. We use this on Mac builds so symbols aren't stripped during packaging, so you can run Shark on nightly builds. > --enable-accessibility Note that this is the default, except on Mac.
Comment on attachment 565847 [details] [diff] [review] Cleanup in-tree mozconfigs I believe there is an updated patch forthcoming.
Attachment #565847 - Flags: review?(khuey)
Attached patch Cleanup in-tree mozconfigs v2 (obsolete) — Splinter Review
(As before, but with the lines Ted mentioned removed). Removes the following... Redundant options, since they are now the default: ac_add_options --disable-debug ac_add_options --enable-optimize ac_add_options --enable-application=browser (bug 644861) ac_add_options --enable-tests ac_add_options --enable-updater ac_add_options --enable-debug-symbols (bug 636695) export MOZ_DEBUG_SYMBOLS=1 Invalid options, that no longer exist in configure: ac_add_options --disable-javaxpcom (bug 648593) ac_add_options --enable-debugger-info-modules (bug 636695) ac_add_options --enable-libxul (bug 638429) ac_add_options --enable-ipc (bug 638755) These are redundant if |ac_add_options --enable-debug-symbols="-gdwarf-2"| present: export CFLAGS="-gdwarf-2" export CXXFLAGS="-gdwarf-2" Jemalloc is force disabled if |--enable-trace-malloc| is used (http://mxr.mozilla.org/mozilla-central/source/configure.in#6947), so I removed |--enable-jemalloc| if both are present.
Attachment #565847 - Attachment is obsolete: true
Attachment #565908 - Flags: review?(catlee)
Comment on attachment 565908 [details] [diff] [review] Cleanup in-tree mozconfigs v2 Looks good to me. May want to push it to try to make sure everything still works.
Attachment #565908 - Flags: review?(ted.mielczarek)
Attachment #565908 - Flags: review?(catlee)
Attachment #565908 - Flags: review+
Comment on attachment 565908 [details] [diff] [review] Cleanup in-tree mozconfigs v2 Review of attachment 565908 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/config/mozconfig @@ -1,5 @@ > # This file specifies the build flags for Firefox. You can use it by adding: > # . $topsrcdir/browser/config/mozconfig > # to the top of your mozconfig file. > - > -ac_add_options --enable-application=browser I think it might be good to leave this in here just for sanity's sake, but it's not that big of a deal. ::: browser/config/mozconfigs/linux32/debug @@ -20,1 @@ > ac_add_options --enable-debug-symbols="-gdwarf-2" I think we could remove all of these as well, since debug symbols are on by default, and dwarf is the default. (You don't have to do that now if you don't want.)
Attachment #565908 - Flags: review?(ted.mielczarek) → review+
I've just run the attached patch (with the changes in comment 28 added locally), past try: https://tbpl.mozilla.org/?tree=Try&rev=63160fd92f97 However, OSX 10.6 is busted, with: { configure: error: --enable-application value not recognized (i386/build.mk does not exist). } (https://tbpl.mozilla.org/php/getParsedLog.php?id=6811759&tree=Try#error0) Looking at http://mxr.mozilla.org/mozilla-central/source/configure.in#4441 implies that MOZ_BUILD_APP had been set to i386 (hence configure looking for i386/build.mk), so the default of "browser" isn't being applied. But yet the other platforms (including OS X 10.5) seem fine, so presumably aren't having MOZ_BUILD_APP set prior. I've searched m-c and also buildbot-config to see if it's being set somewhere specifically for 10.6 (and why), but the closest I can find is the check here: http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig.common#54 I guess I could just leave |ac_add_options --enable-application=browser| in for OSX 10.6, but it would be nice to either comment next to it explaining why, or else fix the underlying problem - if it turns out to actually be a bug. Any ideas? :-)
10.6 is the only one platform doing universal builds, so it sounds like the universal build process doesn't respect the default --enable-application.
Ah thanks! I found |mk_add_options MOZ_BUILD_PROJECTS="i386 x86_64"| in the universal mozconfig, which gets fed finally into: http://mxr.mozilla.org/mozilla-central/source/client.mk#152 Seems like that line should be: > BUILD_PROJECT_ARG = $(MOZ_CURRENT_PROJECT) instead of: > BUILD_PROJECT_ARG = MOZ_BUILD_APP=$(MOZ_CURRENT_PROJECT) ...and then the check here http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig.common#54 changed to use BUILD_PROJECT_ARG rather than MOZ_BUILD_APP perhaps? Does that sounds about right? If so I'll file a bug on fixing that and make it a dep of this. Thanks again :-)
Well, client.mk historically has had support for building multiple projects, which is why that is there. (You can do MOZ_BUILD_PROJECTS="xulrunner browser", for example.) Historically that's been useful to build xulrunner + an app built on xulrunner, which is how we used to build Fennec.
So really sounds like the universal mozconfig.common is abusing MOZ_BUILD_PROJECTS when it should be using another variable, since i386 or x86_64 aren't valid projects per se. I guess I could just add a bodge to http://mxr.mozilla.org/mozilla-central/source/configure.in#4441 which sets back to browser if i386 or x86_64 found, but it's not exactly an ideal solution. For now I'm just going to leave |ac_add_options --enable-application=browser| in the OS X universal build mozconfigs, and add a comment explaining why, so I can get this landed. If you'd like the universal build behaviour fixed, let me know and I'll file a followup.
As v2, but with the changes mentioned in comment 28 and comment 33. Carrying forwards ted and catlee's r+. https://tbpl.mozilla.org/?tree=Try&rev=3c998b954ded
Attachment #565908 - Attachment is obsolete: true
Flags: in-testsuite-
Target Milestone: --- → mozilla10
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size:

OSZAR »