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

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Masatoshi Kimura [:emk]]

15.8 moved _RAISE definitions to <yvals.h>.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Fix _RAISE macro check with MSVC 2017 15.8

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, this affects clang build as well because it's the SDK change.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Also I can confirm that this patch fixes that problem.

Comment 4

[Masatoshi Kimura [:emk]]

(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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 6

[Masatoshi Kimura [:emk]]

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.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Remove now unused _RAISE macro check to deal with MSVC 2017 15.8

Comment 8

[Nathan Froyd [:froydnj]]

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!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ef81d07bc5b
Remove now unused _RAISE macro check to deal with MSVC 2017 15.8. r=froydnj

Comment 10

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on test_toolchain_configure.py

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dca5444170e0a59c7a3a5321bece62d717fb6c1a
Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e949e5a9a62d0db0d33833e1e875f12f2969ac4
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=194880403&repo=mozilla-inbound&lineNumber=41172

Comment 11

[Masatoshi Kimura [:emk]]

This test failure is bug 1484190. I'll re-land this patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

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

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/95bc53e0f0a7