Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows Support (and Fewer Dependencies) #32

Closed
shiftkey opened this issue Nov 28, 2016 · 7 comments
Closed

Windows Support (and Fewer Dependencies) #32

shiftkey opened this issue Nov 28, 2016 · 7 comments

Comments

@shiftkey
Copy link

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
Copy link

ccoenen commented Dec 5, 2016

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
Copy link
Author

shiftkey commented Dec 5, 2016

@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
Copy link
Owner

mooz commented Dec 8, 2016

@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.

@shiftkey
Copy link
Author

shiftkey commented Dec 8, 2016

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
Copy link

ccoenen commented Dec 8, 2016

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
Copy link
Author

shiftkey commented Mar 15, 2017

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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants