search

kuno / GeoIP

GeoIP binding for nodejs(>=0.10) and iojs
GNU Lesser General Public License v2.1
414 stars 129 forks source link

Support for binary deployment #74

Closed springmeyer closed 10 years ago

springmeyer commented 10 years ago

Using node-pre-gyp this change advances GeoIP to enable users to install the module without a compiler via a binary. This can make GeoIP much easier to install for javascript developers not familiar with native addons and it can make GeoIP much faster to install for devops teams.

Please see the node-pre-gyp docs (https://github.com/mapbox/node-pre-gyp#features) for details of how this works. And this page for examples of other apps that are using node-pre-gyp to provide binaries.

Next steps:

kuno commented 10 years ago

To use this package, you just need make and some c/c++ compiler installed. I think the most *NIX already have it.

So I think maybe this feature is make most sense for windows user?

How easy is it to produce a windows binary, do I need to build one using visual studio?

springmeyer commented 10 years ago

The node-gyp dependencies (python, make, c++ compiler) often are confusing and error prone for Node.js users not familiar with compiling. Using node-pre-gyp addresses this by making it easy to install the module without any knowledge of development tools and without failing if the development tools are broken or only partially installed. This becomes most useful for applications that depend on GeoIP. Users may try to install those applications without knowing they need make, python, and g++.

The primary reason I am interested in GeoIP being packaged this way is to be able to install the module without needing a compiler on Linux. For reasons of speed and simplicity in deployment on Linux I do packaging of node modules without g++ installed.

Yes, regarding windows, because the compiler toolchain is so much harder to set up node-pre-gyp binaries are especially useful. So I did start looking into how to build GeoIP on windows. I saw that this first problem is libgeoip.gypi which has DEFINES that are only valid on unix. The first compile error was fixed by commenting HAVE_GETTIMEOFDAY: https://github.com/kuno/GeoIP/blob/4e63f370b9f4ba491087257a99bcd20d93679588/deps/geoip-api-c-1.6.0/libgeoip.gypi#L64. After that I saw more compile errors and figured I would return to it when I had more time.

springmeyer commented 10 years ago

regarding windows these are the problems seen so far:

I've fixed the ssize_t and unistd.h issues and started fixing the variable length array issue (but it is repeated in many files): https://github.com/mapbox/GeoIP/commit/2c5b50cfee71f294b7eedada8c1ea29df839899d. You can see all the variable length array misusage (plus other issues) on unix by doing:

export CXXFLAGS="-pedantic"
npm install

I get: https://gist.github.com/springmeyer/9ceb37d1664d90a51f09

kuno commented 10 years ago

C++ is f*cking complicated lol, must I saw the almost errors were came from third parity module memwatch and the libgeoip c lib itself.

Is it hard to fixed those kind of error/warning?

springmeyer commented 10 years ago

Is it hard to fixed those kind of error/warning?

No, it is very easy. Just see this section: https://github.com/mapbox/GeoIP/commit/2c5b50cfee71f294b7eedada8c1ea29df839899d#diff-bd5b7ee8f0c1b47a7c7a003e7fcfd619L70. You need to do that for every file that has the same kind of code. Turn on the warnings so you can see for yourself when it is fixed. In other words I think this should be fixed in general and not just for windows.

kuno commented 10 years ago

sorry, after thought some while, I found this pr was against my intention to make this module as self hosting as possible.

But anyway, thank u for your efforts.