eslint requires nodejs built with ssl support

[jbergstroem]

eslint requires nodejs to be built against openssl which is optional (nodejs $ ./configure --without-ssl). A quick check points to eslint using it to calculate a md5sum. Would a patch that uses a javascript version of a md5sum possibly be accepted?

Upstream issue: nodejs/node#5610

[ilyavolodin]

md5sum calculation is used for caching. In general, I don't see a problem with replacing it with JavaScript only implementation, but would love to see perf numbers, as well as additional download size before making a call.

[jbergstroem]

Thanks for the feedback. I'll get to benchmarking and writing a simple implementation. Would you prefer a package through npm (larger) or bundled version (smaller)?

[michaelficarra]

@jbergstroem Be aware that it doesn't have to be MD5 or even a decent cryptographic hash function. It just has to be a hash with a large enough digest size to avoid collisions. It doesn't have to have any preimage or second preimage resistance because we're assuming good faith.

[ilyavolodin]

@michaelficarra Is correct. We are not using MD5 for anything related to security. We are using them to track changed files. We would definately not want to maintain MD5 implementation as part of the repository, so we are talking about external package, unless it can be done in a few lines of code. I'm not sure it's worth spending too much time on implementation yet. Compiling node without SSL feels like a very much an edge case to me, so if performance is going to suffer or we will have to add large dependency, it might not be something we would want to do. Having all your users pay a tax for a few edge cases might not be the best idea. I assume there are already existing MD5 JavaScript NPM packages out there, so maybe just comparing performance of calculating a hash with one of them vs. the method we are using now would give us enough understanding of how we want to proceed forward.

[michaelficarra]

A quick search turned up this very promising package which implements the MurmurHash hashing algorithm: https://www.npmjs.com/package/imurmurhash

Claims sub-millisecond performance.

[nzakas]

We need to be careful about changing the hashing algorithm, as people who currently use caching will already have a cache file with hashes stored. All it takes is one collision to mess up the the caching functionality, so sticking to md5 is preferred.

@jbergstroem what's the use case you're trying to address?

[michaelficarra]

@nzakas Changing the hashing algorithm won't cause a collision. Worst case (and expectedly) it will cause a cache miss for every file one time.

[ilyavolodin]

I think @michaelficarra is correct. The first time you run ESLint after new hashing algorithm will be introduced, it will basically just lint every single file (because new hashes will not match old ones, so our code will think that files were modified). After that, everything should go back to normal.

[jbergstroem]

@nzakas if you check the nodejs issue reference in the first comment you can see that you run into issues if you use a nodejs built without openssl (crypto not being available).

@ilyavolodin perhaps check for openssl support (we check for process.versions.openssl in our test suite) and only add md5 for those that lacks it?

In general I'm happy to change hashing algorithm but since I haven't done any benchmarking I feel that consistency with current implementation is more important.

[nzakas]

@jbergstroem yes, my ask is who is this affecting and why are they using Node without openssl? Is this just something happening in the course of Node development or is there something else going on?

Also, this only affects one feature (caching results), so what about just not using caching?

In short, I don't see enough information in this issue to rationalize swapping in a custom hashing implementation. I'd like a better description of the use case before we consider solutions.

[jbergstroem]

@nzakas building without ssl support is commonly done to achieve small binaries (think limited architectures). I can't really provide numbers here but my rationale was that if the usage of crypto in eslint is so limited, why just not swap it out?

[nzakas]

@jbergstroem similarly, why not just avoid using the one feature that uses crypto?

Again, I'd like more information about what you're doing, what error you're seeing, etc. Full console output would be very helpful. I'd like to refer you to our bug reporting guidelines: http://eslint.org/docs/developer-guide/contributing/reporting-bugs

[jbergstroem]

Strange. I swear I replied yesterday. Anyway, I'll write it again.

Apologies for now following your guidelines. Here's the error I'm seeing, trying to use eslint from node with a binary built with the --without-ssl flag:

bash-3.2$ make lint
./node tools/eslint/bin/eslint.js benchmark lib src test tools/doc \
          tools/eslint-rules --rulesdir tools/eslint-rules
crypto.js:17
  throw new Error('Node.js is not compiled with openssl crypto support');
  ^

Error: Node.js is not compiled with openssl crypto support
    at crypto.js:17:9
    at NativeModule.compile (node.js:956:5)
    at Function.NativeModule.require (node.js:900:18)
    at Function.Module._load (module.js:299:25)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at Object.<anonymous> (/Users/james/Node/main/node/tools/eslint/lib/cli-engine.js:40:14)
    at Module._compile (module.js:417:34)
    at Object.Module._extensions..js (module.js:426:10)
    at Module.load (module.js:357:32)
make: *** [jslint] Error 1

You can find a full discussion from the nodejs point of view at nodejs/node#5610.

[alberto]

We could also disable caching if there is no ssl support.

[ilyavolodin]

@alberto I think the issue is not running eslint with caching, it's that we are using crypto = require( "crypto" ) on top of the cli-engine. So if you have node compiled without SSL, that line will fail, whether you use caching or not. We could make that require conditional, and then everything should pass.

[alberto]

Yeah, I didn't look at the implementation, but that's what I really meant :)

[ilyavolodin]

@jbergstroem Would a simple change of making require("crypto") conditional solve your problem?

[jbergstroem]

@ilyavolodin: sure; conditional on what? Disable caching? Sounds like a fair tradeoff

[ilyavolodin]

Yes, conditional on not passing --cache flag. That way, everything except for caching is going to work fine without SSL. I think that's probably a good middle ground solution.

[michaelficarra]

Or we could just use a faster hashing algorithm and allow these users to continue using the --cache flag. Win-win?

[BYK]

👍 to @michaelficarra

[platinumazure]

Yeah, I also don't see why we can't take in a package that will improve the experience for everyone when it's not even that large. 👍 to @michaelficarra

[michaelficarra]

If it helps at all, I'd be willing to do the work.

edit: PR at #5560.