robertklep / node-metrohash

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

Suggestion: Add AppVeyor test suite to check for Windows support #8

Closed swernerx closed 6 years ago

swernerx commented 6 years ago

I am the author of asset hash where I chose metrohash as the default hashing algorithm. Right now I experience some issues in my package as it does not seem to correctly compile on appveyor. This seems to be related to Metrohash. This is the output I am currently seeing:

..\src\metrohash128crc.cpp(55): error C3861: '_mm_crc32_u64': identifier not found [C:\projects\rollup-plugin-rebase\node_modules\metrohash\build\metrohash.vcxproj]
...

Full log output: https://ci.appveyor.com/project/swernerx/rollup-plugin-rebase/build/136/job/rw5qlf48amcmb9s9

Do have any hint of fixing this? Would it be feasible for you to add appveyor testing?

A good initial appveyor.yml could be this one:

# http://www.appveyor.com/docs/appveyor-yml

clone_depth: 10

environment:
  matrix:
    - nodejs_version: 6
    - nodejs_version: 8
    - nodejs_version: 10

version: "{build}"
build: off
deploy: off

install:
  - ps: Install-Product node $env:nodejs_version
  - npm install

test_script:
  - npm test

cache:
  # global npm cache
  - '%APPDATA%\npm-cache'
robertklep commented 6 years ago

_mm_crc32_u64 is an SSE4 instruction for 64-bit environments.

If I understand the log output correctly, the tests are using a 32-bit Node.js version, which is causing the error.

There's some info about how to install a 64-bit version of Node using AppVeyor here: https://www.appveyor.com/docs/lang/nodejs-iojs/#selecting-nodejs-or-iojs-version

It looks like you have to install the 64-bit versions explicitly:

install:
  - ps: Install-Product node $env:nodejs_version x64

Let me know if that solves your problem, in which case I'm happy to add AppVeyor support.

swernerx commented 6 years ago

Thanks for the quick reply! This indeed seems to be the cause.

Looks good now: https://ci.appveyor.com/project/swernerx/asset-hash/build/45

swernerx commented 6 years ago

This is the config I am using right now:

clone_depth: 10

environment:
  matrix:
    - nodejs_version: 6
    - nodejs_version: 8
    - nodejs_version: 10

matrix:
  fast_finish: true

platform:
  - x64

version: "{build}"
build: off
deploy: off

install:
  - ps: Install-Product node $env:nodejs_version $env:platform
  - npm install

test_script:
  - npm test

cache:
  # global npm cache
  - '%APPDATA%\npm-cache'
robertklep commented 6 years ago

Awesome, thanks for testing! I'll try to add support for it ASAP.

robertklep commented 6 years ago

Just pushed metrohash@2.4.2 with AppVeyor support.

swernerx commented 6 years ago

Thanks!