search

FrontierPsychiatrist / node-spotify

A module for node.js to use libspotify.
MIT License
142 stars 20 forks source link

Can't compile 0.7.1 on node 4.1.0 #97

Closed amaramth closed 8 years ago

amaramth commented 9 years ago

I reverted to node 0.12 for now. Does npm install node-spotify reproduce this for you on node 4.1.0?

(osx, vanilla setup; node 4.x installed with "brew upgrade node")

FrontierPsychiatrist commented 9 years ago

Yes, I tried that too. node-spotify is not compatible with the changes in V8 that are in NodeJS 4. Unfortunately I currently don't have the time to take a look at it.

Nuxij commented 9 years ago

:+1: Please!

I tried having a look myself but it's certainly not the kind of thing to be diving in to for your first Node/C++ expedition!

amaramth commented 9 years ago

I'm working on pruning it down to the subset that's not compatible with the new web api, and then that would be much easier to convert to node 4.x or whatever.

FrontierPsychiatrist commented 8 years ago

I guess I could take a look into upgrading nan to a newer version and hope that helps.

FrontierPsychiatrist commented 8 years ago

Ok, I started an upgrade to nan 2.1.x which sadly means a lot of changes (NanNew -> Nan::New, EscapableHandleScopes,...). I hope I find some time to make it compile and work again.

Nuxij commented 8 years ago

@FrontierPsychiatrist That's what I was trying, I hope you have a better time of it!

FrontierPsychiatrist commented 8 years ago

Ok, I progressed a bit further, this is really a lot of work to do and I don't even know if it will work when it compiles. Will take a while.

FrontierPsychiatrist commented 8 years ago

Just a short life sign: Still not much progress. I'm still working on making it compile and I really hope when it compiles it will also run. There were so many changes to nan...

Nuxij commented 8 years ago

Stay strong brother!

FrontierPsychiatrist commented 8 years ago

Since it's been a while: I haven't found time to work on this, especially after the need to travel for my work. I'm really sorry for that and hope that soon there will be more time.

clarkieryan commented 8 years ago

I've had a look into this - looks as if it's the nan library - had a shot at upgrading to the latest version - but they have changed their namespacing - so really not sure what to do? I'm happy to help if you have any pointers!

FrontierPsychiatrist commented 8 years ago

@clarkieryan basically all nan usages have to be changed, sadly it's not just adding the Nan:: namespace. A lot of functions have different signatures. Maybe I can push my migration branch later.

crwgregory commented 8 years ago

I am also willing to lend a helping hand.

clarkieryan commented 8 years ago

@FrontierPsychiatrist If you could push that branch that would be helpfull :)

FrontierPsychiatrist commented 8 years ago

So I pushed https://github.com/FrontierPsychiatrist/node-spotify/tree/nan-2.1

As you can see, my last commit before today was start of December. I had some uncommited stuff from during December lying around.

I might want to restart clean at some point., these commits were not meant to be published, my plan was to squash them. Have a look at it and if you can manage to get more stuff to compile, feel free to open a PR (be sure to make it point to nan-2.1). Please add why you changed something to compile in the commit messages (unlike I did!).

Oh, and by the way, end of week I don't need to travel anymore for work - maybe that will result in a fresh start.

FrontierPsychiatrist commented 8 years ago

So, https://github.com/FrontierPsychiatrist/node-spotify/tree/nan-2.3 just happened - at least it compiles. I almost don't dare to start it...

FrontierPsychiatrist commented 8 years ago

Well, my tests don't exactly run, but in the REPL I can load a track just fine (node 5.3.0).

Anyone interested, please check the branch out and give it a try and tell me what goes wrong, that would be very helpful.

FrontierPsychiatrist commented 8 years ago

On OSX I had to do a xcode-select --install, otherwise it wouldn't find the libspotify header file in /usr/local. Probably not a problem with node, but if someone gets the error that #include <libspotify/api.h> is not working on OSX even though it is installed (e.g. brew install libspotify), do that.

Source http://stackoverflow.com/questions/23905661/on-mac-g-clang-fails-to-search-usr-local-include-and-usr-local-lib-by-def

FrontierPsychiatrist commented 8 years ago

Yesterday I changed some more things (mostly Handle<T> to Local<T>) and node-spotify now compiles on 0.10, 0.12, 5.0, 6.0 (I guess 4.0 to). 0.8 doesn't have libuv's uv_cond_t, I'm leaving that out for now.

After I look a little bit more into why my tests don't run I will publish 0.7.2 with the updated Nan. A next good step would then be to look into #89 to make the binary distribution easier.

FrontierPsychiatrist commented 8 years ago

I pushed 0.7.2 to npm with nan 2.3.5.

There is one regression, spotify.player.off does not do anything anymore (see here: https://github.com/FrontierPsychiatrist/node-spotify/blob/b800e2318a2216ce379620cd101bf4ac840b56f7/src/objects/node/NodePlayer.cc#L87 and #100).

philipbulley commented 8 years ago

Working well for me under node 6.2.1... I can finally ditch 0.12.7 which also means that iron-node can be used to debug my app! :zap: Thanks for fixing!

Nuxij commented 8 years ago

Thanks guys!!!