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

For a documentation change (to README.md), should I still fork the repo? #1287

Closed
dscotese opened this issue Dec 16, 2018 · 4 comments
Closed

Comments

@dscotese
Copy link

The change I propose to make is to the line saying to npm install bitcoinjs-lib which generated errors for me. I resolved these errors by npm install -g node-gyp and reading through GYP docs, which explained that I needed to npm install --global --production windows-build-tools as well.

I would simply add a line after the instruction to npm install bitcoinjs-lib that says "This may require that you have node-gyp installed. On Windows, you may also have to install windows-build-tools as described in the documentation for node-gyp (at https://github.com/nodejs/node-gyp/blob/master/README.md).

@junderw
Copy link
Member

junderw commented Dec 16, 2018

I have never gotten nodeJS anything to work on Windows... even doing npm install --global --production windows-build-tools didn't help me...

In all honesty, this is not a problem with bitcoinjs-lib specifically. It is because we have native bindings in one of our dependencies... so any other library with similar native bindings would have the exact same problem.

So the question then becomes: should every single repository contain a "How to troubleshoot Windows when it doesn't have a C++ building environment for node." when they want to include native bindings?

I would say no.

Also, there is a good chance that 5 years from now Windows 11 or some update breaks everything and someone on stackoverflow figures out "oh you need to run npm install --global fix-windows-bug" and then we need to add that too?...

I feel like this is not appropriate to be included in the README. And the whole point for error messages (like the ones npm and node-gyp give you) is to let you know what is wrong...

If it wasn't apparent to you from the error message, perhaps ask node-gyp to add more info to the error message when the person is running in a Windows environment. Perhaps they will include your suggestion.

Thanks for the issue. Please close if this answer is satisfactory.

@dscotese
Copy link
Author

dscotese commented Dec 17, 2018

I'll close this issue when I get some agreement that the answer is:
"No. Documentation is sensitive enough to hash out the best way to handle a proposed change here in issues first. See the first response to this issue for an example."

Off topic: I went through npm install -g learnyounode successfully on windows, but I think that was before I installed the windows build tools. If it's true that node.js won't run bitcoinjs-lib properly on Windows, that should be stated at the top of the readme, I think.

Request for clarification: Can you provide a link to "native bindings in one of our dependencies" for anyone who would like to understand that better?

Also, in case this shows anything useful regarding your "I have never gotten nodeJS anything to work on Windows," here's my console output after installing the build tools with npm and issuing npm install bitcoinjs-lib:

C:\Users\dscot>npm install bitcoinjs-lib

> tiny-secp256k1@1.0.1 install C:\Users\dscot\node_modules\tiny-secp256k1
> node-gyp rebuild


C:\Users\dscot\node_modules\tiny-secp256k1>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.
  addon.cpp
  win_delay_load_hook.cc
  secp256k1.c
     Creating library C:\Users\dscot\node_modules\tiny-secp256k1\build\Release\secp256k1.lib and object C:\Users\dscot\node_modules\tiny-secp256k1\build\Release\secp256
  k1.exp
  Generating code
  All 254 functions were compiled because no usable IPDB/IOBJ from previous compilation was found.
  Finished generating code
  secp256k1.vcxproj -> C:\Users\dscot\node_modules\tiny-secp256k1\build\Release\\secp256k1.node
npm WARN saveError ENOENT: no such file or directory, open 'C:\Users\dscot\package.json'
npm WARN enoent ENOENT: no such file or directory, open 'C:\Users\dscot\package.json'
npm WARN dscot No description
npm WARN dscot No repository field.
npm WARN dscot No README data
npm WARN dscot No license field.

+ bitcoinjs-lib@4.0.2
added 31 packages from 13 contributors, moved 19 packages and audited 375 packages in 48.154s
found 0 vulnerabilities

I couldn't get mocha installed properly (I kept getting "ReferenceError: describe is not defined"), so I removed the line for mocha, wrote my own "describe()" and "it()" functions and ran a local copy of https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/test/integration/stealth.js. I got no errors.

@junderw
Copy link
Member

junderw commented Dec 17, 2018

Try installing npm install bcrypt without first installing npm install --global --production windows-build-tools and it will fail.

bcrypt has nothing to do with bitcoinjs-lib.

The reason why it fails in the same way is because NodeJS has two types of installs.

  1. Native JS installs (learnnode etc. are probably this, since they don't teach you C++)
  2. C++ native bindings called with JS (npm install runs a C++ compiler to compile a C++ compiled binary program that NodeJS runtime manipulates... since the whole point of using C++ bindings in NodeJS is to "make it faster" your code needs to be optimized for the environment it is run in... so the binary needs to be compiled on "npm install" and can not just be downloaded etc.)

Windows is not an OS for developers, so by default Windows has 0. You need to install everything, compilers etc.

NodeJS doesn't include them in the Windows installer (why, I have no clue) so they offer some weird package via npm that you install globally to install the C++ compilers...

This has nothing to do with bitcoinjs-lib specifically, and is more of an issue of "NodeJS made decisions that are hard for new NodeJS programmers to understand, node-gyp made error messages that make it sound like the library is at fault (us), and you had a lack of understanding (which is fine)..."

Which is why I think that adding this to the README is excessive... as we would need to update the README every time NodeJS, npm, Microsoft, etc. decides to break something...

Not to be flippant or anything... but I highly recommend developing node apps in Linux. Using a VM like VirtualBox is actually not that bad.

@Janaka-Steph
Copy link

This node-gyp missing dependency is a JS dev classic. It's not related to Bitcoinjs-lib.

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

4 participants