Dredd: Native compilation failure of optional dep makes installation super slow

[ianwremmel]

Describe the bug
Having Dredd present in a project using Node 10 leads to an attempt to natively compile protagonist (or drafter, or snowcrash, or something, I'm not quite sure which is actually the culprit). For reasons not entirely clear to me, this native compile is attempted not just when installing dredd, but when installing other modules (maybe anytime the dredd tree in package-lock gets touched?)

The issue seems to be fixed in protagonist but Dredd needs to update to consume it.

To Reproduce
Use Node 10. Run npm install dredd. You'll see a very long install time with a failed native compile (but a zero exit code).

Expected behavior
Installs go quickly because the native module (which is an optional dependency) cannot install.

What is in your dredd.yml?
Not Relevant
What's your dredd --version output?

Same thing happens when installing 5.3.0 on heroku and on debian

dredd v5.3.0 (Darwin 18.2.0; x64)

Does dredd --level=debug uncover something?
Not relevant

Can you send us failing test in a Pull Request?
Not Applicable

[honzajavorek]

Hi @ianwremmel, the API Blueprint parser (let's refer to all the projects with this name to make it easier) is delivered in two forms - a compiled parser written in C++, and an equivalent, slower, but more portable JS parser compiled from the same C++ code.

It's true the parser doesn't support Node 10 correctly in the version Dredd uses, but even if the compilation fails, the parser should work correctly as there's fallback to the JS form. Making the compilation pass won't speed-up the installation though, it only speeds up Dredd's parsing of API Blueprint documents.

I'd say that what you really want (and I really want as well) is to get https://github.com/apiaryio/drafter-npm/pull/16 finished so that Dredd installs with the JS form by default, which avoids any compilation when managing the lifecycle of your project with npm, and so that the C++ form is an opt-in for deployments of Dredd where speed of parsing of API Blueprint documents is more important than the speed of installation.

See also docs on this topic. Pinging @apiaryio/adt so they know this is still an issue for Dredd users and we should prioritize it.

[ianwremmel]

Well, it slows down installation on Heroku or Circle CI or anywhere that uses npm ci because that seems to always try to recompile the native module. The fact that the native compile fails means that even with a hot cache, my npm install phase burns two minutes per build/release.

Locally, I've found installing other packages often causes npm to try to compile the parser, thus burning a minute or two on a process that will definitely fail.

[ianwremmel]

Note: I'm talking about situations where dredd is a dev dependency. I suppose I could go the global install route to get around this issue, but that tends to have its own set of brittleness and I think I saw somewhere that npm has begun discouraging global installs.

[honzajavorek]

So @kylef suggested we could use npm-shrinkwrap.json instead of package-lock.json. npm respects it in distributed packages and recommends it for things like CLI tooks. We could craft it so it doesn't contain protagonist (the compiled part) at all. It would remove some performance in parsing large API Blueprint files, but my hunch is the installation performance of Dredd is a bigger issue, hurting everyone.

I'd consider it a sort of a temporary hack though, as I still think the API Elements tooling should have a way to opt-in to compiling instead of opting-out by --no-optional. OTOH AFAIK opt-in is not an easy workflow to achieve with current npm's capabilities.

shrinkwrap docs

• https://docs.npmjs.com/files/shrinkwrap.json
• https://docs.npmjs.com/files/package-locks

postshrinkwrap

[ianwremmel]

Are you proposing this as a change to the published dredd artifact or a change to my project?

[honzajavorek]

To the published Dredd artifact.

[kylef]

I think it needs a proof of concept to see if it works as intended. Long-term we are working on a WASM version of our API Blueprint parser. It should offer better performance than drafter.js (transpiled parser), it isn't clear how the performance metrics will compare to Protagonist. If it is negligable (I think there is a chance it may be) then I would consider us removing Protagonist from our API Elements API Blueprint adapter so that most consumers won't know about it.

We may still need that little edge in performance for some of our internal services where performance is a concern, and in that case I think it can become an opt-in parser. We can alter the API for the API Blueprint API Elements adapter so that a user can inject a version of the parser into it when it has different requirements or use Protagonist directly. One concern that I have with this is that we won't have semantic version locking between the adapter and parser and it can be tricky to manage manually especially when a consumer might not depend on a specific version and that running npm install one day can bring a broken experiance to a once working environment. For example "does version 1.2 of Protagonist work with the API Elements Adapter 2.3 etc". There is a feature request to NPM which can solve this for us: https://github.com/npm/npm/issues/20646

Unfortunately the WASM build is a little behind and it is blocked by a larger rollout of an entirely new version of the API Blueprint parser so it is going to be a little while before we can finish this off, I am hoping early next year. So I would persue removing Protagonist via the npm-shrinkwrap for an interim solution.

[mdular]

This would be a great relief, we started getting random timeouts on our CI / build server. The node-gyp compile attempt (which fails) adds 3-4 minutes to our build pipeline on each run on a medium-sized ec2 instance.

I would even suggest going for the temporary workaround (shrinkwrap file to avoid protagonist) proposed by @honzajavorek , even if it isn't the final & elegant solution.

It's still better than seeing the below output in the build logs each run (and losing the time):

> [email protected] install node_modules/protagonist
> node-gyp rebuild

...

4 warnings and 1 error generated.
make: *** [Release/obj.target/protagonist/src/parse_async.o] Error 1
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:262:23)
gyp ERR! stack     at ChildProcess.emit (events.js:189:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
gyp ERR! System Darwin 18.2.0
gyp ERR! command "/usr/local/Cellar/node/11.4.0/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd node_modules/protagonist
gyp ERR! node -v v11.4.0
gyp ERR! node-gyp -v v3.8.0
gyp ERR! not ok 
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules/protagonist):
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] install: `node-gyp rebuild`
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: Exit status 1

