b2g doesn't build with gcc 4.7

RESOLVED FIXED

Status

P4
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: julienw, Assigned: hub)

Tracking

unspecified
x86_64
Linux

Firefox Tracking Flags

(b2g-v1.2 affected)

Details

(Whiteboard: [c= p=1 s= u=])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
The error we get with gcc 4.7 is :

frameworks/base/include/utils/KeyedVector.h: In instantiation of 'const VALUE& android::DefaultKeyedVector<KEY, VALUE>::valueFor(const KEY&) const [with KEY = android::String8; VALUE = android::sp<AaptDir>]':
frameworks/base/tools/aapt/AaptAssets.cpp:1700:53:   required from here
frameworks/base/include/utils/KeyedVector.h:196:31: error: 'indexOfKey' was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]
frameworks/base/include/utils/KeyedVector.h:196:31: note: declarations in dependent base 'android::KeyedVector<android::String8, android::sp<AaptDir> >' are not found by unqualified lookup
frameworks/base/include/utils/KeyedVector.h:196:31: note: use 'this->indexOfKey' instead 

We should do one of :
* fix this header
* have a check for the available gcc versions
Anyone know how hard it would be to check in a change to this header?

Comment 2

6 years ago
The current policy is that only gcc 4.6 is supported on host for ICS based devices. JB support will bring in support for newer toolchains.
(In reply to Michael Wu [:mwu] from comment #2)
> The current policy is that only gcc 4.6 is supported on host for ICS based
> devices. JB support will bring in support for newer toolchains.

But similarly, the current Android policy seems to be that MacOS isn't supported, based on the fact that they have many files whose names differ only in case.  Yet we support MacOS B2G builds.

Anyway, I don't care whether we patch this header or provide a meaningful error message.  Just that we do something other than hide this requirement in our docs.

Comment 4

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #3)
> (In reply to Michael Wu [:mwu] from comment #2)
> > The current policy is that only gcc 4.6 is supported on host for ICS based
> > devices. JB support will bring in support for newer toolchains.
> 
> But similarly, the current Android policy seems to be that MacOS isn't
> supported, based on the fact that they have many files whose names differ
> only in case.  Yet we support MacOS B2G builds.
> 

Actually, Android policy is that osx builds are supported on case sensitive file systems as long as you're not building the emulator. (in which case an old gcc is required to build)

> Anyway, I don't care whether we patch this header or provide a meaningful
> error message.  Just that we do something other than hide this requirement
> in our docs.

A meaningful error message or automatically picking gcc-4.6 where it's available is fine. Your error messages just happened to break builds last time. (though not mine)
> Your error messages just happened to break builds last time. (though not mine)

I thought we fixed it.

https://github.com/mozilla-b2g/platform_build/pull/5

But it appears that the build system has changed since then.
The upstream change we're looking for appears to be 7a4d92af9bdcf94d770bfa8313ad5b21c829ed96, and I have a patch we can apply to our current version for it.

It looks like we prefer to pull in mozilla-b2g repos from https://git.mozilla.org/releases; is gonk-patches already there, or do things need to be done first?
Assignee: nobody → jld
Flags: needinfo?(jhford)

Comment 7

6 years ago
No patches for fixing this on ICS will be accepted. Warning about gcc versions or automatically using gcc 4.6 is ok.
Flags: needinfo?(jhford)
(In reply to Jed Davis [:jld] from comment #6)
> The upstream change we're looking for appears to be
> 7a4d92af9bdcf94d770bfa8313ad5b21c829ed96

If anyone wants to patch their own copy, another way: the line changed under an #ifdef __clang__ 3975b24c9 should be enough (unifdef'ed) for recent GCC; the other two lines that 7a4d92af9 changes are in the base class and should be fine.  This advice is untested.
(Assignee)

Comment 9

6 years ago
This is the bugs I filed eons ago. Before b2g used Bugzilla. 
https://github.com/mozilla-b2g/B2G/issues/33
(Assignee)

Comment 10

6 years ago
(In reply to Michael Wu [:mwu] from comment #7)
> No patches for fixing this on ICS will be accepted. Warning about gcc
> versions or automatically using gcc 4.6 is ok.

Sadly this is the kind of stuff git would allow us to do and that would save countless of engineering time. Invalid C++ code is invalid.
git won't allow you to push this change upstream...

In order to do this in git, you'd need to fork the repository (wh(In reply to Hubert Figuiere [:hub] from comment #10)
> (In reply to Michael Wu [:mwu] from comment #7)
> > No patches for fixing this on ICS will be accepted. Warning about gcc
> > versions or automatically using gcc 4.6 is ok.
> 
> Sadly this is the kind of stuff git would allow us to do and that would save
> countless of engineering time. Invalid C++ code is invalid.

I don't see how git helps you out here. We don't control the repository that this comes from.
(Assignee)

Comment 12

6 years ago
Need a fix? Mirror the repository and use that to maintain a limited set of patches.

AFAIK we don't (nor shoudln't) automatically follow new versions of upstream as we set hourselves on a base OS - so this is just a matter of pointing to our repository. And when a new version comes we check whether we need the patches or not.
(Reporter)

Comment 13

6 years ago
As far as we have a clear path on supporting gcc 4.7 at one time, I don't care if we don't support it now. Let's guard against gcc 4.6 not being there for now.
(Assignee)

Comment 14

6 years ago
(In reply to Julien Wajsberg [:julienw] from comment #13)
> As far as we have a clear path on supporting gcc 4.7 at one time, I don't
> care if we don't support it now. Let's guard against gcc 4.6 not being there
> for now.

As long as I can override it.
You can of course override it by backing out this patch locally.

Do you want something more convenient than that?  If so, what's the use-case?  We don't have convenient overrides for all of the unsupported compilers in mozilla-central.

I'd like it if we didn't over-complicate this bug with edge-case feature requests.  I already implemented it once and that functionality was removed during an apparent build-system refactoring.  The simpler this is, the more likely we'll be able to keep this in.
(Assignee)

Comment 16

6 years ago
(In reply to Justin Lebar [:jlebar] from comment #15)
> You can of course override it by backing out this patch locally.

I already have to keep patches locally to build, which is why we are having that conversation.

> Do you want something more convenient than that? 

Yes. We already have overrides for other stuff. Of course I'd just prefer we actually fix the problem.

> If so, what's the
> use-case?  We don't have convenient overrides for all of the unsupported
> compilers in mozilla-central.

4.7 IS supported on mozilla-central. I build inbound with that.
 
> I'd like it if we didn't over-complicate this bug with edge-case feature
> requests.  I already implemented it once and that functionality was removed
> during an apparent build-system refactoring.  The simpler this is, the more
> likely we'll be able to keep this in.

Which is why I'm proposing we actually fix stuff instead of working around it. Invalid C++ code is invalid whihc is the actual problem. Instead of fixing it because more recent compiler are more strict we want to prevent having said recent compiler.

A bit like ignoring warnings because they are warnings.
> We already have overrides for other stuff.

That's not correct.  For example, there is no override in m-c to let you configure with gcc 3, other than modifying configure.  That's because gcc 3 is not supported.

> Which is why I'm proposing we actually fix stuff instead of working around it.

It's fine that you propose that, but the module owner has said no.  So that's over.

I feel your pain, but please don't make this harder for us.  If you do, it's likely that we won't fix this bug at all, and that would be a shame.

Updated

6 years ago
Whiteboard: c=performance
Priority: -- → P4
Summary: b2g doesn't build with gcc 4.7 → fail faster when b2g is built with gcc 4.7 as a host compiler
Whiteboard: c=performance
Assignee: jld → nobody
(Assignee)

Comment 18

5 years ago
got a fix involving using the vendor patches system. will submit a PR
Assignee: nobody → hub
(Assignee)

Comment 19

5 years ago
Created attachment 801550 [details] [diff] [review]
Bug 854389 - Patch to allow building with gcc 4.7

This works for hamachi. Should work for buri.

But the patches repository is not pulled for other platform. Will need another patch.
(Assignee)

Updated

5 years ago
Summary: fail faster when b2g is built with gcc 4.7 as a host compiler → b2g doesn't build with gcc 4.7
(Assignee)

Updated

5 years ago
Attachment #801550 - Attachment is obsolete: true
Created attachment 802555 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-patches/pull/4

Pointer to Github pull-request
Comment on attachment 802555 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-patches/pull/4

I was looking at the PR and I must have accidentally clicked the 'Attach to bug...' button. Sorry :(
Attachment #802555 - Attachment is obsolete: true
(Assignee)

Comment 24

5 years ago
Created attachment 802810 [details] [diff] [review]
gonk-patches PR
(Assignee)

Comment 25

5 years ago
Created attachment 802811 [details] [diff] [review]
b2g-manifest PR
(Assignee)

Comment 26

5 years ago
Created attachment 802813 [details] [diff] [review]
android-sdk PR
(Assignee)

Updated

5 years ago
Attachment #802810 - Flags: review?(21)
(Assignee)

Updated

5 years ago
Attachment #802811 - Flags: review?(21)
(Assignee)

Updated

5 years ago
Attachment #802813 - Flags: review?(mwu)
(Assignee)

Updated

5 years ago
Attachment #802811 - Flags: review?(21) → review?(mwu)
(Assignee)

Updated

5 years ago
Attachment #802810 - Flags: review?(21) → review?(mwu)

Updated

5 years ago
Status: NEW → ASSIGNED
Hardware: x86_64 → x86
Whiteboard: [c= p= s=2013.09.20 u=]

Updated

5 years ago
Whiteboard: [c= p= s=2013.09.20 u=] → [c= p= s= u=]
(Assignee)

Updated

5 years ago
Status: ASSIGNED → NEW
status-b2g-v1.2: --- → affected
Hardware: x86 → x86_64
(Assignee)

Updated

5 years ago
Duplicate of this bug: 788834
(Assignee)

Updated

5 years ago
Whiteboard: [c= p= s= u=] → [c= p=1 s= u=]
Target Milestone: --- → 1.2 C3(Oct25)

Comment 28

5 years ago
Comment on attachment 802810 [details] [diff] [review]
gonk-patches PR

Hey Diego, would it be possible to land this patch on b2g_ics_1.2 in the frameworks/base repo? This patch actually adds other patches, but the actual change is pretty simple.
Attachment #802810 - Flags: feedback?(dwilson)

Updated

5 years ago
Status: NEW → ASSIGNED
(In reply to Michael Wu [:mwu] from comment #28)
> Comment on attachment 802810 [details] [diff] [review]
> gonk-patches PR
> 
> Hey Diego, would it be possible to land this patch on b2g_ics_1.2 in the
> frameworks/base repo? This patch actually adds other patches, but the actual
> change is pretty simple.

The patch looks good to me except we are still using gcc 4.4, so we'll need to use a version macro instead. I'll see what I can do.
So why do you care about gcc 4.7?
Flags: needinfo?(mwu)
(Assignee)

Comment 31

5 years ago
> The patch looks good to me except we are still using gcc 4.4, so we'll need
> to use a version macro instead. I'll see what I can do.

As far as I know it will work on 4.4 as well. The syntax is completely valid, it was not prior to the patch ; the special casing only for clang was incorrect.
(Assignee)

Comment 32

5 years ago
(In reply to Diego Wilson [:diego] from comment #30)
> So why do you care about gcc 4.7?

Because we do. The old code was invalid C++ that older compiler accepted. 4.7 is default on most recent Linux distro.

Comment 33

5 years ago
(In reply to Diego Wilson [:diego] from comment #30)
> So why do you care about gcc 4.7?

I don't, but this makes things easier for people trying to build with newer host toolchains. It should be a generally correct change AFAICT.
The patch works for gcc 4.4 too. I'll update once it lands on CAF.
(Assignee)

Comment 36

5 years ago
Comment on attachment 802811 [details] [diff] [review]
b2g-manifest PR

This change is no longer needed as the patches have been put upstream as per previous comment.
Attachment #802811 - Attachment is obsolete: true
Attachment #802811 - Flags: review?(mwu)
(Assignee)

Comment 37

5 years ago
(In reply to Diego Wilson [:diego] from comment #35)
> This fix landed on platform/frameworks/base[b2g_ics_1.2] in CAF
> 
> https://www.codeaurora.org/cgit/quic/la/platform/frameworks/base/commit/
> ?h=b2g_ics_1.2&id=89820fe3eb90807bb1d7f26519b3af04e6f633fe

Thanks !!
(Assignee)

Comment 38

5 years ago
Created attachment 819929 [details] [review]
Fix Android SDK (emulator) build on recent gcc.

proper PR attachment
Attachment #802813 - Attachment is obsolete: true
Attachment #802813 - Flags: review?(mwu)

Updated

5 years ago
Flags: needinfo?(mwu)

Updated

5 years ago
Attachment #802810 - Flags: feedback?(dwilson)
(Assignee)

Updated

5 years ago
Attachment #802810 - Flags: review?(mwu)
(Assignee)

Updated

5 years ago
Attachment #819929 - Flags: review?(mwu)
(Assignee)

Comment 39

5 years ago
What about that last patch (comment 38)? who own our clone of the android-sdk repository?

Thanks
Flags: needinfo?(mwu)

Updated

5 years ago
Attachment #819929 - Flags: review?(mwu) → review+
(Assignee)

Comment 40

5 years ago
Has been fixed now. I don't think there is anything left.

Thanks !!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(mwu)
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: 1.2 C3(Oct25) → ---
I encountered this compilation error with gcc/g++ 4.8.1. Currently I just downgraded my gcc/g++ to 4.6 to build b2g. Does this issue rise again on gcc 4.8?
(Reporter)

Comment 42

5 years ago
can you share the error messages?
(In reply to Julien Wajsberg [:julienw] from comment #42)
> can you share the error messages?
I think it's just the same message as the initial comment in this bug.
My error message:
frameworks/base/include/utils/KeyedVector.h:196:31: error: ‘indexOfKey’ was not declared in this scope, and no declarations were found by argument-dependent lookup at the point of instantiation [-fpermissive]

OS: Ubuntu 13.10
gcc: gcc version 4.8.1 (Ubuntu/Linaro 4.8.1-10ubuntu8)
(Reporter)

Comment 44

5 years ago
I don't know what the fix was in the end, maybe it's only for some device targets or some Firefox OS versions.

Hubert, could you please clarify?
Flags: needinfo?(hub)
(Assignee)

Comment 45

5 years ago
The fix the framework was pushed to the codeaurora repositories. No need for an after the fact patch (*hint* they have fixed because clang also fails on it... but only for clang).

Not sure which devices were impacted, but inari and hamachi definitely.

The other fix is for the emulator and our mirror has the patch now (we use a branch).

I think it has always been fixed in the emulator-jb
(Assignee)

Updated

5 years ago
Flags: needinfo?(hub)
You need to log in before you can comment on or make changes to this bug.