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 SpiderMonkey to m-c bcf4ff0c3eef. #12255

Merged
merged 1 commit into from Jul 28, 2016

Conversation

Projects
None yet
8 participants
@Ms2ger
Contributor

Ms2ger commented Jul 5, 2016

This currently breaks Servo on Android, because there are a number of interdependent changes that cannot easily land serially in a way that keeps it working throughout. We expect to fix this in the near future.

This change is Reviewable

@highfive

This comment has been minimized.

highfive commented Jul 5, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/bindings/callback.rs, components/script/dom/bindings/interface.rs, components/script/dom/browsingcontext.rs, components/script/Cargo.toml, components/script/dom/bindings/error.rs, components/script/dom/bindings/proxyhandler.rs
@highfive

This comment has been minimized.

highfive commented Jul 5, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 5, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 5, 2016

⌛️ Trying commit 0eb1b06 with merge 90bb904...

bors-servo added a commit that referenced this pull request Jul 5, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 5, 2016

💔 Test failed - arm64

@nox

This comment has been minimized.

Member

nox commented Jul 5, 2016

-S-awaiting-review


Reviewed 3 of 3 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


components/script/dom/bindings/callback.rs, line 198 [r2] (raw file):

            unsafe {
                let _ac = JSAutoCompartment::new(self.cx, *self.exception_compartment);
                // XXX JS_ReportPendingException(self.cx);

Why?


components/script/dom/bindings/error.rs, line 127 [r2] (raw file):

    if JS_IsExceptionPending(cx) {
        let _ac = JSAutoCompartment::new(cx, obj);
        // XXX JS_ReportPendingException(cx);

Why?


components/script/dom/bindings/interface.rs, line 154 [r2] (raw file):

    /// Create a new `NonCallbackInterfaceObjectClass` structure.
    pub const fn new(ops: &'static ClassOps,

If you make InterfaceConstructorBehavior wrap a ClassOps instead, the only needed change is ops: &'static InterfaceConstructorBehavior.


components/script/dom/bindings/interface.rs, line 190 [r2] (raw file):

    call: JSNative,
    construct: JSNative,
}

Could you instead just make that wrap a ClassOps?


Comments from Reviewable

@nox nox removed the S-awaiting-review label Jul 5, 2016

@nox nox assigned nox and unassigned asajeffrey Jul 5, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 7, 2016

☔️ The latest upstream changes (presumably #12272) made this pull request unmergeable. Please resolve the merge conflicts.

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 12, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 12, 2016

⌛️ Trying commit eb3c72f with merge 90b3b09...

bors-servo added a commit that referenced this pull request Jul 12, 2016

Auto merge of #12255 - servo:smup, r=<try>
Smup

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12255)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 12, 2016

💔 Test failed - windows

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 12, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 12, 2016

⌛️ Trying commit e0da4d0 with merge 2b96de4...

bors-servo added a commit that referenced this pull request Jul 12, 2016

Auto merge of #12255 - servo:smup, r=<try>
Smup

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12255)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 12, 2016

💔 Test failed - windows

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 13, 2016

☔️ The latest upstream changes (presumably #12438) made this pull request unmergeable. Please resolve the merge conflicts.

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 22, 2016

@froydnj I just spent the evening trying to reproduce the configure scripts reproduction of:

  1. translating target triples to google-android-target triples to find gcc
  2. similar hackery to get a valid --sysroot
    And I think I have that working.

However, the libc++_static.a is in, e.g., ./sources/cxx-stl/llvm-libc++/libs/armeabi/libc++_static.a, and it's not at all clear to me how to get that on the library include path. Is there some special magic I should be doing here?

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 22, 2016

That's $cxx_libs in android.m4; not sure how to get that out of there. (Note that changing android.m4 requires re-running autoconf2.13 on old-configure.in.)

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 22, 2016

@Ms2ger You're right! I probably need to reproduce the behavior in:
https://dxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4?q=android.m4&redirect_type=direct#139-161

Kinda annoying, as I think this means that I need to duplicate it in fake-ld.sh (to construct the linker line) and several separate build.rs files. Maybe I should do the detection / env var setting in the python code in mach, so that it's at least only in one place? I'm not sure there's a good solution, with various bits of build hackery scattered across a handful of build systems.

I'll probably go with the "bash my head against it until it works" solution to get the SMUP landed, but figuring out a better way to deal with Android NDK detection would be nice in general. It's already in a pretty... fragile... state.

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 22, 2016

In good news, I've got a build that now links against c++_static and properly resolves those symbols.

In bad news, since many of our other dependencies still link against stlport, we now have ~90 undefined symbols of the form _ZNSt9basic_iosIcSt11char_traitsIcEE4initEPSt15basic_streambufIcS1_E.

Hopefully, this is just a matter of rebuilding the whole tree with magically repointed include directories. WHAT COULD POSSIBLY GO WRONG? :-)

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 24, 2016

@froydnj Have you ever seen the following error (about a bunch of missing really basic C symbols)? I've got most of our native deps compiling w/ the LLVM libc++ and no standalone NDK (freetype, SM, azure, skia, openssl), but angle is still giving me some fits:

error: failed to run custom build command for `angle v0.1.0 (https://github.com/servo/angle?branch=servo#d0a2db05)`
Process didn't exit successfully: `/Users/larsberg/servo/target/debug/build/angle-7062634bbf237306/build-script-build` (exit code: 101)
--- stdout
arm-linux-androideabi-g++ src/glslang-c.cpp -o /Users/larsberg/servo/target/arm-linux-androideabi/debug/build/angle-7062634bbf237306/out/src/glslang-c.o -c --sysroot /Users/larsberg/android-ndk-r12b/platforms/android-18/arch-arm -I/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include -Wall -std=gnu++11 -DANGLE_TRANSLATOR_STATIC -DANGLE_ENABLE_GLSL -DLIBGLESV2_EXPORT_H_ -DANGLE_EXPORT="" -Iinclude -Isrc -O3 -Wall -DCOMPONENT_BUILD  -fPIC

--- stderr
In file included from /Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:107:0,
                 from /Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/string:438,
                 from include/GLSLANG/ShaderLang.h:31,
                 from src/glslang-c.cpp:1:
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwctype:88:9: error: '::iswblank' has not been declared
 using ::iswblank;
         ^
In file included from /Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/string:438:0,
                 from include/GLSLANG/ShaderLang.h:31,
                 from src/glslang-c.cpp:1:
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:132:9: error: '::vfwscanf' has not been declared
 using ::vfwscanf;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:133:9: error: '::vswscanf' has not been declared
 using ::vswscanf;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:134:9: error: '::vwscanf' has not been declared
 using ::vwscanf;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:150:9: error: '::wcstof' has not been declared
 using ::wcstof;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:151:9: error: '::wcstold' has not been declared
 using ::wcstold;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:155:9: error: '::wcstoll' has not been declared
 using ::wcstoll;
         ^
/Users/larsberg/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/cwchar:159:9: error: '::wcstoull' has not been declared
 using ::wcstoull;
         ^
make: *** [/Users/larsberg/servo/target/arm-linux-androideabi/debug/build/angle-7062634bbf237306/out/src/glslang-c.o] Error 1
thread 'main' panicked at 'assertion failed: Command::new("make").args(&["-R", "-f", "makefile.cargo",
                            &format!("-j{}" , env :: var ( "NUM_JOBS" ) .
                                     unwrap (
                                     ))]).status().unwrap().success()', /Users/larsberg/servo/.cargo/git/checkouts/angle-017278ddf8240375/servo/build.rs:10
