854389 - b2g doesn't build with gcc 4.7

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect, P4)

Description

[Julien Wajsberg [:julienw]]

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

Comment 1

[Justin Lebar (not reading bugmail)]

Anyone know how hard it would be to check in a change to this header?

Comment 2

[Michael Wu [:mwu]]

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.

Comment 3

[Justin Lebar (not reading bugmail)]

(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

[Michael Wu [:mwu]]

(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)

Comment 5

[Justin Lebar (not reading bugmail)]

> 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.

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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?

Comment 7

[Michael Wu [:mwu]]

No patches for fixing this on ICS will be accepted. Warning about gcc versions or automatically using gcc 4.6 is ok.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 9

[Hubert Figuiere [:hub]]

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

Comment 10

[Hubert Figuiere [:hub]]

(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.

Comment 11

[Dave Hylands [:dhylands]]

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.

Comment 12

[Hubert Figuiere [:hub]]

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.

Comment 13

[Julien Wajsberg [:julienw]]

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.

Comment 14

[Hubert Figuiere [:hub]]

(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.

Comment 15

[Justin Lebar (not reading bugmail)]

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.

Comment 16

[Hubert Figuiere [:hub]]

(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.

Comment 17

[Justin Lebar (not reading bugmail)]

> 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.

Comment 18

[Hubert Figuiere [:hub]]

got a fix involving using the vendor patches system. will submit a PR

Comment 19

[Hubert Figuiere [:hub]]

Attachment: 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.

Comment 20

[Hubert Figuiere [:hub]]

Here are the pull requests.

https://github.com/mozilla-b2g/gonk-patches/pull/4
https://github.com/mozilla-b2g/b2g-manifest/pull/100

Comment 21

[Hubert Figuiere [:hub]]

Also a third fix:
https://github.com/mozilla-b2g/android-sdk/pull/5

Comment 22

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gonk-patches/pull/4

Pointer to Github pull-request

Comment 23

[Gabriele Svelto [:gsvelto] (PTO)]

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 :(

Comment 24

[Hubert Figuiere [:hub]]

Attachment: gonk-patches PR

Comment 25

[Hubert Figuiere [:hub]]

Attachment: b2g-manifest PR

Comment 26

[Hubert Figuiere [:hub]]

Attachment: android-sdk PR

Comment 28

[Michael Wu [:mwu]]

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.

Comment 29

[Diego Wilson [:diego]]

(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.

Comment 30

[Diego Wilson [:diego]]

So why do you care about gcc 4.7?

Comment 31

[Hubert Figuiere [:hub]]

> 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.

Comment 32

[Hubert Figuiere [:hub]]

(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

[Michael Wu [:mwu]]

(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.

Comment 34

[Diego Wilson [:diego]]

The patch works for gcc 4.4 too. I'll update once it lands on CAF.

Comment 35

[Diego Wilson [:diego]]

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

Comment 36

[Hubert Figuiere [:hub]]

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.

Comment 37

[Hubert Figuiere [:hub]]

(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 !!

Comment 38

[Hubert Figuiere [:hub]]

Attachment: Fix Android SDK (emulator) build on recent gcc.

proper PR attachment

Comment 39

[Hubert Figuiere [:hub]]

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

Thanks

Comment 40

[Hubert Figuiere [:hub]]

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

Thanks !!

Comment 41

[Ethan Tseng [:ethan]]

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?

Comment 42

[Julien Wajsberg [:julienw]]

can you share the error messages?

Comment 43

[Ethan Tseng [:ethan]]

(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)

Comment 44

[Julien Wajsberg [:julienw]]

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?

Comment 45

[Hubert Figuiere [:hub]]

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