See discussion in bug 558180.

shouldn't be too hard for you to add this back in.

We already have something similar for OS X and linux at least: build/unix/mozconfig.linux build/macosx/common Adding one for windowns and having them all include a root file would probably fix this bug.

New Thunderbird Try is affected too.

I used this patch for my try push and it seemed to work.

Comment on attachment 653871 [details] [diff] [review] Include common mozconfigs so try pushes can modify them easily Not really sure who should review this. Please redirect if I got it wrong.

I should also mention that to test this I wrote a simple script to validate that I did not introduce any multiple includes in the mozconfig include graph, and that all of the mozconfigs (that I could find) do indeed include build/mozconfig.common. I'm not sure if I really did find all of the mozconfigs in the tree, though. The mozconfig include graph could afford to be refactored (eg all nightlies include a mozconfig.nightly instead of duplicating that portion of the configuration, etc.) but I did not attempt that in this patch.

Comment on attachment 653871 [details] [diff] [review] Include common mozconfigs so try pushes can modify them easily Review of attachment 653871 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but it doesn't let to override default settings :-/ ::: browser/config/mozconfigs/macosx32/debug @@ +1,1 @@ > . $topsrcdir/build/macosx/mozconfig.leopard Something is missing wrt this file, isn't it?

Comment on attachment 653871 [details] [diff] [review] Include common mozconfigs so try pushes can modify them easily Review of attachment 653871 [details] [diff] [review]: ----------------------------------------------------------------- ::: xulrunner/config/mozconfigs/common @@ +1,1 @@ > +. "$topsrcdir/build/mozconfig.common" Add a comment here like you did for the other common mozconfig files?