note: Run with `RUST_BACKTRACE=1` for a backtrace.

[Warning] Could not generate notification! Optional Python module 'pyobjc' is not installed.
Build completed in 0:03:55
@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 27, 2016

The plan for now is to disable Android (servo/saltfs#450), get this landed, and come back to it after the msvc support has also landed.

@nox @Ms2ger Please finish reviews and squash this for landing, after I get the salt change approved and deployed.

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 27, 2016

OK, I'll wrap it up tomorrow.

@larsbergstrom

This comment has been minimized.

Contributor

larsbergstrom commented Jul 28, 2016

servo/saltfs#450 has landed and been rolled out, so you should be good to go here. Thanks!

Update SpiderMonkey to m-c bcf4ff0c3eef.
This currently breaks Servo on Android, because there are a number of
interdependent changes that cannot easily land serially in a way that
keeps it working throughout. We expect to fix this in the near future.

@Ms2ger Ms2ger force-pushed the smup branch from ba35c14 to 89efccc Jul 28, 2016

@Ms2ger Ms2ger changed the title from Smup to Update SpiderMonkey to m-c bcf4ff0c3eef. Jul 28, 2016

@Ms2ger

This comment has been minimized.

Contributor

Ms2ger commented Jul 28, 2016

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved, some commit checks failed.


components/script/dom/bindings/error.rs, line 127 [r2] (raw file):

Previously, nox (Anthony Ramine) wrote…

Why?

Fixed

components/script/dom/bindings/interface.rs, line 154 [r2] (raw file):

Previously, nox (Anthony Ramine) wrote…

If you make InterfaceConstructorBehavior wrap a ClassOps instead, the only needed change is ops: &'static InterfaceConstructorBehavior.

Not done, because it caused a segfault. Might be worth trying to fix it after this lands.

Comments from Reviewable

@jdm

This comment has been minimized.

Member

jdm commented Jul 28, 2016

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

📌 Commit 89efccc has been approved by jdm

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

⌛️ Testing commit 89efccc with merge 5ae1fcd...

bors-servo added a commit that referenced this pull request Jul 28, 2016

Auto merge of #12255 - servo:smup, r=jdm
Update SpiderMonkey to m-c bcf4ff0c3eef.

This currently breaks Servo on Android, because there are a number of interdependent changes that cannot easily land serially in a way that keeps it working throughout. We expect to fix this in the near future.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12255)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Jul 28, 2016

@bors-servo bors-servo merged commit 89efccc into master Jul 28, 2016

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
dependency-ci Failed dependency checks
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@froydnj

This comment has been minimized.

Contributor

froydnj commented Jul 28, 2016

@larsbergstrom Sorry for not responding sooner! Gmail failed to highlight your message as important. 😞

This is what Gecko does for -I and -l flags:

http://dxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#170

It looks like your command line got truncated somehow, so I can't tell if you have all those -I flags, but my guess is that you're missing the -I$NDK/android/support/include bit somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment