Windows Support (and Fewer Dependencies)
Closed this issue · 7 comments
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)?
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.
@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.
@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.
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.
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.
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
, dropbrew install
dependency - Linux - obtain pre-built ICU4C, unpack as part of
npm install
, dropapt-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...
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