[EWS] Report basic statuses to GitHub#72
Conversation
There was a problem hiding this comment.
is this method name intentional? (same as buildbot method name)
There was a problem hiding this comment.
Yes, although I can't say I gave much thought to the name. It could be anything that gets the idea of extracting a property from the build dictionary across.
There was a problem hiding this comment.
same name might cause confusion or might be misleading. We can call it something like extractProperty. I guess then we can also rename property_ to property.
|
Just proto-typing EWS comment format (which isn't something this pull-request implements): EWS Result summary✅ 🧪 style ⏳ 🧪 webkitpy ✅ 🛠 ios ✅ 🛠 ios-sim ✅ 🛠 mac Details❌ 🛠🧪 winFound 1 new test failure ❌ 🧪 mac-wk1Found 5 new test failures ❌ 🧪 mac-wk2Found 5 new test failures ❌ 🧪 mac-debug-wk1Found 5 new test failures ❌ 🧪 mac-AS-debug-wk2Found 5 new test failures |
|
Test editing a comment one more time! |
f67f0bd to
e4729fc
Compare
There was a problem hiding this comment.
Probably better to construct all the urls at a single place, inside the GitHub class.
There was a problem hiding this comment.
How exactly would we do that though? We have at least two different URL paths that we're using at the moment, there will likely be 3 or 4 by the time we're done with GitHub stuff in EWS. I suppose we could pass a "path" argument to the api_url function, that would look like this:
d = agent.request(b'POST', GitHub.api_url(repository, 'statuses/{}'.format(sha)), sha).encode('utf-8'))...
which doesn't seem any better, possibly more confusing because you would need to consult the function to see what the extra argument is doing.
There was a problem hiding this comment.
Nit: Since this is pretty long, it might be a good idea to store in a variable for better readability.
There was a problem hiding this comment.
same name might cause confusion or might be misleading. We can call it something like extractProperty. I guess then we can also rename property_ to property.
There was a problem hiding this comment.
For all these events, it might be better/cleaner to create two methods one for ews and one for github. Maybe something like: sendBuildStartedDataToEWS, sendBuildStartedDataToGitHub(), and have buildStarted() call them appropriately.
There was a problem hiding this comment.
Does that apply only to BuildStarted or to all of them? I was trying to avoid a ton of function bloat in this class, if we did this everywhere, we'd be changing 4 functions to into 12.
There was a problem hiding this comment.
Actually, I just removed the code in BuildStarted completely. The reason I did this is that we really don't want to report anything until we validate the patch is actually relevant, I haven't made that change in it's entirety yet, but given we're reporting on each intermediate step, it shouldn't be terribly difficult to do.
|
@JonWBedard have you looked into buildbot implementation GitHubStatusPush http://docs.buildbot.net/2.8.3/manual/configuration/reporters.html#githubstatuspush If that works well, it might be better to use that instead of maintaining our own code, for something which has already been developed by buildbot and hopefully polished over a period of time. Similarly, there is also GitHubCommentPush which might be helpful with commenting on GitHub. |
I did look into this, and I think we want the finer control of handling all this ourselves. As an example of something we're probably going to want to do: we probably don't want to report to GitHub that a build has started until we've passed the validation phase, otherwise we'll end up reporting "success" for builds that actually didn't run. Our comments are almost certainly going to be coming from ews.webkit.org, not ewe-build.webkit.org because one comment will contain details for multiple queues. That being said, if we want to make explicit comments on test failures, yes, we should use GitHubCommentPush |
e4729fc to
e1a7379
Compare
43440b1 to
caa2937
Compare
aj062
left a comment
There was a problem hiding this comment.
As discussed, this change is temporary for quick prototyping. Since we want to generate comments from ews django app, eventually ews-app would be the one sending data to github, not buildbot. Buildbot will send the data (for both patch and pr work-flow) to ews-app.
So we would want to revert/remove this change when we implement that communication with ews-app. So, can you please split this patch in two parts/commits, first one which would be permanent change, and second one which could be reverted easily, and then land as two separate commits.
There was a problem hiding this comment.
Might be a good idea to print something to logs here, in case we have to debug a scenario when there is a problem with github communication.
There was a problem hiding this comment.
Are we using 'github.number' as the main identifier to decide whether it is a PR or a patch based build? Whatever we are using, let's make sure to use the same identifier through-out the ews code.
There was a problem hiding this comment.
Yes, that's what I've been keying off of. I often check more that just github.number, but I try to make that the primary check.
There was a problem hiding this comment.
Do we even need this check? We didn't had it before. If we don't have it, it will try to send data to ews django app, and django app would reject it. I guess that might log an error in logs (i haven't checked though).
There was a problem hiding this comment.
We also didn't have tons of builds without patch_id before, though. Which is why I think we should have keep this check.
There was a problem hiding this comment.
That's extra database call, but seems like we can't avoid that.
caa2937 to
2975d2c
Compare
2975d2c to
76b8712
Compare
https://bugs.webkit.org/show_bug.cgi?id=241905 Reviewed by Yusuke Suzuki. offlineasm used to emit this LLInt code: ".loc 1 996\n" "ldr x19, [x0] \n" // LowLevelInterpreter.asm:996 ".loc 1 997\n" "ldr x20, [x0, WebKit#8] \n" // LowLevelInterpreter.asm:997 ".loc 1 998\n" "ldr x21, [x0, WebKit#16] \n" // LowLevelInterpreter.asm:998 ".loc 1 999\n" "ldr x22, [x0, WebKit#24] \n" // LowLevelInterpreter.asm:999 ... ".loc 1 1006\n" "ldr d8, [x0, WebKit#80] \n" // LowLevelInterpreter.asm:1006 ".loc 1 1007\n" "ldr d9, [x0, WebKit#88] \n" // LowLevelInterpreter.asm:1007 ".loc 1 1008\n" "ldr d10, [x0, WebKit#96] \n" // LowLevelInterpreter.asm:1008 ".loc 1 1009\n" "ldr d11, [x0, WebKit#104] \n" // LowLevelInterpreter.asm:1009 ... Now, it can emit this instead: ".loc 1 996\n" "ldp x19, x20, [x0, #0] \n" // LowLevelInterpreter.asm:996 ".loc 1 997\n" "ldp x21, x22, [x0, WebKit#16] \n" // LowLevelInterpreter.asm:997 ... ".loc 1 1001\n" "ldp d8, d9, [x0, WebKit#80] \n" // LowLevelInterpreter.asm:1001 ".loc 1 1002\n" "ldp d10, d11, [x0, WebKit#96] \n" // LowLevelInterpreter.asm:1002 ... Also, there was some code that kept recomputing the base address of a sequence of load/store instructions. For example, ".loc 6 902\n" "add x13, sp, x10, lsl WebKit#3 \n" // WebAssembly.asm:902 "ldr x0, [x13, WebKit#48] \n" "add x13, sp, x10, lsl WebKit#3 \n" "ldr x1, [x13, WebKit#56] \n" "add x13, sp, x10, lsl WebKit#3 \n" "ldr x2, [x13, WebKit#64] \n" "add x13, sp, x10, lsl WebKit#3 \n" "ldr x3, [x13, WebKit#72] \n" ... For such places, we observe that the base address is the same for every load/store instruction in the sequence, and precompute it in the LLInt asm code to help out the offline asm. This allows the offlineasm to now emit this more efficient code instead: ".loc 6 896\n" "add x10, sp, x10, lsl WebKit#3 \n" // WebAssembly.asm:896 ".loc 6 898\n" "ldp x0, x1, [x10, WebKit#48] \n" // WebAssembly.asm:898 "ldp x2, x3, [x10, WebKit#64] \n" ... * Source/JavaScriptCore/llint/LowLevelInterpreter.asm: * Source/JavaScriptCore/llint/WebAssembly.asm: * Source/JavaScriptCore/offlineasm/arm64.rb: * Source/JavaScriptCore/offlineasm/instructions.rb: Canonical link: https://commits.webkit.org/251799@main
The legacy App Store (which links our installed WebKit2) crashed ~10s
after launch and, had it rendered, showed raw MZStore XML. Both fixed in
WKPage.cpp:
1. Off-main crash: the App Store calls WKPageLoadURL from its own
background GCD queue ("WebView Initial Load Queue"), reaching
WebProcessPool::pageEndUsingWebsiteDataStore where
RELEASE_ASSERT(RunLoop::isMain()) correctly fires. New
runLoadOnMainRunLoop helper marshals the WKPageLoadURL load family
onto the main RunLoop when called off-main (synchronous passthrough
when already on main, so Safari/QuickLook are unchanged), capturing
page/userData by Ref. The work now runs on main; the assert holds
legitimately (NOT weakened).
2. Raw XML: narrowed the backport's V0 decidePolicyForResponse bypass
with && !frame.isMainFrame(), so the App Store's main-frame V0 policy
client again intercepts the text/xml MZStore plist instead of WebKit
auto-allowing it and rendering it raw. Upstream has no such bypass;
this shrinks the deviation back toward upstream. No download
regression (the bypass only ever fired for canShowMIMEType==true; real
downloads are canShowMIMEType==false and already reached the client).
Verified: App Store renders the live Featured storefront, stable 7+ min;
Safari unregressed. Hack-checked clean. Follow-up debt: the remaining
subframe V0 bypass is still non-upstream (#60).
Co-Authored-By: Claude Opus 4.8 <[email protected]>
The backport installed a global NSEvent monitor (+[WKView installEventMonitorOnce]) that intercepted mouse/scroll/key events and dispatched them to WKView, on the belief that layer-tree mirror subviews intercepted hit-testing. That was a MISDIAGNOSIS: WKView hosts the WebContent layer tree as a plain CALayer sublayer of its backing layer (MinimalPageClient enterAcceleratedCompositingMode/setRemoteLayerTreeRootNode, [[m_view layer] addSublayer:]) with ZERO NSView subviews. CALayers don't participate in NSView -hitTest:, so nothing ever intercepted events. Worse, the monitor consumed events before -[NSWindow sendEvent:] (where AppKit activates the window on click), creating the very click-to-activate problem it then re-patched. Remove the monitor and rely on AppKit's natural responder chain; WKView's own scrollWheel:/keyDown:/mouseDown:/mouseMoved: handlers receive events directly. Keep the -hitTest: override as upstream-equivalent insurance (upstream WebViewImpl::hitTest does the same redirect). Verified live: main-view links/scroll/typing/right-click; hover (#17) fires :hover on a fresh page AND with the address bar focused; Reader scroll (#39) works via natural routing to the topmost ReaderWKView; and every secondary WKView that relied on the monitor -- uBlock popover (#14/#23), Web Inspector (#16), App Store (#72), Top Sites (#13), Preferences>Extensions (#24) -- stays interactive. No crashes. Retires the #62 event-monitor keystone and supersedes #39's monitor routing. (The DDG-logo black box, a separate #56 IOSurface-backing symptom, is root-caused but not fixed here.) Co-Authored-By: Claude Opus 4.8 <[email protected]>
76b8712