Closed Bug 1381211 Opened 2 years ago Closed 2 years ago

Clean up SeaMonkeys mozconfig files

Categories

(SeaMonkey :: Build Config, enhancement)

enhancement
Not set

Tracking

(seamonkey2.52 affected, seamonkey2.53 fixed)

RESOLVED FIXED
seamonkey2.53
Tracking Status
seamonkey2.52 --- affected
seamonkey2.53 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(1 file, 1 obsolete file)

Title says it all
Attached patch 1381211-suite-mozconfigs.patch (obsolete) — Splinter Review
I looked at the mozconfigs for the api key patch and found some inconsistencies. 

First I think we could also remove release-10n in all configurations. The cleaned up files are identical to l10n-mozconfig. I didn't do it because I am not sure what breaks in automation if I do. 

Second, is the OSX hack still required?

> # Big Hack that unsets CC / CXX so that mozconfig.common doesn't get
> # mixed up with host/target CPUs when trying to work out how to do the
> # universal build. When we redo the build system (bug 648979) this will
> # go away.
> if test -e "$topsrcdir/suite/config/version.txt"; then
>   unset CC
>   unset CXX
> fi

We no longer do OSX universal builds.

I added the with-official-branding necessary for the suite patch in bug 1354845 here and will remove it from the patch there. It is only needed for the release configs and not the l10n ones as I thought first.

I also added a win64 release config for day x.

This can be further cleaned up There is much overlapping in them which will never individually change.
Attachment #8886769 - Flags: review?(iann_bugzilla)
Attachment #8886769 - Flags: review?(ewong)
Comment on attachment 8886769 [details] [diff] [review]
1381211-suite-mozconfigs.patch

r=me assuming the conflicts with Bug 1354845 are removed.
Attachment #8886769 - Flags: review?(iann_bugzilla) → review+
Blocks: 903439
Comment on attachment 8886769 [details] [diff] [review]
1381211-suite-mozconfigs.patch

Review of attachment 8886769 [details] [diff] [review]:
-----------------------------------------------------------------

no.. don't remove the release-l10n.    They are needed for release automation, whereas l10n-mozconfigs are for the 'normal'
l10n stuff.  Should they be the same?  I don't think so as we're probably using options on the normal l10n which we don't
use in the release environment.

All in all, looks good.  Just not particularly sure about the disable-compile-environment part.

I'm giving a 'reserved' r+.  (the l10n are broken... so they can't get anymore broken that his...) :)

::: suite/config/mozconfigs/linux32/nightly
@@ -5,5 @@
>  ac_add_options --disable-webrender
>  
>  ac_add_options --enable-application=suite
>  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
> -ac_add_options --disable-elf-hack

we don't need to do this? is it because we've moved on
to a different gcc (compared to when this was inserted to the mozconfig when we were still on the old linux slaves)?

::: suite/config/mozconfigs/linux64/nightly
@@ -5,5 @@
>  ac_add_options --disable-webrender
>  
>  ac_add_options --enable-application=suite
>  ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
> -ac_add_options --disable-elf-hack

we don't need to disable this anymore?

::: suite/config/mozconfigs/macosx64/l10n-mozconfig
@@ -6,5 @@
> -  unset CC
> -  unset CXX
> -fi
> -
> -. $topsrcdir/build/macosx/mozconfig.common

I believe we still need this line.

@@ +7,5 @@
>  ac_add_options --enable-calendar
>  
> +# Disabling is needed, see bug 1345422 comment #58.
> +ac_add_options --disable-compile-environment
> +

I'm not convinced this is the right thing to do, despite TB doing it.  However, since our l10n nightlies are screwed up, we can't test this until after 2.48 and after we fix this repack mess.

::: suite/config/mozconfigs/macosx64/release-l10n
@@ -1,1 @@
> -. "$topsrcdir/build/mozconfig.common"

we need this for automation.

::: suite/config/mozconfigs/win32/l10n-mozconfig
@@ +1,1 @@
>  . "$topsrcdir/build/mozconfig.common"

I suspect we'll also need "$topsrcdir/build/mozconfig.win-common" here as well.

::: suite/config/mozconfigs/win32/release-l10n
@@ +null,0 @@


Please keep this line.
Thanks will look at it over the weekend.

Do you think the "Big Hack that unsets CC / CXX" is still needed for OSX?

Should I add bug 1380814 too?

https://hg.mozilla.org/mozilla-central/rev/18aeda1e84fb

Might affect us in the future.
Flags: needinfo?(ewong)
> > -ac_add_options --disable-elf-hack
> we don't need to do this? is it because we've moved on
> ..

As far as I see it no longer needed. Both enable-elf-hack and enable-profiling are in the Fx nightly configs via includes. I think it is worth a try and we might be able to enable this in a followup bug. Success is only a backout away :)

See also bug 892355. I think the profiler has been fixed.
V2 comments from ewong addressed. r+ from IanN carried forward. reserved r+ from ewong too.

> ::: suite/config/mozconfigs/macosx64/release-l10n
> @@ -1,1 @@
> > -. "$topsrcdir/build/mozconfig.common"

mozconfig.common is pulled in via local-mozconfig.common which is pulled in via .$topsrcdir/build/macosx/mozconfig.common if not cross-compiled. 

> Do you think the "Big Hack that unsets CC / CXX" is still needed for OSX?
Answered this to myself here. The current debug config does not contain the hack and builds are produced. So no longer needed in the configs.

If something breaks I will either fix it or back the patch out if this can't be done easy.
Attachment #8886769 - Attachment is obsolete: true
Attachment #8886769 - Flags: review?(ewong)
Flags: needinfo?(ewong)
Attachment #8889507 - Flags: review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/abd557868538
Clean up suite mozconfig files and align them with TB and Fx. r=IanN,ewong
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
I hope this stays closed :)

https://hg.mozilla.org/comm-central/rev/abd55786853812e0f1aed0c286c9bc432d06da2a
Target Milestone: --- → seamonkey2.53
Duplicate of this bug: 1214705
You need to log in before you can comment on or make changes to this bug.