chore: fix `lzma-native` build issues on Windows #1191

jviotti · 2017-03-18T07:09:52Z

We've been recently hitting a weird lzma-native build error on Windows
(both locally and on Appveyor CI):

Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  build
  The input line is too long.

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 1. [C:\projects\etcher\node_modules\lzma-native\build\liblzma.vcxproj]

After a lot of experimentation, we realised the issue was gone if we
removed node-sass from the development dependencies.

The issue is that node-gyp was recently upgraded to v3.6.0, which was
picked up by node-sass, which declares node-gyp as a dependency. For
some reason, if node-sass causes node-gyp to be updated, then
lzma-native fails with the above cryptic error.

I was able to trace down the error to the following node-gyp commit:

nodejs/node-gyp@ae141e1

As a solution, this commit starts to shrinkwrap development
dependencies, and locks node-gyp to v3.5.0 until the issue is fixed.

Fixes: addaleax/lzma-native#30
See: nodejs/node-gyp#1151
Signed-off-by: Juan Cruz Viotti jviotti@openmailbox.org

lurch · 2017-03-18T10:39:51Z

Have you created an issue against the node-gyp repo?
Should https://github.com/resin-io/etcher/blob/master/docs/CONTRIBUTING.md be updated to explain the --dev flag that you talk about here ?

Great to see the CI tests working again!

lurch · 2017-03-18T10:43:52Z

npm-shrinkwrap.json

    "mute-stream": {
      "version": "0.0.5",
      "from": "mute-stream@0.0.5",
      "resolved": "https://registry.npmjs.org/mute-stream/-/mute-stream-0.0.5.tgz"
    },
-    "nan": {


I was concerned that some modules have been removed from the shrinkwrap, but it's actually just github's diff-display getting confused ;-)

lurch · 2017-03-18T14:30:06Z

...and when we next need to update the shrinkwrap file, how do we ensure that node-gyp gets locked to v3.5.0 ?

We've been recently hitting a weird `lzma-native` build error on Windows (both locally and on Appveyor CI): ``` Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch. build The input line is too long. C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 1. [C:\projects\etcher\node_modules\lzma-native\build\liblzma.vcxproj] ``` After a lot of experimentation, we realised the issue was gone if we removed `node-sass` from the development dependencies. The issue is that `node-gyp` was recently upgraded to v3.6.0, which was picked up by `node-sass`, which declares `node-gyp` as a dependency. For some reason, if `node-sass` causes `node-gyp` to be updated, then `lzma-native` fails with the above cryptic error. I was able to trace down the error to the following `node-gyp` commit: nodejs/node-gyp@ae141e1 As a solution, this commit starts to shrinkwrap development dependencies, and locks `node-gyp` to v3.5.0 until the issue is fixed. Fixes: addaleax/lzma-native#30 See: nodejs/node-gyp#1151 Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

jviotti · 2017-03-20T03:37:54Z

Have you created an issue against the node-gyp repo?
Should https://github.com/resin-io/etcher/blob/master/docs/CONTRIBUTING.md be updated to explain the --dev flag that you talk about here ?

Good catch! Done!

...and when we next need to update the shrinkwrap file, how do we ensure that node-gyp gets locked to v3.5.0 ?

I just tried to upgrade node-sass to the latest version, and since node-gyp is being independently locked, it remains at v3.5.0 on the shrinkwrap file! We should still keep an eye during code reviews to ensure the dependency is not manually upgraded.

lurch · 2017-03-20T10:48:28Z

scripts/build/dependencies-npm.sh

