Skip to content
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

Target new v8 from chromium 38 #369

Merged
merged 3 commits into from
Dec 8, 2014
Merged

Target new v8 from chromium 38 #369

merged 3 commits into from
Dec 8, 2014

Conversation

bwin
Copy link
Contributor

@bwin bwin commented Nov 25, 2014

This fixes compiling errors for node-webkit v0.11.0 and atom-shell v0.18.0 as reported in #365

v8::Object::Set in recent versions of chromium (v38) has no overloaded version which takes 3 arguments (as before).
I replaced those occurences with v8::Object::ForceSet which can take the 3rd arg PropertyAttribute.

Travis tests pass, except for the node-webkit-v0.11.0-ia32 one.
I tested it with atom-shell v0.19.4 on ia32 windows and it works. (That's my use-case. Couldn't upgrade past 0.17.2 before.)

This is definitely not my field of expertise, so please feel free to call bullsh**t on this.
@Mithgol your opinion?

+ recent atom-shell >= 0.18.0
replaced occurences of v8::Object::Set with 3 arguments with
v8::Object::ForceSet
@springmeyer
Copy link
Contributor

Thanks for digging into this and finding a fix. My sense is that this fix may be more appropriate for https://github.com/rvagg/nan/ if it is resulting from a difference in v8 versions.

@Mithgol
Copy link
Contributor

Mithgol commented Nov 25, 2014

By the way, does it result from a difference in v8 versions or a difference in nan versions? (Not a rhetorical question, because I do not yet have an opinion here.)

On an unrelated note, having looked at a failing job's log, it seems that a newer 32-bit node-webkit requires yet another 32-bit library to run.

@Mithgol
Copy link
Contributor

Mithgol commented Nov 25, 2014

I hereby use a GitHub mention and summon @rvagg to look on the PR's changelog and decide if these changes (mostly ->Set replaced by ->ForceSet) can be made unnecessary by an improvement of nan or not.

@rvagg
Copy link

rvagg commented Nov 26, 2014

Discussion continued @ nodejs/nan#221

My initial instinct would be to say that ForceSet() is the way to go here if you really need to set ReadOnly but we'll see what the others have to say about the matter.

@bwin bwin mentioned this pull request Dec 2, 2014
@bwin
Copy link
Contributor Author

bwin commented Dec 8, 2014

Searching github for "v8 forceSet" it seems to me, ForceSet is indeed the way to go.

About the failing AppVeyor builds. It seems that there is something else wrong:
gyp ERR! stack Error: node.lib local checksum [...] not match remote [...]

@Mithgol what to do about the missing lib for [email protected] 32bit?

@Mithgol
Copy link
Contributor

Mithgol commented Dec 8, 2014

@bwin

In the job log node-webkit reports the error /usr/lib32/libstdc++.so.6: version GLIBCXX_3.4.18 not found.

That lib has to be installed by adding a package (containing that lib) to the line 49 of build_against_node_webkit.sh where the sudo apt-get -y install command already installs some other necessary packages.

What package does contain that lib? I don't know (yet), but Ubuntu package search may help.

@bwin
Copy link
Contributor Author

bwin commented Dec 8, 2014

Thanks, I will look into it.

@springmeyer
Copy link
Contributor

gyp ERR! stack Error: node.lib local checksum [...] not match remote [...]

@bwin you can ignore this error. If it persists I'll will take a look after merging your fix. It is only breaking for the builds against a custom node.exe on windows built against Visual Studio 2014 (https://github.com/mapbox/node-cpp11) and the problem I think is unrelated to your fix and rather that I need to update the shasums.

Thanks, I will look into it.

Great, once you have [email protected] 32bit working on travis I'll merge this. Thanks!

@bwin
Copy link
Contributor Author

bwin commented Dec 8, 2014

:feelsgood: In the end it was so easy... wasted a lot of time on this, though. But hey, it works.
Since Travis runs Ubuntu 12.04, we have to add a ppa to install gcc/g++ 4.8. I special cased this to nw >= 0.11.0 on ia32.
Please have a look at the changes.
If you merge this, please republish on npm.

springmeyer pushed a commit that referenced this pull request Dec 8, 2014
@springmeyer springmeyer merged commit cb994d7 into TryGhost:master Dec 8, 2014
@springmeyer
Copy link
Contributor

Thanks! Will issue a new release asap.

@springmeyer
Copy link
Contributor

gyp ERR! stack Error: node.lib local checksum [...] not match remote [...]

Okay, this is now solved: https://ci.appveyor.com/project/Mapbox/node-sqlite3

@martinib77
Copy link

How can the new version be used with node-webkit ?. It's necesary to wait until a new npm release (3.0.5) ? Thanks!

bendrucker added a commit to knex/knex that referenced this pull request Feb 7, 2015
sqlite3 has an unpublished fix:

TryGhost/node-sqlite3#369

Right now this causes npm install to fail and so the 0.11 job is
meaningless. This can be reverted when sqlite3 can be built on
node > 0.10 and iojs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants