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)
Firefox Build System
Mach Core
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: danilo.eu, Unassigned)
Details
(Whiteboard: [npotb])
Attachments
(1 file, 1 obsolete file)
1.25 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•9 years ago
|
||
Attachment #8600718 -
Flags: review?(ehsan)
Comment 2•9 years ago
|
||
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 3•9 years ago
|
||
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+
Reporter | ||
Comment 4•9 years ago
|
||
(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?
Reporter | ||
Comment 5•9 years ago
|
||
r? again as I changed the approach.
Attachment #8600718 -
Attachment is obsolete: true
Attachment #8600898 -
Flags: review?(ehsan)
Comment 6•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 7•9 years ago
|
||
can we get a try run for this changes, thanks!
Flags: needinfo?(danilo.eu)
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
Whiteboard: [npotb]
Updated•9 years ago
|
Flags: needinfo?(danilo.eu)
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b146d08b2c5
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b146d08b2c5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•