bnoordhuis / node-buffertools

working with node.js buffers made easy
ISC License
205 stars 35 forks source link

Visual Studio build fails #58

Closed matthewgertner closed 10 years ago

matthewgertner commented 10 years ago

Since you added a call to snprintf, npm is no longer able to build buffertools using Visual Studio:

..\buffertools.cc(431): error C3861: 'snprintf': identifier not found

Believe it or not, even Visual Studio 2013 does not appear to support snprintf.

The suggested workaround is:

#if _MSC_VER
#define snprintf _snprintf
#endif
bnoordhuis commented 10 years ago

Right, I kind of knew that in the back of my head, I fixed the same issue in joyent/node a while ago. Does this patch help?

diff --git a/buffertools.cc b/buffertools.cc
index c3b49f4..34ba18d 100644
--- a/buffertools.cc
+++ b/buffertools.cc
@@ -20,8 +20,10 @@
 #include "v8.h"

 #include <algorithm>
-#include <cstring>
+#include <stdarg.h>
 #include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
 #include <string>

 namespace {
@@ -97,6 +99,24 @@ using v8::Value;
     v8::ThrowException(v8::String::New(message))
 #endif  // NODE_MAJOR_VERSION > 0 || NODE_MINOR_VERSION > 10

+#if defined(_WIN32)
+// Emulate snprintf() on windows, _snprintf() doesn't zero-terminate
+// the buffer on overflow.
+inline int snprintf(char* buf, size_t size, const char* fmt, ...) {
+  va_list ap;
+  va_start(ap, fmt);
+  const int len = _vsprintf_p(buf, size, fmt, ap);
+  va_end(ap);
+  if (len < 0) {
+    abort();
+  }
+  if (static_cast<unsigned>(len) >= size && size > 0) {
+    buf[size - 1] = '\0';
+  }
+  return len;
+}
+#endif
+
 // this is an application of the Curiously Recurring Template Pattern
 template <class Derived> struct UnaryAction {
   Local<Value> apply(Local<Object> buffer,
matthewgertner commented 10 years ago

Well it builds of course. I'll have to do some more testing to verify that it runs properly (though I don't doubt that it does). The main problem is that buffertools is being installed and built by npm inside a deep tree of packages, so I need to figure out how to get it to use the patched version.

bnoordhuis commented 10 years ago

You can npm link it in your top-level node_modules directory and then you just delete all lower-down buffertools directories.

matthewgertner commented 10 years ago

The problem is that it runs on our CI server with a fresh npm install every time, so it's pulling and building without any obvious possibility to get in there and patch something. I dealt with it for now by dropping your snprintf implementation into stdio.h.

bnoordhuis commented 10 years ago

Okay, if you can confirm that it works for you, I'll land it and publish a new release.

matthewgertner commented 10 years ago

I guess you don't have a test that triggers that error message? I have everything built but will have to write a testcase if you don't have one.

bnoordhuis commented 10 years ago

Well, there's not much to test for - the module either builds or it doesn't. The confirmation I'm looking for is whether it builds for you. :-)

vineet89 commented 10 years ago

I can confirm that @matthewgertner 's solution worked and I was able to build packages with buffertools as their dependency(by using npm link). Git threw some fatal errors when I tried to apply the above patch, I will try to add the lines manually and revert in some time.

Thanks to both of you for the fix

matthewgertner commented 10 years ago

@bnoordhuis Ah okay I thought you wanted me to test that the snprintf function works. I can confirm that it does build.

bnoordhuis commented 10 years ago

Cheers, fixed in 70732be and published as v2.1.2.

matthewgertner commented 10 years ago

Awesome, thanks @bnoordhuis.