Closed Bug 738612 Opened 8 years ago Closed 8 years ago

Restore mozconfig-extra (Try) feature, or similar

Categories

(Release Engineering :: General, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sfink)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

See discussion in bug 558180.
shouldn't be too hard for you to add this back in.
Assignee: nobody → sgautherie.bz
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.
Blocks: 750635
Assignee: sgautherie.bz → nobody
Component: Release Engineering → Release Engineering: Developer Tools
QA Contact: release → lsblakk
QA Contact: lsblakk → hwine
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.
Attachment #653871 - Flags: review?(ted.mielczarek)
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?
Attachment #653871 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → sphink
(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.
Attachment #654278 - Flags: review?(ted.mielczarek)
(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.
Attachment #654278 - Attachment is obsolete: true
Attachment #654278 - Flags: review?(ted.mielczarek)
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.
Attachment #656098 - Flags: review?(ted.mielczarek) → review+
I messed up the rename of build/macosx/common -> build/macosx/mozconfig.common. Fixed. Proof: https://tbpl.mozilla.org/?tree=Try&rev=83e308353e8a
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).
Product: mozilla.org → Release Engineering
Component: Tools → General
You need to log in before you can comment on or make changes to this bug.