Remove now unused _RAISE macro check to deal with MSVC 2017 15.8 (also affects clang-cl builds)

RESOLVED FIXED in Firefox -esr60

Status

enhancement
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 months ago
15.8 moved _RAISE definitions to <yvals.h>.
(Assignee)

Comment 1

9 months ago
Attachment #9001940 - Flags: review?(core-build-config-reviews)
(Assignee)

Updated

9 months ago
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(Assignee)

Updated

9 months ago
Blocks: 1484190
Hmmm, this affects clang build as well because it's the SDK change.
Also I can confirm that this patch fixes that problem.
(Assignee)

Comment 4

9 months ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> Hmmm, this affects clang build as well because it's the SDK change.

More precisely, this is a CRT change. Although CRT headers do not belong to SDK, clang-cl depends on CRT headers and libraries.
(Assignee)

Updated

9 months ago
Summary: Fix _RAISE macro check with MSVC 2017 15.8 → Fix _RAISE macro check with MSVC 2017 15.8 (also affects clang-cl builds)
Actually, I'm wondering do we still need this check at all.

Grepping the whole codebase, I don't see we override _RAISE at all. Maybe this is just some out-of-date stuff we didn't clean up?
(Assignee)

Comment 6

9 months ago
Comment on attachment 9001940 [details] [diff] [review]
Fix _RAISE macro check with MSVC 2017 15.8

Oh, you are right. Bug 1376057 removed the use of _RAISE. I will update the patch.
Attachment #9001940 - Flags: review?(core-build-config-reviews)
(Assignee)

Updated

9 months ago
Summary: Fix _RAISE macro check with MSVC 2017 15.8 (also affects clang-cl builds) → Remove now unused _RAISE macro check to deal with with MSVC 2017 15.8 (also affects clang-cl builds)
(Assignee)

Updated

9 months ago
Summary: Remove now unused _RAISE macro check to deal with with MSVC 2017 15.8 (also affects clang-cl builds) → Remove now unused _RAISE macro check to deal with MSVC 2017 15.8 (also affects clang-cl builds)
(Assignee)

Comment 7

9 months ago
Attachment #9002421 - Flags: review?(core-build-config-reviews)
(Assignee)

Updated

9 months ago
Attachment #9001940 - Attachment is obsolete: true
Comment on attachment 9002421 [details] [diff] [review]
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8

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

This is great, I had hacky patches to workaround this for AArch64, and it's nice to know that I don't need to polish them.  Thank you!
Attachment #9002421 - Flags: review?(core-build-config-reviews) → review+
Depends on: 1376057

Comment 9

9 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef81d07bc5b
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8. r=froydnj
(Assignee)

Comment 11

9 months ago
This test failure is bug 1484190. I'll re-land this patch.
Flags: needinfo?(VYV03354)

Comment 12

9 months ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66cc808862f
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8. r=froydnj

Comment 13

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c66cc808862f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 9002421 [details] [diff] [review]
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Current tooling used to build central may fail to build esr60 on Windows using either msvc or clang. This is a hindrance to devs, who either need to rollback their tooling, or manually apply the fixes from bug 1484184 (this bug), and in the case of clang also bug 1462616 -- which I will request uplift of separately. Uplifting these patches smooths development of esr60 on Windows, and avoids devs having to manually discover these fixes and apply them.

User impact if declined: End users are not impacted. Devs will not be able build esr60 without finding and applying the above patches.

Fix Landed on Version: 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The change is small and to the build system. The change has baked since 63. I have verified that esr60 Windows builds work with clang following the application of this and the other mentioned patch.

String or UUID changes made by this patch: None.
Attachment #9002421 - Flags: approval-mozilla-esr60?
Comment on attachment 9002421 [details] [diff] [review]
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8

Makes life easier for devs compiling ESR60 with newer MSVC toolchains. Approved for 60.5.0esr.
Attachment #9002421 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.