-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update mozjs (SpiderMonkey) to 128.0 #32769
Conversation
Results are pretty good: https://github.com/sagudev/servo/actions/runs/9911961223/job/27388962921 "Bad" results
Timeouts are on all browsers: https://wpt.fyi/results/webmessaging/message-channels/close-event/garbage-collected.tentative.any.html?label=experimental&label=master&aligned, and before subcase was fail so this is not a regression. RegressionsOK /html/semantics/scripting-1/the-script-element/module/crossorigin.html
I also expected https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers (apparently there are no wpt tests for it? probably because this is in ecme test suite). |
NOTE-TO-SELF: Run debugmozjs in CI: #14759. |
Android still fails: https://github.com/sagudev/servo/actions/runs/9913444136/job/27390478428#step:13:1242, but it works in mozjs, so maybe more changes are needed in mach. cc @mukilan |
🔨 Triggering try run (#9919707596) for Linux WPT |
Test results for linux-wpt-layout-2020 from try job (#9919707596): Flaky unexpected result (11)
Stable unexpected results that are known to be intermittent (19)
|
✨ Try run (#9919707596) succeeded. |
@mukilan are you doing/willing to do android work or should somebody else do it? |
Sorry, missed the notifications from this PR. I can look into the android part. |
We need Android NDK r26c for the SM upgrade (servo#32769) and the fix in nixpkgs to make r26 NDKs function correctly (NixOS/nixpkgs#298285). Both of those are not available in the snapshot of nixpkgs we use currently. Signed-off-by: Mukilan Thiyagarajan <[email protected]>
* build: bump the nixpkgs snapshot We need Android NDK r26c for the SM upgrade (#32769) and the fix in nixpkgs to make r26 NDKs function correctly (NixOS/nixpkgs#298285). Both of those are not available in the snapshot of nixpkgs we use currently. Signed-off-by: Mukilan Thiyagarajan <[email protected]> * add cargo-deny to shell.nix Signed-off-by: Mukilan Thiyagarajan <[email protected]> --------- Signed-off-by: Mukilan Thiyagarajan <[email protected]>
* build: bump the nixpkgs snapshot We need Android NDK r26c for the SM upgrade (#32769) and the fix in nixpkgs to make r26 NDKs function correctly (NixOS/nixpkgs#298285). Both of those are not available in the snapshot of nixpkgs we use currently. Signed-off-by: Mukilan Thiyagarajan <[email protected]> * add cargo-deny to shell.nix Signed-off-by: Mukilan Thiyagarajan <[email protected]> --------- Signed-off-by: Mukilan Thiyagarajan <[email protected]>
@sagudev Apologies, I accidentally pushed to your fork while trying to trigger the CI job on my fork. Shall I force push without my commits? |
It's okay, you can push to my branch/fork as I am not currently doing any work here. |
So 2460979 fixes the Android build on both the CI and my NixOS system. However, I'm not sure if I understand the underlying issue correctly. On NixOS, without the fixes for setting
I set I also needed the Interestingly, the CI fails without the libz fixes, but it failing during the configure step of mozjs (same error that you shared) instead of a linker error.
I'll try to figure out the exact reason, but let me know if you are able to make sense of the above issues. Note: The NDK does have the libz and is part of the system search paths for the linker and header includes. |
This sounds like libz-sys problem, maybe we can isolate it and fix it upstream.
This is very weird, maybe there is some escaping problem or something, could you share whole invocation. Also what is weird is that android CI in mozjs works (although we only run cargo build which does not trigger linker step). |
This is the invocation without the link-lib directive in servoshell/build.rs. When we add the directive, we get an additional |
* build: bump the nixpkgs snapshot We need Android NDK r26c for the SM upgrade (servo#32769) and the fix in nixpkgs to make r26 NDKs function correctly (NixOS/nixpkgs#298285). Both of those are not available in the snapshot of nixpkgs we use currently. Signed-off-by: Mukilan Thiyagarajan <[email protected]> * add cargo-deny to shell.nix Signed-off-by: Mukilan Thiyagarajan <[email protected]> --------- Signed-off-by: Mukilan Thiyagarajan <[email protected]>
I think the issue might in mozjs, specifically in how zlib is included and compiled. I'm able to reproduce the issue I'm seeing with:
I get the same 'undefined hidden symbol' error when trying to link the above with both static and dynamic libz. I don't see any error if the visibilty is changed to 'default' or we use the zlib header directly. Although, it is curious why static linking doesn't work here even though it does in servo (i.e the rust-link-lib=static=z directive). |
Where exactly is this in mozjs? https://github.com/sagudev/mozjs/tree/e618981f24a5105321301b33ab874632661846ed does not have any manual declarations by my search. |
Sorry, I can clarify. I'm not saying we have literally declared them using the
|
Try sagudev/mozjs@ff754c5 of mozjs (main branch on sagudev/mozjs) |
Yes, sagudev/mozjs@ff754c5 does work without the libz related hacks in Servo! Also, I found that SM forces all declarations to be hidden by default, including extern declarations.
Commenting out the gcc_hidden.h include flag in toolchain.configure also makes Servo compile without sagudev/mozjs@ff754c5. With sagudev/mozjs@ff754c5, the mozjs rlib now contains 'default' visibility references to the zlib symbols.
I still can't find documentation that explains the linker's behaviour in this scenario with hidden symbols, so I don't yet understand how it works now with |
Sounds good to me.
Okay, I pushed changes to esr-128 branch (commit hash is different due to rebase), so you can proceed. EDIT: Do not forget to sign-off your commits. |
Signed-off-by: sagudev <[email protected]>
https://bugzilla.mozilla.org/show_bug.cgi?id=1842713 Signed-off-by: sagudev <[email protected]>
Signed-off-by: sagudev <[email protected]>
Android NDK's layout has changed in r26 and 'lib64' no longer exists under `toolchain/llvm/prebuilt/linux-x86_64`. The libraries that used to be it are now present in `lib` folder itself. This patch updates the build configuration to use the `lib` folder instead when configuring the LIBCLANG_PATH environment variable. This patch also updates to a newer mozjs version that includes fixes for linker errors faced on Android (see servo#32769). Signed-off-by: Mukilan Thiyagarajan <[email protected]>
Signed-off-by: sagudev <[email protected]>
Signed-off-by: Mukilan Thiyagarajan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Signed-off-by: sagudev <[email protected]>
Something is wrong with windows artifact: https://github.com/sagudev/servo/actions/runs/10134348491/job/28020479847#step:12:580 cc @wusyong |
@sagudev I tested on my local Windows device, and everything is working. Maybe it's because Servo CI doesn't run on |
After a small research, I think the problem might be in stripping, so I am doing test run for that https://github.com/sagudev/mozjs/actions/runs/10138127855 |
It works: https://github.com/sagudev/servo/actions/runs/10138407681/job/28030063629 |
Signed-off-by: sagudev <[email protected]>
I would like to thank everyone who helped me on this SM bump. |
* Update mozjs Signed-off-by: sagudev <[email protected]> * Fix changed readTransfer callback https://bugzilla.mozilla.org/show_bug.cgi?id=1842713 Signed-off-by: sagudev <[email protected]> * Use NewExternalArrayBuffer from glue servo/mozjs@d33454b Signed-off-by: sagudev <[email protected]> * Fix columnorigin and filename being in latin1 Signed-off-by: sagudev <[email protected]> * fixup newexternalarray Signed-off-by: sagudev <[email protected]> * Float16 (this might require more work for codegen support) https://bugzilla.mozilla.org/show_bug.cgi?id=1833647 Signed-off-by: sagudev <[email protected]> * js.strict is removed https://bugzilla.mozilla.org/show_bug.cgi?id=1621603 Signed-off-by: sagudev <[email protected]> * asm options are now somewhere else https://hg.mozilla.org/mozilla-central/rev/26045c88e3972957087d535e7f259e08857bd2a2 Signed-off-by: sagudev <[email protected]> * Comment out offthread compilation Signed-off-by: sagudev <[email protected]> * Set NDK to 26 Signed-off-by: sagudev <[email protected]> * Fix 1-origin handling Signed-off-by: sagudev <[email protected]> * Expect `FinalizationRegistry` interface Signed-off-by: sagudev <[email protected]> * Good expectations Signed-off-by: sagudev <[email protected]> * more expectations Signed-off-by: sagudev <[email protected]> * Add `WeakRef` to interfaces expectation Signed-off-by: sagudev <[email protected]> * mozjs upgrade: fixes for Android Android NDK's layout has changed in r26 and 'lib64' no longer exists under `toolchain/llvm/prebuilt/linux-x86_64`. The libraries that used to be it are now present in `lib` folder itself. This patch updates the build configuration to use the `lib` folder instead when configuring the LIBCLANG_PATH environment variable. This patch also updates to a newer mozjs version that includes fixes for linker errors faced on Android (see servo#32769). Signed-off-by: Mukilan Thiyagarajan <[email protected]> * Patch libz-sys & update mozjs Signed-off-by: sagudev <[email protected]> * update NDK version in README Signed-off-by: Mukilan Thiyagarajan <[email protected]> * Use servo/mozjs Signed-off-by: sagudev <[email protected]> * Update mozjs again Signed-off-by: sagudev <[email protected]> --------- Signed-off-by: sagudev <[email protected]> Signed-off-by: Mukilan Thiyagarajan <[email protected]> Co-authored-by: Mukilan Thiyagarajan <[email protected]>
Companion PR for servo/mozjs#474
OffThread stuff is commented out.
Fun fact: No changes in codegen needed🎉
./mach build -d
does not report any errors./mach test-tidy
does not report any errors