robertklep / node-metrohash

Node.js bindings for MetroHash
MIT License
26 stars 8 forks source link

Installing metrohash on Windows fails at metrohash_wrapper.cpp #7

Closed mviranyi closed 6 years ago

mviranyi commented 6 years ago

On my Windows 10 environment (Windows_NT 10.0.15063) install of metrohash fails during compilation with msbuild.exe:

Directory: C:\Users\{USER}\Git\{PROJECT}\runtime\node_modules\metrohash
Output:
gyp info it worked if it ends with ok
gyp info using node-gyp@3.7.0
gyp info using node@8.11.3 | win32 | x64
gyp info spawn C:\Python27\python.exe
gyp info spawn args [ 'C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\node-gyp\\gyp\\gyp_main.py',
gyp info spawn args   'binding.gyp',
gyp info spawn args   '-f',
gyp info spawn args   'msvs',
gyp info spawn args   '-G',
gyp info spawn args   'msvs_version=2015',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\metrohash\\build\\config.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\node-gyp\\addon.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'C:\\Users\\{USER}\\.node-gyp\\8.11.3\\include\\node\\common.gypi',
gyp info spawn args   '-Dlibrary=shared_library',
gyp info spawn args   '-Dvisibility=default',
gyp info spawn args   '-Dnode_root_dir=C:\\Users\\{USER}\\.node-gyp\\8.11.3',
gyp info spawn args   '-Dnode_gyp_dir=C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\node-gyp',
gyp info spawn args   '-Dnode_lib_file=C:\\Users\\{USER}\\.node-gyp\\8.11.3\\<(target_arch)\\node.lib',
gyp info spawn args   '-Dmodule_root_dir=C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\metrohash',
gyp info spawn args   '-Dnode_engine=v8',
gyp info spawn args   '--depth=.',
gyp info spawn args   '--no-parallel',
gyp info spawn args   '--generator-output',
gyp info spawn args   'C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\metrohash\\build',
gyp info spawn args   '-Goutput_dir=.' ]
gyp info spawn C:\Program Files (x86)\MSBuild\14.0\bin\msbuild.exe
gyp info spawn args [ 'build/binding.sln',
gyp info spawn args   '/clp:Verbosity=minimal',
gyp info spawn args   '/nologo',
gyp info spawn args   '/p:Configuration=Release;Platform=x64' ]
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  metrohash_wrapper.cpp
  metrohash128.cpp
  metrohash128crc.cpp
  metrohash64.cpp
  win_delay_load_hook.cc