+function run_install() {
+  npm install $INSTALL_OPTS
+
+  # Turns out that if `npm-shrinkwrap.json` contains development


Only a small nit, but could this comment be moved inside the if-block?

lurch · 2017-03-20T10:53:49Z

docs/CONTRIBUTING.md

@@ -56,7 +56,7 @@ necessary dependencies get added is to run the following commands:
 ```sh
 make electron-develop
 npm prune --production


Do you also need to remove --production from this line?

I"m removing this whole section, which described an approach to avoid development dependencies from being shrinkwrapped, which we are doing now.

lurch · 2017-03-20T11:04:54Z

scripts/clean-shrinkwrap.js

@@ -25,7 +25,7 @@ const EXIT_CODES = require('../lib/shared/exit-codes');
 const SHRINKWRAP_PATH = path.join(__dirname, '..', 'npm-shrinkwrap.json');

 try {
-  console.log(childProcess.execSync('npm shrinkwrap', {
+  console.log(childProcess.execSync('npm shrinkwrap --dev', {
    cwd: path.dirname(SHRINKWRAP_PATH)
  }));


I think you need to change this to }).toString()); ? Otherwise I just get <Buffer and a load of hex-digits printed to the screen.

And I think this script might need to be using npm prune too? In fact, could the CONTRIBUTING.md instructions be updated to use only this script (even if that means modifications are needed to this script also), as currently it's unclear in the instructions when to use npm run clean-shrinkwrap and when to use npm shrinkwrap --dev.

...and when I run this on my local machine (which currently has npm 3.10.10) it adds a load of "dev": true entries into my npm-shrinkwrap.json, which aren't in the shrinkwrap file in this PR? :-/

e.g.

diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 5556a28..f7916e9 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -5,61 +5,72 @@ "abbrev": { "version": "1.1.0", "from": "abbrev@>=1.0.0 <2.0.0", - "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.0.tgz" + "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.0.tgz", + "dev": true }, "acorn": { "version": "4.0.11", "from": "acorn@>=4.0.3 <5.0.0", - "resolved": "https://registry.npmjs.org/acorn/-/acorn-4.0.11.tgz" + "resolved": "https://registry.npmjs.org/acorn/-/acorn-4.0.11.tgz", + "dev": true }, "acorn-jsx": {

Regarding this scripts, I think the complexity of this script lies in the fact that it call npm shrinkwrap itself, when its not really its responsibility. Now that we're shrinkwrapping development dependencies, we shouldn't need to call npm shrinkwrap ourselves (npm will do it automatically for us whenever we do npm install, npm uninstall, etc).

I edited the script to only scans the current shrinkwrap file and remove the optional dependencies.

Regarding the dev: true properties, it seems that they are being added by a new npm version (mine is a bit older than yours). I'll upgrade and get those changes added here.

lurch · 2017-03-20T11:06:21Z

We should still keep an eye during code reviews to ensure the dependency is not manually upgraded.

Sounds like a job for another CI bash script. Give me 10 minutes... ;-)

lurch · 2017-03-20T11:29:57Z

...I've added scripts/ci/ensure-npm-shrinkwrap-versions.sh to make sanity-checks :-)

jviotti · 2017-03-20T16:09:03Z

Amazing stuff! :D

jviotti · 2017-03-20T16:19:13Z

@lurch Updated!

refack · 2017-03-20T17:12:12Z

Hello from nodejs/node-gyp#1151,

It's not a "length of command line" issue, it's a "not run from VS console" issue.
The lzma-native build calls lib.exe which is not in the path unless vsvarsall.bat (or it's derivatives) is run.
Up till node-gyp:v3.5.x the whole thing would fail. Now we can find & call msbuild.exe in the general case, but then we fail if the build is complicated ;(

I'm working on a better workflow, but till then you'll need to setup VC Vars...
Anyway I just found out that lzma-native:2.0.0 uses pre-build binaries, maybe that will solve this?

jviotti · 2017-03-20T17:40:28Z

Hi @refack ,

Thanks for reaching out.

I'm working on a better workflow, but till then you'll need to setup VC Vars...

Correct me if I'm wrong (I'm not a Windows expert), but this means that we should run vsvarsall.bat before compiling, right? If so, we're using the Visual Studio command prompt, which does that already AFAIK. In any case, I tried re-running the script before building the module, and it fails with the same error.

Anyway I just found out that lzma-native:2.0.0 uses pre-build binaries, maybe that will solve this?

We were using pre-build binaries at some point, but decided to build ourselves after hitting an ABI issue after an electron upgrade (better to make sure they are always on sync by compiling ourselves)

refack · 2017-03-20T17:41:46Z

Sorry, I was too hasty. it's not a vcvarsall.bat error.
It's a bug in GYP
nodejs/node-gyp#1151 (comment)

addaleax · 2017-03-20T17:44:15Z

Anyway I just found out that lzma-native:2.0.0 uses pre-build binaries, maybe that will solve this?

We were using pre-build binaries at some point, but decided to build ourselves after hitting an ABI issue after an electron upgrade (better to make sure they are always on sync by compiling ourselves)

Btw, earlier versions of the library also have pre-buit binaries available. But fwiw, if there’s any help I can offer by providing more pre-built binaries, I’m glad to look into that. (It would be really cool to know which electron versions to look out for in that case, though.)

jviotti · 2017-03-20T17:51:00Z

Hi @addaleax

Thank you very much the good will. If you want to ensure lzma-native pre compiled binaries work out of the box for Electron project, I guess you could keep an eye on Electron releases (https://github.com/electron/electron/releases) and publish as soon as Electron publishes a new version that makes use of a new node ABI.

In any case, I don't think its reasonable for you to invest time into that. Compiling the module just takes a few seconds from our side, and allows you to not have to worry about being on top of nodejs releases all the time.

lurch · 2017-03-20T18:04:38Z

scripts/build/dependencies-npm.sh

  if [ "$ARGV_PRODUCTION" == "true" ]; then
+
+    # Turns out that if `npm-shrinkwrap.json` contains development
+    # dependencies then `npm install --production` will also install


Does this comment imply that when "$ARGV_PRODUCTION" == "true" we ought to be adding --production to $INSTALL_OPTS?

(I guess there's a slim chance that npm install --production might get fixed in future, and in that case npm install --production; npm prune --production will probably be quicker than npm install; npm prune --production?)

I mean that when doing npm install --production, npm will also install the development dependencies (I know, npm is messed :P), which is the reason why we just do npm install + npm prune --production.

(I guess there's a slim chance that npm install --production might get fixed in future, and in that case npm install --production; npm prune --production will probably be quicker than npm install; npm prune --production?)

Fair enough. Lets add it in case it gets fixed in the future :)

lurch · 2017-03-20T18:07:18Z

docs/CONTRIBUTING.md

-npm prune --production
-npm shrinkwrap --dev
-```
-


Ahh, much cleaner 👍

lurch · 2017-03-20T18:09:34Z

OTOH if pre-compiled versions of lzma-native did work with all versions of Electron, would it allow us to remove the Visual Studio C++ requirement from the RUNNING-LOCALLY doc?

jviotti · 2017-03-20T18:13:15Z

@lurch We will have various more native dependencies very soon:

mountutils
drivelist (at least for Windows due to the WMI size bug)
Some module to do Windows lock/unlocking, etc

so I wouldn't try to remove the Visual Studio dependency.

Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

lurch

🎉 😄

refack · 2017-03-21T02:14:28Z

Found the bug in node-gyp

jviotti · 2017-03-21T15:11:12Z

Amazing stuff, thanks a lot @refack ! Do you mind letting us know when you release a new version, so we can upgrade to it?

This doesn't appear to be an issue anymore. Connects To: #1191

jviotti added the in progress label Mar 18, 2017

jviotti mentioned this pull request Mar 18, 2017

Sudden Windows build failures addaleax/lzma-native#30

Closed

lurch reviewed Mar 18, 2017

View reviewed changes

jviotti mentioned this pull request Mar 20, 2017

Weird error MSB6006: "cmd.exe" exited with code 1 on some packages nodejs/node-gyp#1151

Closed

jviotti force-pushed the fix-lzma-native-windows-builds branch from bf5044d to 839ff07 Compare March 20, 2017 03:31

jviotti force-pushed the fix-lzma-native-windows-builds branch from 839ff07 to e3a4bd2 Compare March 20, 2017 03:34

lurch reviewed Mar 20, 2017

View reviewed changes

chore: ensure some modules in npm-shrinkwrap stay at specific versions

701d38f

lurch reviewed Mar 20, 2017

View reviewed changes

Address code review comments

1026b15

Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>

jviotti force-pushed the fix-lzma-native-windows-builds branch from 902189a to 1026b15 Compare March 20, 2017 18:30

lurch approved these changes Mar 20, 2017

View reviewed changes

jviotti merged commit 0873b1d into master Mar 20, 2017

jviotti deleted the fix-lzma-native-windows-builds branch March 20, 2017 19:03

jviotti removed the in progress label Mar 20, 2017

lurch mentioned this pull request Jun 14, 2017

chore(ci): Use npm@5 on CI services #1487

Closed

8 tasks

jhermsmeier added a commit that referenced this pull request Oct 16, 2017

chore(scripts): Remove hard dep on node-gyp 3.5.0

0bbc16e

This doesn't appear to be an issue anymore. Connects To: #1191

jhermsmeier added a commit that referenced this pull request Oct 19, 2017

chore(scripts): Remove hard dep on node-gyp 3.5.0

bfbea08

This doesn't appear to be an issue anymore. Connects To: #1191

jhermsmeier added a commit that referenced this pull request Oct 23, 2017

chore(scripts): Remove hard dep on node-gyp 3.5.0

ccd7215

This doesn't appear to be an issue anymore. Connects To: #1191

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix `lzma-native` build issues on Windows #1191

chore: fix `lzma-native` build issues on Windows #1191

jviotti commented Mar 18, 2017 •

edited

Loading

lurch commented Mar 18, 2017

lurch Mar 18, 2017

lurch commented Mar 18, 2017

jviotti commented Mar 20, 2017

lurch Mar 20, 2017

jviotti Mar 20, 2017

lurch Mar 20, 2017

jviotti Mar 20, 2017

lurch Mar 20, 2017

lurch Mar 20, 2017

jviotti Mar 20, 2017

jviotti Mar 20, 2017

lurch commented Mar 20, 2017

lurch commented Mar 20, 2017

jviotti commented Mar 20, 2017

jviotti commented Mar 20, 2017

refack commented Mar 20, 2017

jviotti commented Mar 20, 2017

refack commented Mar 20, 2017

addaleax commented Mar 20, 2017

jviotti commented Mar 20, 2017

lurch Mar 20, 2017

jviotti Mar 20, 2017

jviotti Mar 20, 2017

lurch Mar 20, 2017

lurch commented Mar 20, 2017

jviotti commented Mar 20, 2017

lurch left a comment

refack commented Mar 21, 2017

jviotti commented Mar 21, 2017

chore: fix lzma-native build issues on Windows #1191

chore: fix lzma-native build issues on Windows #1191

Conversation

jviotti commented Mar 18, 2017 • edited Loading

lurch commented Mar 18, 2017

Choose a reason for hiding this comment

lurch commented Mar 18, 2017

jviotti commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lurch commented Mar 20, 2017

lurch commented Mar 20, 2017

jviotti commented Mar 20, 2017

jviotti commented Mar 20, 2017

refack commented Mar 20, 2017

jviotti commented Mar 20, 2017

refack commented Mar 20, 2017

addaleax commented Mar 20, 2017

jviotti commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lurch commented Mar 20, 2017

jviotti commented Mar 20, 2017

lurch left a comment

Choose a reason for hiding this comment

refack commented Mar 21, 2017

jviotti commented Mar 21, 2017

chore: fix `lzma-native` build issues on Windows #1191

chore: fix `lzma-native` build issues on Windows #1191

jviotti commented Mar 18, 2017 •

edited

Loading