Last Comment Bug 721807 - APKOpen.cpp fails to compile on X86 android
: APKOpen.cpp fails to compile on X86 android
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: mozglue (show other bugs)
: Trunk
: x86 Android
: -- normal (vote)
: mozilla13
Assigned To: Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
:
Mentors:
Depends on:
Blocks: 723713
  Show dependency treegraph
 
Reported: 2012-01-27 10:51 PST by Thomas G Eaton
Modified: 2012-02-15 09:19 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Proposed Patch (702 bytes, patch)
2012-01-27 10:52 PST, Thomas G Eaton
no flags Details | Diff | Review
fix v2 (1.14 KB, patch)
2012-02-14 23:43 PST, Makoto Kato [:m_kato] (PTO 6/20-21, 6/24)
mh+mozilla: review+
Details | Diff | Review

Description Thomas G Eaton 2012-01-27 10:51:54 PST
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928224508

Steps to reproduce:

I tried to build Fennec for an x86 Android tablet.


Actual results:

It failed to build APKOpen.cpp due to a chain of header dependencies ending in xpcom-config.h.  Since xpcom hadn't been built yet, it couldn't find the header.  


Expected results:

It should have known there was an stl dependency and built correctly.  The fix is to add STL_FLAGS= to mozglue/android/Makefile.in.
Comment 1 Thomas G Eaton 2012-01-27 10:52:31 PST
Created attachment 592177 [details] [diff] [review]
Proposed Patch
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-30 10:25:30 PST
You should ask review on someone. Brad, could you perhaps review this patch?
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2012-01-31 07:38:23 PST
Thomas, why does clearing the STL_FLAGS fix this bug? And why x86 only?
Comment 4 Thomas G Eaton 2012-02-03 08:37:22 PST
(In reply to Brad Lassey [:blassey] from comment #3)
> Thomas, why does clearing the STL_FLAGS fix this bug? And why x86 only?

When i pulled the latest code some time after the mozglue portion was reorganized from other-liscenses, it built fine for ARM/Android, but not for x86/Android. The new code created a dependency on stl_wrappers for APKOpen.  Specifically the cstring file.  The cstring file has a dependency on xpcom and it hadn't been built yet when APKOpen was being built.  The stl_wrappers dependency doesn't appear to be present in the ARM build.  So I made it for x86 only so as to not affect the ARM build.  

As far as to why/how the solution works, I got the recommendation from Mike Hommey and don't know what further implications it has.  

"Thomas,

You can probably add STL_FLAGS= in mozglue/android/Makefile.in. I think this would be the correct fix.

Mike
"
Comment 5 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-11 05:36:04 PST
glandium, why do you use cstring and vector into mozglue?  x86 compiler use stl-wrapper, but Zip.h uses cstring and vector.  So cstring includes mozalloc, so this issue occurs.
Comment 6 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-11 05:37:43 PST
comment #4's fix is hack.  Correct fix is that mozglue dones't use STL (cstring, vector and etc).
Comment 7 Mike Hommey [:glandium] 2012-02-11 07:51:33 PST
(In reply to Makoto Kato from comment #6)
> comment #4's fix is hack.  Correct fix is that mozglue dones't use STL
> (cstring, vector and etc).

No, the correct fix is that mozglue does no use mozalloc, and "STL_FLAGS=" is the way to do that.
Comment 8 Mike Hommey [:glandium] 2012-02-11 07:52:17 PST
I'd say the ifeq ($(CPU_ARCH),x86) in the patch is unnecessary.
Comment 9 Mike Hommey [:glandium] 2012-02-11 07:54:02 PST
Or, if you want an even more correct way to do it, it would be to fold mozalloc into mozglue, and remove mozalloc dependency on xpcom-config.h. While folding mozalloc into mozglue is in my agenda, it's more work than just adding "STL_FLAGS=", which, again, is a right way to fix this issue.
Comment 10 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-14 23:43:57 PST
Created attachment 597322 [details] [diff] [review]
fix v2
Comment 11 Mike Hommey [:glandium] 2012-02-14 23:48:16 PST
Comment on attachment 597322 [details] [diff] [review]
fix v2

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

::: mozglue/android/Makefile.in
@@ +67,5 @@
>  DEFINES += -DANDROID_ARM_LINKER
> +else
> +ifeq ($(CPU_ARCH),x86)
> +DEFINES += -DANDROID_X86_LINKER
> +endif

You don't need that. ANDROID_ARM_LINKER is only needed in mozglue/android for cacheflush, which is arm-specific. Nothing requires ANDROID_X86_LINKER, and nothing will.
Comment 12 Makoto Kato [:m_kato] (PTO 6/20-21, 6/24) 2012-02-15 02:38:22 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9156e16286dd
Comment 13 Marco Bonardo [::mak] 2012-02-15 09:19:01 PST
https://hg.mozilla.org/mozilla-central/rev/9156e16286dd

Note You need to log in before you can comment on or make changes to this bug.