Closed Bug 894885 Opened 7 years ago Closed 7 years ago

move common mozconfig options to the common mozconfig

Categories

(Firefox for Android :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: blassey, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patchSplinter Review
I also added ANDROID_NDK_VERSION, ANDROID_NDK_VERSION_32BIT and ANDROID_SDK_VERSION such that these can be included in a local mozconfig and any changes will be picked up.
Attachment #777081 - Flags: review?(khuey)
Is it intended that developers include the "common" mozconfig in their local mozconfigs? If so then changes to it should be severely restricted as they can have unintended consequences and/or break stuff. See for example https://bugzilla.mozilla.org/show_bug.cgi?id=892355#c10 and subsequent comments. If not then I should stop including that common mozconfig in my local one.
Attachment #777081 - Flags: review?(khuey) → review?(mh+mozilla)
Comment on attachment 777081 [details] [diff] [review]
patch

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

r=me with the following fix

::: mobile/android/config/mozconfigs/android-armv6/nightly
@@ -22,5 @@
> -
> -export JAVA_HOME=/tools/jdk6
> -export MOZILLA_OFFICIAL=1
> -export MOZ_TELEMETRY_REPORTING=1
> -export MOZ_PKG_SPECIAL=armv6

MOZ_PKG_SPECIAL should not be removed.
Attachment #777081 - Flags: review?(mh+mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Is it intended that developers include the "common" mozconfig in their local
> mozconfigs? If so then changes to it should be severely restricted as they
> can have unintended consequences and/or break stuff. See for example
> https://bugzilla.mozilla.org/show_bug.cgi?id=892355#c10 and subsequent
> comments. If not then I should stop including that common mozconfig in my
> local one.

In-tree mozconfigs are not for developer consumption.
Comment on attachment 777081 [details] [diff] [review]
patch

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

r=me with the following fix

::: mobile/android/config/mozconfigs/android-armv6/nightly
@@ -22,5 @@
> -
> -export JAVA_HOME=/tools/jdk6
> -export MOZILLA_OFFICIAL=1
> -export MOZ_TELEMETRY_REPORTING=1
> -export MOZ_PKG_SPECIAL=armv6

MOZ_PKG_SPECIAL should not be removed.

::: mobile/android/config/mozconfigs/common
@@ +32,5 @@
> +fi
> +
> +ac_add_options --with-android-gnu-compiler-version=4.7
> +ac_add_options --with-android-version=9
> +ac_add_options --with-system-zlib

Note: none of these three are actually required. The build system will use these values on its own.
Comment on attachment 777081 [details] [diff] [review]
patch

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

::: mobile/android/config/mozconfigs/common
@@ +35,5 @@
> +ac_add_options --with-android-version=9
> +ac_add_options --with-system-zlib
> +ac_add_options --enable-update-channel=${MOZ_UPDATE_CHANNEL}
> +ac_add_options --enable-profiling
> +ac_add_options --disable-elf-hack # --enable-elf-hack conflicts with --enable-profiling

Isn't that in conflict with the --enable-elf-hack above this chunk?
https://hg.mozilla.org/mozilla-central/rev/8e672b39183d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
This patch appears to have regressed a startup resident memory usage improvement that glandium landed not too long ago.

The improvement (~4.5MB) came from this range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=285c7cb20fc2&tochange=f4a2508ab57c

And the regression came from this range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=843d948e138b&tochange=8e672b39183d

The most likely culprit as far as I can see is that bug 892355 enabled elf-hack on armv6 builds (which is what AWSY runs on) and then this bug disabled it again (on all android builds). See also comment 5. I don't know what the intention here was - to keep or disable elf-hacking and on which builds?
Flags: needinfo?(mh+mozilla)
This enabled profiling on armv6 builds, which needs elf-hack disabled. Eventually, both won't conflict (bug 788974), but that currently doesn't work. This only affects nightlies, not release builds.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #8)
> This enabled profiling on armv6 builds, which needs elf-hack disabled.
> Eventually, both won't conflict (bug 788974), but that currently doesn't
> work. This only affects nightlies, not release builds.

ah, huh, I spoke too quickly... it does :(
So the following shouldn't be in the common file:

ac_add_options --enable-profiling
ac_add_options --disable-elf-hack # --enable-elf-hack conflicts with --enable-profiling
STRIP_FLAGS="--strip-debug"
And
ac_add_options --with-branding=mobile/android/branding/nightly
See comments 5-11. Looks like this needs a follow-up patch.
Status: RESOLVED → REOPENED
Flags: needinfo?(blassey.bugs)
Resolution: FIXED → ---
Attached patch mozconfig_follow_up.patch (obsolete) — Splinter Review
Attachment #779921 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(blassey.bugs)
Comment on attachment 779921 [details] [diff] [review]
mozconfig_follow_up.patch

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

::: mobile/android/config/mozconfigs/android-armv6/nightly
@@ +8,5 @@
> +ac_add_options --with-branding=mobile/android/branding/nightly
> +
> +# This will overwrite the default of stripping everything and keep the symbol table.
> +# This is useful for profiling with eideticker. See bug 788680
> +STRIP_FLAGS="--strip-debug"

Why --strip-debug here if profiling is not enabled in this mozconfig? Should this be moved to the x86 mozconfig instead?

::: mobile/android/config/mozconfigs/common
@@ +45,1 @@
>  . "$topsrcdir/mobile/android/config/mozconfigs/common.override"

According to the documentation in common.override, this include should be at the bottom of the individual mozconfigs so as to override anything in them, rather than at the bottom of the common config which will not have the same effect.
Attachment #779921 - Flags: review?(bugmail.mozilla) → review?(mh+mozilla)
Comment on attachment 779921 [details] [diff] [review]
mozconfig_follow_up.patch

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

I agree with everything kats said.
Attachment #779921 - Flags: review?(mh+mozilla) → review-
Attachment #779921 - Attachment is obsolete: true
Attachment #780026 - Flags: review?(mh+mozilla)
Comment on attachment 780026 [details] [diff] [review]
mozconfig_follow_up.patch

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

LGTM.

Sorry not to have cought these on the original patch review.
Attachment #780026 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/e36551e9cd27
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.