mooz/node-icu-charset-detector

Windows Support (and Fewer Dependencies)

Closed this issue · 7 comments

shiftkey commented

I've started on a branch here which has a few goals:

  • first-class support for Windows
  • remove the requirement for ICU to be installed on the user's machine
  • add some tests to ensure it's working for each supported platform

I can see there's some related issues around Window support: #2 #27

So far it's proceeding nicely:

C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector>if not defined npm_config_node_gyp (node "C:\Program Files\nodejs\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  node-icu-charset-detector.cpp
  win_delay_load_hook.cc
..\node-icu-charset-detector.cpp(44): warning C4267: 'argument': conversion from 'size_t' to 'int32_t', possible los
s of data [C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\node-icu-charset-detector.vcxproj]
..\node-icu-charset-detector.cpp(77): warning C4530: C++ exception handler used, but unwind semantics are not enable
d. Specify /EHsc [C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\node-icu-charset-detector.vcxpr
oj]
c:\users\shiftkey\documents\github\node-icu-charset-detector\node_modules\nan\nan_new.h(214): warning C4267: 'argume
nt': conversion from 'size_t' to 'int', possible loss of data (compiling source file ..\node-icu-charset-detector.cp
p) [C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\node-icu-charset-detector.vcxproj]
  ..\node-icu-charset-detector.cpp(95): note: see reference to function template instantiation 'v8::MaybeLocal<v8::S
  tring> Nan::New<v8::String,const char*,size_t>(A0,A1)' being compiled
          with
          [
              A0=const char *,
              A1=size_t
          ]
     Creating library C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\Release\node-icu-charset-de
  tector.lib and object C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\Release\node-icu-charset-
  detector.exp
  Generating code
  Finished generating code
  node-icu-charset-detector.vcxproj -> C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector\build\Release\\n
  ode-icu-charset-detector.node
node-icu-charset-detector@0.2.0 C:\Users\shiftkey\Documents\GitHub\node-icu-charset-detector

A couple of questions:

  • Is this something you're interested in supporting? I know Windows things can be a hassle without having access to the OS, so perhaps I can help with setting up some continuous integration...
  • Is there anything I should be aware of to help make this easy for you to review when I do open a PR?
  • Are you fine with me only testing this on Node 6 (LTS) and 7 (current)?
ccoenen commented

I'm not on this project's team, but I just came here to find out about the state of windows support. So seeing your work makes me very hopeful.

shiftkey commented

@ccoenen given the amount of silence right now I'll probably publish this up to a scoped package under my NPM account when it's ready to consume. Stay tuned.

mooz commented

@shiftkey Sorry for the delayed response. This is an exciting news! I appreciate your effort.

Is this something you're interested in supporting?

I'm happy to support Windows. Since I don't use Windows (as a development environment), I would be happy if you can help me setting CI env. I can add you to developers for giving a privilege to this repository.

Is there anything I should be aware of to help make this easy for you to review when I do open a PR?

Nothing.

Are you fine with me only testing this on Node 6 (LTS) and 7 (current)?

That's fine.

  • ❤️1
shiftkey commented

I can add you to developers for giving a privilege to this repository.

That'd be great. I've used Appveyor for this sort of thing in the past. I'll get that working as part of the PR.

ccoenen commented

I would like to express my sincere thanks to both of you! It's details like this that make JavaScript my current go-to language.

shiftkey commented

I have a more stable development branch underway here which is getting close to opening a PR targeting this repository - master...shiftkey:dev

Done:

  • Windows support
  • Linux support (probably needs some refinement)
  • Continuous Integration - example Travis build and example Appveyor build
  • Integration tests verifying non-UTF-8 files on all platforms (to ensure we're using ICU correctly)

Remaining tasks are mostly related to how ICU4C is distributed:

  • Windows - unpack pre-built ICU4C as part of npm install, drop custom CI setup and environment variables
  • macOS - obtain pre-built ICU4C (or just build it from source), unpack as part of npm install, drop brew install dependency
  • Linux - obtain pre-built ICU4C, unpack as part of npm install, drop apt-get install dependency

Once I've closed out these tasks, I'll make a scoped package available for others to help with testing, while we talk about getting these changes incorporated into the upstream project...

shiftkey commented

Hey gang, I'm actually not going to move ahead with improving this project, because I think a perfectly fine solution already exists leveraging iconv-lite and jschardet. You can see a sample of this here: atom/node-pathwatcher#120

  • 👍1