59 bytes, text/x-review-board-request
1.26 KB, patch
|Details | Diff | Splinter Review|
Bug 1458161 added a rust OOM handler based on an unstable API that was removed in 1.27, replaced with something that didn't allow to get the failed allocation size. Latest 1.28 nightly (2018-05-31) has https://github.com/rust-lang/rust/pull/50880 merged, which allows to hook the OOM handler and get the failed allocation size again.
Comment on attachment 8982149 [details] Bug 1465709 - Hook rust OOM handler on rustc 1.28. https://reviewboard.mozilla.org/r/248172/#review254416 Seems a bit odd for the API to change that much?
Attachment #8982149 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3) > Comment on attachment 8982149 [details] > Bug 1465709 - Hook rust OOM handler on rustc 1.28. > > https://reviewboard.mozilla.org/r/248172/#review254416 > > Seems a bit odd for the API to change that much? OOM handling was moved off GlobalAlloc before its stabilization.
I'm going to hold on before landing this because the oom api might still change in 1.28.
Still holding on to it because the api is going to move behind its own feature().
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/5182bca90d06 Hook rust OOM handler on rustc 1.28. r=froydnj
I was looking at this patch just now because it broke SearchFox for some reason (presumably rust needs to be updated in SearchFox, though I don't know why it would break a week later), and it looks to me like part of this patch might be wrong? +elif not CONFIG['MOZ_AUTOMATION']: + # We don't want builds on automation to unwillingly stop annotating OOM + # crash reports from rust. + error('Builds on automation must use a version of rust that supports OOM ' + 'hooking') Isn't the "not" erroneous? It sounds like you want this to produce an error when MOZ_AUTOMATION is set. Maybe I'm just confused. (Also, I feel like "unwillingly" should be "unwittingly"...)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/d81bb2c26824 Fixup moz.build error logic from bug 1465709. r=me
(In reply to Andrew McCreight [:mccr8] from comment #11) > Isn't the "not" erroneous? It is. Thanks.
Approval Request Comment [Feature/Bug causing the regression]: Regression from this bug [User impact if declined]: Unintended build failure with rustc 1.27 or >= 1.29 for downstreams. [Is this code covered by automated tests?]: N/A [Has the fix been verified in Nightly?]: N/A [Needs manual test from QE? If yes, steps to reproduce]: N/A [List of other uplifts needed for the feature/fix]: bug 1469766 [Is the change risky?]: No [Why is the change risky/not risky?]: It's a moz.build change only, that changes the condition of a configure error. [String changes made/needed]: N/A
Attachment #8986600 - Flags: approval-mozilla-beta?
Comment on attachment 8986600 [details] [diff] [review] Fixup moz.build error logic, landed as d81bb2c26824 IIUC, this bug only affects 62, which is still being kept in sync between m-c and m-b until the version bump on Monday. So no need to uplift :)
Attachment #8986600 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.