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

typecast to uv_async_cb to fix node-webkit compile #239

Closed
wants to merge 1 commit into from
Closed

typecast to uv_async_cb to fix node-webkit compile #239

wants to merge 1 commit into from

Conversation

springmeyer
Copy link
Contributor

@rvagg
Copy link
Member

rvagg commented Jan 17, 2015

oh, not so great that we let this slip in, we obviously don't have test cases for this. I'm 👍 on this change and pushing a patch-release for 1.5. While we wait for additional input @springmeyer can you have a quick look at the tests to see why this isn't flagged anywhere?

@bnoordhuis
Copy link
Member

I suspect a bug in the NAUV_UVVERSION macro because the NAUV_WORK_CB macro is supposed to take care of the differences in uv_async_cb prototypes. @brett19? I believe you originally wrote that?

springmeyer pushed a commit to TryGhost/node-sqlite3 that referenced this pull request Jan 17, 2015
@springmeyer
Copy link
Contributor Author

Just confirmed that all of the crazy node-sqlite3 build matrix works with this patch: https://travis-ci.org/mapbox/node-sqlite3/builds/47314165

@springmeyer can you have a quick look at the tests to see why this isn't flagged anywhere?

Okay, taking a look. As a byproduct see #240 which should help those new to running the tests like me.

@springmeyer
Copy link
Contributor Author

I've come up short on a reduced testcase to replicate/prevent regression. But I've found that it is easy to replicate within the nan directory just by doing:

npm install nw-gyp
./node_modules/.bin/nw-gyp rebuild --directory test --target=0.11.5

Is it reasonable for the nan .travis.yml to test against node-webkit to be able to catch more subtle breakages like this or do you need something more reduced?

@rvagg
Copy link
Member

rvagg commented Jan 17, 2015

I don't mind using nw-gyp as a devdep if it's going to catch more, I just don't know anything about nw-gyp at all

@springmeyer
Copy link
Contributor Author

Okay, new pull request testing node-webkit at #241 (idea is we can watch the travis build there to make sure it fails).

@kkoopa
Copy link
Collaborator

kkoopa commented Jan 17, 2015

The declaration of AsyncProgress_ comes after this, maybe that's why it's whining? The symbol is undefined at that point.

@kkoopa kkoopa mentioned this pull request Jan 18, 2015
@kkoopa kkoopa closed this Jan 18, 2015
kkoopa added a commit that referenced this pull request Jan 18, 2015
Replicate issue #239 by testing node-webkit on travis
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.

node-webkit compile error
4 participants