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

RESOLVED FIXED in Firefox -esr60

Status

enhancement
P5
normal
RESOLVED FIXED
4 months ago
2 days ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Blocks 1 bug)

Trunk
mozilla66

Firefox Tracking Flags

(firefox-esr60 fixed, firefox66 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 months ago

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

Attachment #9035372 - Flags: review?(jacek)

Comment 1

3 months ago

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

3 months ago

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.

Attachment #9035372 - Flags: review?(jacek) → review+
(Assignee)

Updated

3 months ago
Attachment #9035372 - Flags: review?(core-build-config-reviews)
Attachment #9035372 - Flags: review?(core-build-config-reviews) → review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 3

3 months ago

Pushed by nerli@mozilla.com:
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

Keywords: checkin-needed

Comment 4

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Comment 5

3 months ago
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:
Attachment #9035372 - Flags: approval-mozilla-esr60?
(Assignee)

Updated

3 months ago
Attachment #9035372 - Flags: approval-mozilla-esr60?

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.

(Assignee)

Comment 8

3 months ago

(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 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.
Attachment #9037354 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.