1518856 - Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS

Georg notices that on esr60 our -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include in the mozconfig is actually 7 despite using clang 8 - and the compile works fine. I tested and confirm that removing this flag entirely still has a successful build, so it seems like we can drop it entirely?

Besides just general 'less is better'; this also removes a hardcoded version that we were going to have to update manually on clang revs so that is very positive.

Try Run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e01361734aa27e020d6214a16f2d8e8355cc42

Comment 1

[Jacek Caban]

Since it works, it looks good. I recall this being needed for proper include order, I wonder what changed... Maybe updated clang doesn't need those flags at all. I pushed a try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02c122db7cbebcafe5089843a87723f3b452a525

Comment 2

[Jacek Caban]

Comment on attachment 9035372 [details] [diff] [review]
Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS

My try failed, looks like other flags are still needed. The patch looks fine then.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5b64daf4ddd
Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS r=dmajory

Comment 4

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c5b64daf4ddd

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9035372 [details] [diff] [review]
Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a build cleanup for for mingw-clang to keep esr60 and central in sync

User impact if declined: Our trees will be different and can lead to confusion for Tor and Mozilla

Fix Landed on Version: 66.0a1 / 20190112094119

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Only affects mingw-clang. If it breaks anything the build will break.

String or UUID changes made by this patch:

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1518856 - Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS r=dmajor (esr60)

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

FWIW, if the patch grafts cleanly, it's much simpler all around to just request uplift on the initial patch rather than make a separate attachment.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Julien Cristau [:jcristau] from comment #7)

FWIW, if the patch grafts cleanly, it's much simpler all around to just request uplift on the initial patch rather than make a separate attachment.

I do try to; but this one doesn't. =) (esr60 has 7.0.0 in the line; while central has 8.0.0)

Comment 9

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9037354 [details] [diff] [review]
Bug 1518856 - Remove -I$TOOLTOOL_DIR/clang/lib/clang/8.0.0/include from the Stylo BINDGEN_CFLAGS r=dmajor (esr60)

Fair enough :)

I'll do s/7/8/ in the commit message as well.  Approved for 60.5esr.

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d0b436176862