..\src\metrohash_wrapper.cpp(140): error C2131: expression did not evaluate to a constant [C:\Users\{USER}\Git\{PROJECT}\runtime\node_modules\metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(140): note: a non-constant (sub-)expression was encountered
..\src\metrohash_wrapper.cpp(141): error C2131: expression did not evaluate to a constant [C:\Users\{USER}\Git\{PROJECT}\runtime\node_modules\metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(141): note: a non-constant (sub-)expression was encountered
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\MSBuild\14.0\bin\msbuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Users\{USER}\Git\{PROJECT}\runtime\node_modules\node-gyp\lib\build.js:262:23)
gyp ERR! stack     at emitTwo (events.js:126:13)
gyp ERR! stack     at ChildProcess.emit (events.js:214:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)
gyp ERR! System Windows_NT 10.0.15063
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\{USER}\\Git\\{PROJECT}\\runtime\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd C:\Users\{USER}\Git\{PROJECT}\runtime\node_modules\metrohash
gyp ERR! node -v v8.11.3
gyp ERR! node-gyp -v v3.7.0
gyp ERR! not ok
Process stalled

I've tried re-installing Windows Build Tools, Python, setting PATHS. Basically all that is mentioned here: https://github.com/Microsoft/nodejs-guidelines/blob/master/windows-environment.md#compiling-native-addon-modules But nothing seemed to change it. Any other suggestions? Glad about any directions

mviranyi commented 6 years ago

When I apply this patch

--- a/src/metrohash_wrapper.cpp
+++ b/src/metrohash_wrapper.cpp
@@ -137,8 +137,8 @@ NAN_METHOD(NodeMetroHashFn) {
     info.GetReturnValue().Set(Nan::Encode((char *) digest, BITS >> 3, Nan::HEX));
 };

-constexpr auto NodeMetroHashFn64  = &NodeMetroHashFn<MetroHash64,  64>;
-constexpr auto NodeMetroHashFn128 = &NodeMetroHashFn<MetroHash128, 128>;
+static const auto NodeMetroHashFn64  = &NodeMetroHashFn<MetroHash64,  64>;
+static const auto NodeMetroHashFn128 = &NodeMetroHashFn<MetroHash128, 128>;

 // Addon initialization.
 NAN_MODULE_INIT(InitAll) {

I am able to build via npm install . from the cloned repo

PS C:\Users\{USER}\Git\{PROJECT}\node-metrohash> npm install .

> metrohash@2.4.0 install C:\Users\{USER}\Git\{PROJECT}\node-metrohash
> node-gyp rebuild

C:\Users\{USER}\Git\{PROJECT}\node-metrohash>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js"
 rebuild )  else (node "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" rebuild )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  metrohash_wrapper.cpp
  metrohash128.cpp
  metrohash128crc.cpp
  metrohash64.cpp
  win_delay_load_hook.cc
..\src\metrohash_wrapper.cpp(24): warning C4244: 'argument': conversion from 'double' to 'uint64_t', possible loss of data [C:\Users\{USER}\Git\{PROJECT}\node-metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(20): note: while compiling class template member function 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHash<MetroHash128,16>::New(Nan::NAN_METHOD_ARGS_TYPE)'
  ..\src\metrohash_wrapper.cpp(85): note: see reference to function template instantiation 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHash<MetroHash128,16>::New(Nan::NAN_METHOD_ARGS_TYPE)' being compiled
  ..\src\metrohash_wrapper.cpp(147): note: see reference to class template instantiation 'NodeMetroHash<MetroHash128,16>' being compiled
..\src\metrohash_wrapper.cpp(133): warning C4244: '=': conversion from 'double' to 'uint64_t', possible loss of data [C:\Users\{USER}\Git\{PROJECT}\node-metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(140): note: see reference to function template instantiation 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHashFn<MetroHash64,64>(Nan::NAN_METHOD_ARGS_TYPE)' being compiled
     Creating library C:\Users\{USER}\Git\{PROJECT}\node-metrohash\build\Release\metrohash.lib and object C:\Users\{USER}\Git\{PROJECT}\node-metrohash\build\Release\metrohash.exp
  Generating code
  Finished generating code
  metrohash.vcxproj -> C:\Users\{USER}\Git\{PROJECT}\node-metrohash\build\Release\\metrohash.node
audited 230 packages in 4.552s
found 1 moderate severity vulnerability
  run `npm audit fix` to fix them, or `npm audit` for details

Would this be the correct fix for this kind of Windows compilation?

robertklep commented 6 years ago

To be honest, I know too little of C++ to give a decisive answer. However, I think that the added static changes the semantics, I think this matches the constexpr code closer:

const auto NodeMetroHashFn64 = ...
const auto NodeMetroHashFn128 = ...

If that works for you, let me know. It doesn't change anything on my side (macOS) so I'm happy to push a new version if that will make it compile on Windows properly.

mviranyi commented 6 years ago

Since I'm not entirely sure about C++ myself, I wouldn't tell the diff. But your proposed changes, not to change semantics, sounds fair. Metrohasher still compiles and seems to function well with your changes. From my side, go ahead.

robertklep commented 6 years ago

Before I publish a new release, can you check and see if the current GH-hosted version works okay for you?

npm i robertklep/node-metrohash
mviranyi commented 6 years ago

Looking good on my Windows machine:

{USER}@4DEL{USER}2 /cygdrive/c/Users/{USER}/Git/{PROJ}/metrohasher-test-github
$ npm i robertklep/node-metrohash

> metrohash@2.4.1 install C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash
> node-gyp rebuild

C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "C:\Program Files\nodejs\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" rebuild )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  metrohash_wrapper.cpp
  metrohash128.cpp
  metrohash128crc.cpp
  metrohash64.cpp
  win_delay_load_hook.cc
..\src\metrohash_wrapper.cpp(24): warning C4244: 'argument': conversion from 'double' to 'uint64_t', possible loss of data [C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(20): note: while compiling class template member function 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHash<MetroHash128,16>::New(Nan::NAN_METHOD_ARGS_TYPE)'
  ..\src\metrohash_wrapper.cpp(85): note: see reference to function template instantiation 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHash<MetroHash128,16>::New(Nan::NAN_METHOD_ARGS_TYPE)' being compiled
  ..\src\metrohash_wrapper.cpp(147): note: see reference to class template instantiation 'NodeMetroHash<MetroHash128,16>' being compiled
..\src\metrohash_wrapper.cpp(133): warning C4244: '=': conversion from 'double' to 'uint64_t', possible loss of data [C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash\build\metrohash.vcxproj]
  ..\src\metrohash_wrapper.cpp(140): note: see reference to function template instantiation 'Nan::NAN_METHOD_RETURN_TYPE NodeMetroHashFn<MetroHash64,64>(Nan::NAN_METHOD_ARGS_TYPE)' being compiled
     Creating library C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash\build\Release\metrohash.lib and object C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash\build\Release\metrohash.exp
  Generating code
  Finished generating code
  metrohash.vcxproj -> C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\node_modules\metrohash\build\Release\\metrohash.node
npm WARN saveError ENOENT: no such file or directory, open 'C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open 'C:\Users\{USER}\Git\{PROJ}\metrohasher-test-github\package.json'
npm WARN metrohasher-test-github No description
npm WARN metrohasher-test-github No repository field.
npm WARN metrohasher-test-github No README data
npm WARN metrohasher-test-github No license field.

+ metrohash@2.4.1
added 108 packages from 74 contributors and audited 191 packages in 12.099s
found 4 moderate severity vulnerabilities
  run `npm audit fix` to fix them, or `npm audit` for details
robertklep commented 6 years ago

Thanks for testing, I just published v2.4.1 👍🏻