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

eslint broken when node built without openssl #5610

Closed
jasnell opened this issue Mar 8, 2016 · 13 comments
Closed

eslint broken when node built without openssl #5610

jasnell opened this issue Mar 8, 2016 · 13 comments
Labels
tools Issues and PRs related to the tools directory.

Comments

@jasnell
Copy link
Member

jasnell commented Mar 8, 2016

When building Node.js with configure --without-ssl, make lint and make test error out with:

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
@jasnell jasnell added the tools Issues and PRs related to the tools directory. label Mar 8, 2016
@vkurchatkin
Copy link
Contributor

Why do we use ./node to run it? It can be broken at any point, doesn't it make sense to use global node which is known to be stable?

@jasnell
Copy link
Member Author

jasnell commented Mar 8, 2016

That would assume that the machine has a global node installed, which may not be the case.

@jbergstroem
Copy link
Member

I don't think we can make an assumption about a global node.

They seem to use md5 from crypto:

/**
 * create a md5Hash of a given string
 * @param  {string} str the string to calculate the hash for
 * @returns {string}    the calculated hash
 */
function md5Hash(str) {
    return crypto
        .createHash("md5")
        .update(str, "utf8")
        .digest("hex");
}

Perhaps just file a PR upstream with a javascript version of md5?

@vkurchatkin
Copy link
Contributor

@jasnell it should be a requirement, then.

@Fishrock123
Copy link
Member

@vkurchatkin I disagree, everything else works without a global node and iirc that is how the CI runs. We should attempt to make it run without that.

@vkurchatkin
Copy link
Contributor

@Fishrock123 the idea is that node compiled without OpenSSL is kind of broken: a lot of modules won't work. We can make eslint work by sticking js implementation of md5, but it is not a good long term solution:

  • updates (including dependencies) can break it again
  • ./node can be broken in many other ways that could prevent eslint and other tools from running correctly

@jbergstroem
Copy link
Member

@vkurchatkin if its broken we shouldn't support it.

The test suite currently passes (albeit with skipped crypto tests) and I don't think we should remove that build option.

@vkurchatkin
Copy link
Contributor

@jbergstroem it's broken in a sense that it's not "node" platform, it's "node-without-ssl" platform, and eslint doesn't support running on "node-without-ssl". Using js implementation of md5 is not going to change that.

@jbergstroem
Copy link
Member

@vkurchatkin ok; lib/cli-engine was the only occurrence I could find. Got more info about eslint not supporting being run on "node-without-ssl"?

@vkurchatkin
Copy link
Contributor

@jbergstroem by supporting "node-without-ssl" I mean running tests on it as a part of CI. It's very unlikely that they would do this (nobody does)

@jasnell
Copy link
Member Author

jasnell commented Mar 9, 2016

The other option here is to simply disable make lint and linting during make test when openssl is not available.

@jbergstroem
Copy link
Member

FWIW, in CI we run nodejs from FreeBSD's pkg. I'd still argue that the tradeoff for eslint to support non-ssl nodejs is so small they (I) should patch (PR) it.

@jbergstroem
Copy link
Member

Fixed with #6132.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

No branches or pull requests

4 participants