Closed Bug 1160897 Opened 9 years ago Closed 9 years ago

Fixing .ycm_extra_conf.py for Fennec

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: danilo.eu, Unassigned)

Details

(Whiteboard: [npotb])

Attachments

(1 file, 1 obsolete file)

For some weird reason beyond my knowledge, clang fails to parse the file if we send the -march=armv7-a flag to YCM. There's no YcmDiags when working with Fennec.

Removing this flag brings YcmDiags again. It doesn't fix all the fennec warnings (related to firefox stl port), but it makes things like JumpToDefinition, GetParent, GetType to work again.
Attached patch fixing_ycm_for_fennec.patch (obsolete) — Splinter Review
Attachment #8600718 - Flags: review?(ehsan)
It seems like you're wallpapering over a bug in ycmd.  I had to fix one for example: <https://github.com/Valloric/ycmd/pull/118>.  You may want to file a bug with ycmd about this and ideally fix it there and then remove this workaround.
Comment on attachment 8600718 [details] [diff] [review]
fixing_ycm_for_fennec.patch

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

::: .ycm_extra_conf.py
@@ +23,5 @@
>  
> +    flags = shlex.split(out.getvalue())
> +
> +    # This flag is added by Fennec for android build and causes clang to fail to parse the file.
> +    # We can remove the following code when clang starts to handle this flag properly.

Nit: please replace clang with ycmd in the above comment, and also link to the issue that you will file with ycmd.

@@ +25,5 @@
> +
> +    # This flag is added by Fennec for android build and causes clang to fail to parse the file.
> +    # We can remove the following code when clang starts to handle this flag properly.
> +    if '-march=armv7-a' in flags:
> +        flags.remove('-march=armv7-a')

You should probably do the same for armv6 and armv8-a as well.
Attachment #8600718 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> Comment on attachment 8600718 [details] [diff] [review]
> fixing_ycm_for_fennec.patch
> 
> Review of attachment 8600718 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: .ycm_extra_conf.py
> @@ +23,5 @@
> >  
> > +    flags = shlex.split(out.getvalue())
> > +
> > +    # This flag is added by Fennec for android build and causes clang to fail to parse the file.
> > +    # We can remove the following code when clang starts to handle this flag properly.
> 
> Nit: please replace clang with ycmd in the above comment, and also link to
> the issue that you will file with ycmd.

Done.

> 
> @@ +25,5 @@
> > +
> > +    # This flag is added by Fennec for android build and causes clang to fail to parse the file.
> > +    # We can remove the following code when clang starts to handle this flag properly.
> > +    if '-march=armv7-a' in flags:
> > +        flags.remove('-march=armv7-a')
> 
> You should probably do the same for armv6 and armv8-a as well.
I didn't test it for all supported platforms, but I think we can blacklisting all -march=arm*. Does it make sense?
r? again as I changed the approach.
Attachment #8600718 - Attachment is obsolete: true
Attachment #8600898 - Flags: review?(ehsan)
Comment on attachment 8600898 [details] [diff] [review]
fixing_ycm_for_fennec.patch

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

Looks great, thanks!
Attachment #8600898 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
can we get a try run for this changes, thanks!
Flags: needinfo?(danilo.eu)
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [npotb]
Flags: needinfo?(danilo.eu)
https://hg.mozilla.org/mozilla-central/rev/5b146d08b2c5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.