(In reply to Serge Gautherie (:sgautherie) from comment #7) > Comment on attachment 653871 [details] [diff] [review] > Include common mozconfigs so try pushes can modify them easily > > Review of attachment 653871 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good, but it doesn't let to override default settings :-/ Yeah, that's a good point that I was wondering about too. mozconfig-extra was included at the end. I put the mozconfig.common include at the top due to bug 558180 comment 48, but really I'd prefer to be able to override. Come to think of it, I don't even understand why putting it at the top would work for setting CC/CXX. Unless some of the mozconfigs use its current value somewhere? I'm going to make one for the bottom. > > ::: browser/config/mozconfigs/macosx32/debug > @@ +1,1 @@ > > . $topsrcdir/build/macosx/mozconfig.leopard > > Something is missing wrt this file, isn't it? Please explain. Double quotes? License header?

Bikeshedding on the name is welcome. This patch applies on top of the previous patch.

(In reply to Steve Fink [:sfink] from comment #9) > I put the mozconfig.common include at the top due > to bug 558180 comment 48, [...]. Come > to think of it, I don't even understand why putting it at the top would work > for setting CC/CXX. Unless some of the mozconfigs use its current value > somewhere? { Rafael Ávila de Espíndola (:espindola) 2011-10-10 17:19:22 CEST Maybe we could change the mozconfigs to start by including a mozconfig.common, even if mozconfig.common is normally empty? Doing it at the start (instead of at the end like the old mozconfig-extra) makes it easier to set CC and CXX. } Rafael, could you give some details, so that case could be documented? > > ::: browser/config/mozconfigs/macosx32/debug > > @@ +1,1 @@ > > > . $topsrcdir/build/macosx/mozconfig.leopard > > > > Something is missing wrt this file, isn't it? > > Please explain. Double quotes? License header? My bad, I missed to see that 'mozconfig.leopard' does include 'common'. By the way, maybe that 'common' file should be renamed 'mozconfig.common' too.

Comment on attachment 654278 [details] [diff] [review] Add mozconfig "override" files to be included after everything else, for overriding previously set options Review of attachment 654278 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/unix/mozconfig.linux @@ +1,4 @@ > . "$topsrcdir/build/mozconfig.common" > > +#CC=/tools/gcc-4.5-0moz3/bin/gcc > +#CXX=/tools/gcc-4.5-0moz3/bin/g++ I assume you either want not to touch these lines or to delete them... ::: xulrunner/config/mozconfigs/common @@ +1,5 @@ > +# 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/. > + > +# This file is included by all xulrunner mozconfigs You want to use "at the top of" in this comment too.

(In reply to Serge Gautherie (:sgautherie) from comment #11) > (In reply to Steve Fink [:sfink] from comment #9) > > > I put the mozconfig.common include at the top due > > to bug 558180 comment 48, [...]. Come > > to think of it, I don't even understand why putting it at the top would work > > for setting CC/CXX. Unless some of the mozconfigs use its current value > > somewhere? > > { > Rafael Ávila de Espíndola (:espindola) 2011-10-10 17:19:22 CEST > > Maybe we could change the mozconfigs to start by including a > mozconfig.common, even if mozconfig.common is normally empty? Doing it at > the start (instead of at the end like the old mozconfig-extra) makes it > easier to set CC and CXX. > } > > Rafael, could you give some details, so that case could be documented? Sorry, ted explained over IRC. The previous definitions are used for http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/mozconfig.common#34 when adding an -arch option for osx universal builds. > > > ::: browser/config/mozconfigs/macosx32/debug > > > @@ +1,1 @@ > > > > . $topsrcdir/build/macosx/mozconfig.leopard > > > > > > Something is missing wrt this file, isn't it? > > > > Please explain. Double quotes? License header? > > My bad, I missed to see that 'mozconfig.leopard' does include 'common'. > By the way, maybe that 'common' file should be renamed 'mozconfig.common' > too. I was trying to fit in with the current naming conventions, which seem to omit "mozconfig" when inside a mozconfigs/ directory.

(In reply to Serge Gautherie (:sgautherie) from comment #12) > Comment on attachment 654278 [details] [diff] [review] > Add mozconfig "override" files to be included after everything else, for > overriding previously set options > > Review of attachment 654278 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/unix/mozconfig.linux > @@ +1,4 @@ > > . "$topsrcdir/build/mozconfig.common" > > > > +#CC=/tools/gcc-4.5-0moz3/bin/gcc > > +#CXX=/tools/gcc-4.5-0moz3/bin/g++ > > I assume you either want not to touch these lines or to delete them... Doh! You're right; that change slipped in. (It's the only thing I need to do in order to make a local build using the in-tree linux64 mozconfigs.) I'll remove it from the patch. (And in fact, I just did a try server push where I erroneously left it in, too. Ugh.) Done. > ::: xulrunner/config/mozconfigs/common > @@ +1,5 @@ > > +# 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/. > > + > > +# This file is included by all xulrunner mozconfigs > > You want to use "at the top of" in this comment too. Done. Thanks.

(In reply to Steve Fink [:sfink] from comment #13) > (In reply to Serge Gautherie (:sgautherie) from comment #11) > Sorry, ted explained over IRC. The previous definitions are used for > http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/ > mozconfig.common#34 when adding an -arch option for osx universal builds. Iiuc, /build/macosx/universal/mozconfig.common includes /build/macosx/common which has some "export CC=...". Wouldn't these exports override the possible definitions in "/build/mozconfig.common", which you are including before the exports? > > By the way, maybe that 'common' file should be renamed 'mozconfig.common' > > too. > > I was trying to fit in with the current naming conventions, which seem to > omit "mozconfig" when inside a mozconfigs/ directory. Yes, I was referring to existing '/build/macosx/common'.

(In reply to Serge Gautherie (:sgautherie) from comment #15) > (In reply to Steve Fink [:sfink] from comment #13) > > (In reply to Serge Gautherie (:sgautherie) from comment #11) > > > Sorry, ted explained over IRC. The previous definitions are used for > > http://mxr.mozilla.org/mozilla-central/source/build/macosx/universal/ > > mozconfig.common#34 when adding an -arch option for osx universal builds. > > Iiuc, > /build/macosx/universal/mozconfig.common includes /build/macosx/common > which has some "export CC=...". > Wouldn't these exports override the possible definitions in > "/build/mozconfig.common", which you are including before the exports? :-) Sounds right. Perhaps that should be a followup -- change those to export CC=${CC-...}. It doesn't seem like there's a way to set CC and CXX via a common mozconfig right now. It's still useful for random things though. And we ought to create separate mozconfigs for m-c, c-c, and SeaMonkey, and include the appropriate ones. Unless we're junking mozconfigs and client.mk soonish. :-) > > > By the way, maybe that 'common' file should be renamed 'mozconfig.common' > > > too. > > > > I was trying to fit in with the current naming conventions, which seem to > > omit "mozconfig" when inside a mozconfigs/ directory. > > Yes, I was referring to existing '/build/macosx/common'. Oh, sorry. Right you are. Done.

Forgot to |hg add| a file.

Comment on attachment 656098 [details] [diff] [review] Add mozconfig "override" files to be included after everything else, for overriding previously set options Review of attachment 656098 [details] [diff] [review]: ----------------------------------------------------------------- A maze of twisty mozconfigs, all alike.

http://hg.mozilla.org/integration/mozilla-inbound/rev/81bbb0b8aff2 http://hg.mozilla.org/integration/mozilla-inbound/rev/ceb6f653de08

http://hg.mozilla.org/integration/mozilla-inbound/rev/20adcf83fc42 - backed out ceb6f653de08 http://hg.mozilla.org/integration/mozilla-inbound/rev/27030680d0cc - backed out 81bbb0b8aff2

Backed out for real. https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd45f6cce87

I messed up the rename of build/macosx/common -> build/macosx/mozconfig.common. Fixed. Proof: https://tbpl.mozilla.org/?tree=Try&rev=83e308353e8a

Re-landed: http://hg.mozilla.org/integration/mozilla-inbound/rev/40dd5296396e http://hg.mozilla.org/integration/mozilla-inbound/rev/7d85b2d9447f

https://hg.mozilla.org/mozilla-central/rev/7d85b2d9447f https://hg.mozilla.org/mozilla-central/rev/40dd5296396e

browser/config/mozconfigs/common says: "This file is included by all browser mozconfigs" I don't think that's true - it looks to me like it is only included in Windows mozconfigs.

(In reply to Mark Banner (:standard8) from comment #25) > browser/config/mozconfigs/common says: > > "This file is included by all browser mozconfigs" > > I don't think that's true - it looks to me like it is only included in > Windows mozconfigs. Win32 only mozconfigs to be precise. Win64 wasn't changed either (though they were for mozconfig.override).