[kylef]

@mdular If you are explicitly installing Dredd in CI, you can skip optional dependencies if that helps you work around this quicker http://dredd.org/en/latest/installation.html#installing-c-dependencies

$ npm install dredd --global --no-optional

[mdular]

@kylef thanks for the response.

I've tried that without the --global flag, but it still attempts the install. --global install is not an option in the way we have currently set up our build pipeline (we are using portable node binaries downloaded via an install script and never installing node via the operating system package manager).

[honzajavorek]

@mdular If you have Dredd as a local (dev)dependency, then you can install it with --no-optional and npm shouldn't compile. But then npm _will_ try to compile every time you'll be manipulating with the package. There's no simple solution to that - there's no .npmrc setting to add --no-optional to all npm calls. The only workaround at this moment is to add --no-optional manually to all further npm commands manipulating with the package. See also the note here.

[ianwremmel]

I came up with a solution that seems to work tolerably:

1. remove dredd from dev dependencies
2. add a step to my circle ci config that installs dredd globally with the following command
   npm install -g dredd --no-optional --unsafe-perm
3. I was already running dredd through an npm script alias, so I changed the alias to:
   npx dredd

Wrapping npx means it'll prefer the already install global version in CI, but if I've forgotten to install dredd on my machine, it'll still install it before running.

[realityking]

It’s not a trivial task but have you considered moving protagonist to N-API? It would allow either shipping the compiled binaries for the most relevant architectures in the archive or, something like bode-pre-gyp, downloading them from an S3 bucket. The great thing about N-API is that it would cut down severely on the number of binaries to build for this. It also wouldn’t require recompiling when updating to a newer Node.js version.

[kylef]

@realityking I'd explore how WASM performs first as that has the potential to make it so most consumers don't even need to worry about Protagonist / compilation at all. Even if we ship binaries of Protagonist the end user will still need a moden C++ runtime which can be problematic. Drafter 4 brings more modern compiler requirements over (which isn't released yet into Dredd and would likely make this problem worse).

As for N-API, a lot of the APIs are only available in newer versions of Node (8, 10 etc) whereas we support all the LTS releases at the moment. N-API stability only came in Node 10, we're still supporting Node 6 in Dredd, and given Node 8 doesn't reach end of life until December 2019 I don't think we will be dropping support for Node < 10 until then (please correct me if I am wrong @honzajavorek).

I do think we should experiment with N-API at some point, but I think it is a bit too early as we support Node LTS. I'd be interested to see how it performs compared to native (current implementation of Protagonist) and WASM. Some benchmarks have seen WASM can outperform both native and N-API.

[honzajavorek]

Given Node 8 doesn't reach end of life until December 2019 I don't think we will be dropping support for Node < 10 until then

Yes, that's the rule now. We would have to have really good reasons to break it and personally I'd prefer to support Node 6 as long as possible. People migrated to Node 6 quite fast from Node 4, to get the ES2015+ features, but I don't see everyone abandoning it now in the same hurry as there are usually no really good reasons and it is no pain to have the codebase to still support Node 6. (I might be wrong in my observations, of course, happy to hear other opinions.)

[realityking]

How flexible are you on the minor version? Node 6.14.2 does support N-API.

[honzajavorek]

Leaving it up to @apiaryio/adt to try N-API and to decide around that. The discussion should probably move to Drafter.js issues.

Regarding versions, Dredd itself is flexible I guess. I'd say Dredd should support "the latest version of the major version".

I'm going to remove Protagonist from Dredd by shrinkwrap as a workaround. The performance benefit does not overweight the installation and distribution downsides. I should have done it way earlier already. Hopefully I can get to issue the PR during today.

[ApiaryBot]

:tada: This issue has been resolved in version 5.4.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: