Recent v8 update (3.19.0 -> 3.19.18.4) breaks rethinkdb #1195

Closed
anatol opened this Issue Jul 21, 2013 · 27 comments

Comments

Projects
None yet
5 participants
@anatol
Contributor

anatol commented Jul 21, 2013

Hi,

I use Linux Arch and it recently updated v8 (3.19.0 -> 3.19.18.4). Before the update rethnkdb 1.7.3 compiled fine, but after the update compilation breaks (see crash below). I do not know whether it is v8 or rethinkdb issue.

    [217/272] CC src/rdb_protocol/proto_utils.cc -o build/release/obj/rdb_protocol/proto_utils.o
    [218/272] CC src/rdb_protocol/pb_server.cc -o build/release/obj/rdb_protocol/pb_server.o
    [219/272] CC src/rdb_protocol/term.cc -o build/release/obj/rdb_protocol/term.o
    [220/272] CC src/rdb_protocol/stream.cc -o build/release/obj/rdb_protocol/stream.o
    [221/272] CC src/rdb_protocol/pb_utils.cc -o build/release/obj/rdb_protocol/pb_utils.o
    [222/272] CC src/rdb_protocol/env.cc -o build/release/obj/rdb_protocol/env.o
    [223/272] CC src/rdb_protocol/transform_visitors.cc -o build/release/obj/rdb_protocol/transform_visitors.o
    [224/272] CC src/rdb_protocol/validate.cc -o build/release/obj/rdb_protocol/validate.o
    [225/272] CC src/rdb_protocol/counted_term.cc -o build/release/obj/rdb_protocol/counted_term.o
    [226/272] CC src/rdb_protocol/js.cc -o build/release/obj/rdb_protocol/js.o
    [227/272] CC src/rdb_protocol/btree.cc -o build/release/obj/rdb_protocol/btree.o
In file included from ./src/rdb_protocol/jsimpl.hpp:9:0,
                 from src/rdb_protocol/js.cc:11:
/usr/include/v8.h: In constructor ‘js::context_t::context_t(js::env_t*)’:
/usr/include/v8.h:750:3: error: ‘v8::Persistent<T>::Persistent(const v8::Persistent<T>&) [with T = v8::Context]’ is private
   V8_INLINE(Persistent(const Persistent& that)) : val_(that.val_) {}
   ^
In file included from src/rdb_protocol/js.cc:11:0:
./src/rdb_protocol/jsimpl.hpp:81:77: error: within this context
     explicit context_t(UNUSED env_t *env) : cx(v8::Context::New()), scope(cx) {}
                                                                             ^
./src/rdb_protocol/jsimpl.hpp:81:77: error: no matching function for call to ‘v8::Context::Scope::Scope(v8::Persistent<v8::Context>&)’
./src/rdb_protocol/jsimpl.hpp:81:77: note: candidates are:
In file included from ./src/rdb_protocol/jsimpl.hpp:9:0,
                 from src/rdb_protocol/js.cc:11:
/usr/include/v8.h:4970:5: note: v8::Context::Scope::Scope(v8::Isolate*, v8::Persistent<v8::Context>&)
     V8_INLINE(Scope(Isolate* isolate, Persistent<Context>& context)) // NOLINT
     ^
/usr/include/v8.h:4970:5: note:   candidate expects 2 arguments, 1 provided
/usr/include/v8.h:4966:14: note: v8::Context::Scope::Scope(v8::Handle<v8::Context>)
     explicit V8_INLINE(Scope(Handle<Context> context)) : context_(context) {
              ^
/usr/include/v8.h:4966:14: note:   no known conversion for argument 1 from ‘v8::Persistent<v8::Context>’ to ‘v8::Handle<v8::Context>’
/usr/include/v8.h:4964:9: note: constexpr v8::Context::Scope::Scope(const v8::Context::Scope&)
   class Scope {
         ^
/usr/include/v8.h:4964:9: note:   no known conversion for argument 1 from ‘v8::Persistent<v8::Context>’ to ‘const v8::Context::Scope&’
src/rdb_protocol/js.cc: In member function ‘js::id_t js::env_t::rememberValue(v8::Handle<v8::Value>)’:
src/rdb_protocol/js.cc:85:75: error: no matching function for call to ‘v8::Persistent<v8::Value>::New(v8::Handle<v8::Value>&)’
     values_.insert(std::make_pair(id, v8::Persistent<v8::Value>::New(value)));
                                                                           ^
src/rdb_protocol/js.cc:85:75: note: candidate is:
In file included from ./src/rdb_protocol/jsimpl.hpp:9:0,
                 from src/rdb_protocol/js.cc:11:
/usr/include/v8.h:5508:4: note: static T* v8::Persistent<T>::New(v8::Isolate*, T*) [with T = v8::Value]
 T* Persistent<T>::New(Isolate* isolate, T* that) {
    ^
/usr/include/v8.h:5508:4: note:   candidate expects 2 arguments, 1 provided
src/rdb_protocol/js.cc: In member function ‘v8::Handle<v8::Value> js::env_t::findValue(js::id_t)’:
src/rdb_protocol/js.cc:92:16: error: could not convert ‘it.std::_Rb_tree_iterator<_Tp>::operator-><std::pair<const unsigned int, v8::Persistent<v8::Value> > >()->std::pair<const unsigned int, v8::Persistent<v8::Value> >::second’ from ‘v8::Persistent<v8::Value>’ to ‘v8::Handle<v8::Value>’
     return it->second;
                ^
src/rdb_protocol/js.cc:93:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors
make[1]: *** [build/release/obj/rdb_protocol/js.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [make] Error 2
@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 22, 2013

Contributor

@anatol -- thanks for reporting this. I believe the V8 API changed across versions. We're either going to have to fetch our own version of v8, or support both APIs in the codebase (for all the distros that didn't upgrade).

Pinging @wmrowan and @AtnNn. What do you guys think we should do about this?

Contributor

coffeemug commented Jul 22, 2013

@anatol -- thanks for reporting this. I believe the V8 API changed across versions. We're either going to have to fetch our own version of v8, or support both APIs in the codebase (for all the distros that didn't upgrade).

Pinging @wmrowan and @AtnNn. What do you guys think we should do about this?

@AtnNn

This comment has been minimized.

Show comment Hide comment
@AtnNn

AtnNn Jul 22, 2013

Owner

I'm not sure at the moment what version of v8 we support.

If you tell configure to --fetch v8 it will use version 3.17.4.1

Owner

AtnNn commented Jul 22, 2013

I'm not sure at the moment what version of v8 we support.

If you tell configure to --fetch v8 it will use version 3.17.4.1

@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 22, 2013

Contributor

Perhaps we should just make it a part of Arch instructions and not use the version of v8 it ships with? @anatol, would that work? (passing --fetch v8 to configure)

Contributor

coffeemug commented Jul 22, 2013

Perhaps we should just make it a part of Arch instructions and not use the version of v8 it ships with? @anatol, would that work? (passing --fetch v8 to configure)

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 22, 2013

Contributor

In general Arch does not recommend to fetch or link to third-party version of the system package. Everyone should use HEAD and 'v8' is currently at version '3.19.18.4' https://www.archlinux.org/packages/community/i686/v8/

The ideal solution for me is if you provide a patch that adopts rethinkdb to this v8 version. You don't need to submit it to github - I can temporary add the patch to my package. Later, when you upgrade v8, you'll submit the patch to repo and I remove it from my package.

Contributor

anatol commented Jul 22, 2013

In general Arch does not recommend to fetch or link to third-party version of the system package. Everyone should use HEAD and 'v8' is currently at version '3.19.18.4' https://www.archlinux.org/packages/community/i686/v8/

The ideal solution for me is if you provide a patch that adopts rethinkdb to this v8 version. You don't need to submit it to github - I can temporary add the patch to my package. Later, when you upgrade v8, you'll submit the patch to repo and I remove it from my package.

@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 22, 2013

Contributor

Ok, let us see what we can do.

Contributor

coffeemug commented Jul 22, 2013

Ok, let us see what we can do.

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 22, 2013

Contributor

@anatol I'm not exactly sure what interface changes were made with the update. Is this just the final removal of deprecated API's? If that's the case then we should be able to move to the more recent API introduced in an earlier version of V8 without breaking anything. I would be surprised if they added a new interface and removed the old one in a single release.

Contributor

wmrowan commented Jul 22, 2013

@anatol I'm not exactly sure what interface changes were made with the update. Is this just the final removal of deprecated API's? If that's the case then we should be able to move to the more recent API introduced in an earlier version of V8 without breaking anything. I would be surprised if they added a new interface and removed the old one in a single release.

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 22, 2013

Contributor

I do not know what exactly changed in v8 - I do not follow v8 project. But you can find API diff by comparing headers from 3.17.4.1 and current HEAD.

Contributor

anatol commented Jul 22, 2013

I do not know what exactly changed in v8 - I do not follow v8 project. But you can find API diff by comparing headers from 3.17.4.1 and current HEAD.

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 22, 2013

Contributor

Ok, I believe this change broke rethinkdb https://code.google.com/p/v8/source/detail?r=14603

Previously it was "class Persistent : public Handle", now Persistent inherits Handle only if V8_USE_UNSAFE_HANDLES is enabled. And as name assumes V8_USE_UNSAFE_HANDLES is undesirable to use.

Contributor

anatol commented Jul 22, 2013

Ok, I believe this change broke rethinkdb https://code.google.com/p/v8/source/detail?r=14603

Previously it was "class Persistent : public Handle", now Persistent inherits Handle only if V8_USE_UNSAFE_HANDLES is enabled. And as name assumes V8_USE_UNSAFE_HANDLES is undesirable to use.

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 22, 2013

Contributor

Thanks, @anatol. It sounds like we could make this change pretty easily without breaking compatibility with older versions of v8.

Contributor

wmrowan commented Jul 22, 2013

Thanks, @anatol. It sounds like we could make this change pretty easily without breaking compatibility with older versions of v8.

@ghost ghost assigned wmrowan Jul 22, 2013

@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 22, 2013

Contributor

Ok -- assigning to @wmrowan.

Contributor

coffeemug commented Jul 22, 2013

Ok -- assigning to @wmrowan.

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 24, 2013

Contributor

So I was wrong. The v8 people totally did push out an update that completely breaks the API (https://groups.google.com/forum/#!topic/v8-users/6kSAbnUb-rQ). I tried to hack it but there doesn't seem to be common API that remains available between versions even with #define's like V8_USE_UNSAFE_HANDLES.

Contributor

wmrowan commented Jul 24, 2013

So I was wrong. The v8 people totally did push out an update that completely breaks the API (https://groups.google.com/forum/#!topic/v8-users/6kSAbnUb-rQ). I tried to hack it but there doesn't seem to be common API that remains available between versions even with #define's like V8_USE_UNSAFE_HANDLES.

@spenthil

This comment has been minimized.

Show comment Hide comment
@spenthil

spenthil Jul 24, 2013

FYI - OSX folks installing via brew get the following error because of this (if brew install rethinkdb also installs the latest v8):

make[1]: *** [build/release_clang_notcmalloc/obj/rdb_protocol/js.o] Error 1

As a stopgap, they can get homebrew to install the previous V8:

git checkout 44505ad /usr/local/Library/Formula/v8.rb
brew unlink v8
brew install v8
brew switch v8 3.18.5
brew install rethinkdb

FYI - OSX folks installing via brew get the following error because of this (if brew install rethinkdb also installs the latest v8):

make[1]: *** [build/release_clang_notcmalloc/obj/rdb_protocol/js.o] Error 1

As a stopgap, they can get homebrew to install the previous V8:

git checkout 44505ad /usr/local/Library/Formula/v8.rb
brew unlink v8
brew install v8
brew switch v8 3.18.5
brew install rethinkdb
@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 24, 2013

Contributor

Thanks @spenthil. We're working to get this fixed.

Contributor

coffeemug commented Jul 24, 2013

Thanks @spenthil. We're working to get this fixed.

@AtnNn

This comment has been minimized.

Show comment Hide comment
@AtnNn

AtnNn Jul 25, 2013

Owner

Homebrew does not provide an easy way to install or depend on an older version of a package.

If you have the wrong version of v8 installed, you need to install an older one to be able to install rethinkdb 1.7.3:

brew unlink v8
brew install https://github.com/mxcl/homebrew/raw/44505addc6c96005b7bf53434b8bbd1c03b486f6/Library/Formula/v8.rb
brew install rethinkdb
Owner

AtnNn commented Jul 25, 2013

Homebrew does not provide an easy way to install or depend on an older version of a package.

If you have the wrong version of v8 installed, you need to install an older one to be able to install rethinkdb 1.7.3:

brew unlink v8
brew install https://github.com/mxcl/homebrew/raw/44505addc6c96005b7bf53434b8bbd1c03b486f6/Library/Formula/v8.rb
brew install rethinkdb
@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 25, 2013

Contributor

I talked to @wmrowan and here's the plan.

We're going to upgrade RethinkDB codebase to work with the most recent v8. Newer platforms will work out of the box. On older platforms we'll fetch the newer version of v8 to make it work.

Contributor

coffeemug commented Jul 25, 2013

I talked to @wmrowan and here's the plan.

We're going to upgrade RethinkDB codebase to work with the most recent v8. Newer platforms will work out of the box. On older platforms we'll fetch the newer version of v8 to make it work.

@AtnNn

This comment has been minimized.

Show comment Hide comment
@AtnNn

AtnNn Jul 25, 2013

Owner

Most of our users will not have the most recent v8. For example, here are the versions of libv8-dev avaiable on Ubuntu:

  • lucid: 2.0.3-2: amd64 i386
  • precise: 3.7.12.22-3: amd64 i386
  • quantal: 3.8.9.20-2: amd64 i386
  • raring: 3.8.9.20-2: amd64 i386

3.8 is a lot older than 3.19

I would prefer doing some ifdefing to stay compatible with the old version of v8.

Owner

AtnNn commented Jul 25, 2013

Most of our users will not have the most recent v8. For example, here are the versions of libv8-dev avaiable on Ubuntu:

  • lucid: 2.0.3-2: amd64 i386
  • precise: 3.7.12.22-3: amd64 i386
  • quantal: 3.8.9.20-2: amd64 i386
  • raring: 3.8.9.20-2: amd64 i386

3.8 is a lot older than 3.19

I would prefer doing some ifdefing to stay compatible with the old version of v8.

@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 25, 2013

Contributor

@AtnNn -- would you mind chatting with @wmrowan about it a bit to see what the easiest path to do this is? (ifdefing might be hard because it's not just a simple api change and might require significant rearchitecture)

Contributor

coffeemug commented Jul 25, 2013

@AtnNn -- would you mind chatting with @wmrowan about it a bit to see what the easiest path to do this is? (ifdefing might be hard because it's not just a simple api change and might require significant rearchitecture)

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 25, 2013

Contributor

Ok, I have this working in branch 1195_v8_update (off of v1.7.x). It turns out that the required changes weren't too extensive. Still, it might be a little tricky though to ifdef the changes.

@anatol would you mind trying this branch to see if it solves your problem on arch?

Contributor

wmrowan commented Jul 25, 2013

Ok, I have this working in branch 1195_v8_update (off of v1.7.x). It turns out that the required changes weren't too extensive. Still, it might be a little tricky though to ifdef the changes.

@anatol would you mind trying this branch to see if it solves your problem on arch?

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 25, 2013

Contributor

Sure, I'll try it. Thanks for fixing it.

Contributor

anatol commented Jul 25, 2013

Sure, I'll try it. Thanks for fixing it.

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 25, 2013

Contributor

So these changes actually weren't very hard to isolate with ifdef's. It's a little ugly but it works. I'll work with @AtnNn to integrate this into building/packaging.

Contributor

wmrowan commented Jul 25, 2013

So these changes actually weren't very hard to isolate with ifdef's. It's a little ugly but it works. I'll work with @AtnNn to integrate this into building/packaging.

@AtnNn AtnNn referenced this issue in Homebrew/legacy-homebrew Jul 25, 2013

Closed

RethinkDB 1.7.3 does not build with the latest v8 #21428

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 25, 2013

Contributor

@AtnNn is currently working on getting the ifdef switches to work correctly during the build process. In the meantime, I've put the bulk of this up for review in 752 by @Tryneus.

When these steps are done we can merge into v1.7.x and get this out. Thanks for your patience @anatol.

Contributor

wmrowan commented Jul 25, 2013

@AtnNn is currently working on getting the ifdef switches to work correctly during the build process. In the meantime, I've put the bulk of this up for review in 752 by @Tryneus.

When these steps are done we can merge into v1.7.x and get this out. Thanks for your patience @anatol.

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 25, 2013

Contributor

The change has passed review. V8 doesn't provide a version macro so integrating into the build process requires some more complex maneuvering that @AtnNn is still working on. As soon as that's done we can merge this.

Contributor

wmrowan commented Jul 25, 2013

The change has passed review. V8 doesn't provide a version macro so integrating into the build process requires some more complex maneuvering that @AtnNn is still working on. As soon as that's done we can merge this.

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 25, 2013

Contributor

I updated the Arch package with provided patch. The package builds without issues now and it looks like binary works fine.

Thanks a lot everyone!

Contributor

anatol commented Jul 25, 2013

I updated the Arch package with provided patch. The package builds without issues now and it looks like binary works fine.

Thanks a lot everyone!

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 25, 2013

Contributor

@anatol this will soon be merged into the rethinkdb next and 1.7.x and you won't need to maintain a patched version any more. It's just been a little extra work to make sure it builds with both the old and new versions of the v8 api. I'll update here when this is done but it should be very soon.

Contributor

wmrowan commented Jul 25, 2013

@anatol this will soon be merged into the rethinkdb next and 1.7.x and you won't need to maintain a patched version any more. It's just been a little extra work to make sure it builds with both the old and new versions of the v8 api. I'll update here when this is done but it should be very soon.

@coffeemug

This comment has been minimized.

Show comment Hide comment
@coffeemug

coffeemug Jul 26, 2013

Contributor

@anatol -- thanks again more maintaining the Arch package and helping us get this done. (Did you get a RethinkDB t-shirt, btw? If you didn't, we'd really love to send you one!)

Contributor

coffeemug commented Jul 26, 2013

@anatol -- thanks again more maintaining the Arch package and helping us get this done. (Did you get a RethinkDB t-shirt, btw? If you didn't, we'd really love to send you one!)

@wmrowan

This comment has been minimized.

Show comment Hide comment
@wmrowan

wmrowan Jul 26, 2013

Contributor

This is now merged in both 1.7.x and next.

@anatol This will be in 1.7.4 and 1.8. Both releases should build just fine on distros with either the new or old v8 api's. Thanks for your patience.

Contributor

wmrowan commented Jul 26, 2013

This is now merged in both 1.7.x and next.

@anatol This will be in 1.7.4 and 1.8. Both releases should build just fine on distros with either the new or old v8 api's. Thanks for your patience.

@wmrowan wmrowan closed this Jul 26, 2013

@anatol

This comment has been minimized.

Show comment Hide comment
@anatol

anatol Jul 26, 2013

Contributor

Thanks for fixing the bug! And yes I already have a rethinkdb t-shirt (thanks for it as well).

Contributor

anatol commented Jul 26, 2013

Thanks for fixing the bug! And yes I already have a rethinkdb t-shirt (thanks for it as well).

@neumino neumino referenced this issue in rethinkdb/docs Oct 7, 2013

Closed

Building rethinkdb now requires libtermcap #8

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