chore: don't remove optional dependencies in clean-shrinkwrap.js #1551

jviotti · 2017-06-27T16:22:02Z

If we include a platform specific optional dependency in the shrinkwrap
file, then npm will insist in installing it even if the platform doesn't
match. As a solution, we figured out we can avoid putting this platform
specific optional dependencies in the npm-shrinkwrap.json file.

In order to do this, we currently have a script called
clean-shrinkwrap.js that runs before any npm shrinkwrap file (its
a preshrinkwrap npm script) that deletes all the platform specific
modules we know about using npm rm.

The problem with this approach is that npm rm will remove the module's
code from node_modules, which means that if we run npm shrinkwrap,
we will lose certain optional dependencies, that may be needed at a
later stage.

The solution is to modify the clean-shrinkwrap.js script to parse
npm-shrinkwrap.json, and manually delete the entries that we want to
omit. Also, the script needs to be run after npm shrinkwrap, so we
change the npm script name to postshrinkwrap.

Signed-off-by: Juan Cruz Viotti [email protected]

jviotti · 2017-06-27T16:23:04Z

Let's see what the CI servers say. Fingers-crossed!

jviotti · 2017-06-27T17:12:57Z

YESS, finally everything works \o/

jviotti · 2017-06-27T20:39:08Z

What do you think @jhermsmeier ?

jhermsmeier

I haven't parsed this fully yet, but looks good to me so far, just a few nits.

jhermsmeier · 2017-06-27T16:33:07Z

scripts/clean-shrinkwrap.js

+  ], getDependencyTree(shrinkwrapFile, dependencyPath)));
+}, []);
+
+const JSON_INDENTATION_SPACES = 2;


Can we declare this at the very top?

jhermsmeier · 2017-06-27T20:43:03Z

scripts/clean-shrinkwrap.js

+ * @returns {Object} shrinkwrap object
+ *
+ * @example
+ * const object = getShrinkwrapDependencyObject(require('npm-shrinkwrap.json'), [


The required path needs a ./ to understand that it's a local path afaik – might be good to have that in the example as well?

jhermsmeier · 2017-06-27T20:48:44Z

scripts/clean-shrinkwrap.js

+ * @example
+ * const dependencies = getTopLevelDependenciesFromShrinkwrapObject([ 'debug' ]);
+ */
+const getTopLevelDependenciesFromShrinkwrapObject = (shrinkwrapPath) => {


This is starting to feel like I'm reading Java 😝

Well, its JavaScript after all :P

From what I can tell, would getTopLevelDependenciesForShrinkwrapPath be a better name? *shrug*

jhermsmeier · 2017-06-27T20:50:11Z

package.json

+  "platformSpecificDependencies": [
+    [ "7zip-bin-mac" ],
+    [ "7zip-bin-win" ],
+    [ "7zip-bin-linux" ]


What's the reasoning behind the nested arrays here?

The new approach works based on shrinkwrap "paths", which we set using array syntax. Basically this means that we want to omit 7zip-bin-mac from the top level of the shrinkwrap file for example. If 7zip-bin-mac was present inside foo in the shrinkwrap file, we should add [ 'foo', '7zip-bin-mac' ].

jhermsmeier · 2017-06-28T09:24:42Z

scripts/clean-shrinkwrap.js

+ * @returns {Object} shrinkwrap object
+ *
+ * @example
+ * const object = getPrettyShrinkwrapDependencyObject(require('npm-shrinkwrap.json'), [


Another relative path missing in the example here

jhermsmeier · 2017-06-28T09:25:29Z

There's also a build failure on Windows:

[...]
mkdir dist/Etcher-cli-1.0.0+2a000ae-win32-x86-app
cp package.json dist/Etcher-cli-1.0.0+2a000ae-win32-x86-app
cp -RLf lib dist/Etcher-cli-1.0.0+2a000ae-win32-x86-app
cp -RLf dist/node-win32-x86-dependencies/* dist/Etcher-cli-1.0.0+2a000ae-win32-x86-app
cp: cannot copy cyclic symbolic link `dist/node-win32-x86-dependencies/node_modules/ajv/node_modules'
make: *** [dist/Etcher-cli-1.0.0+2a000ae-win32-x86-app] Error 1
Command exited with code 2

Is this a transient error, that only occasionally occurs?

lurch · 2017-06-28T12:30:07Z

cp: cannot copy cyclic symbolic link
Is this a transient error, that only occasionally occurs?

Yeah, it keeps popping up now and then, but only on Appveyor, and we've no idea why :-( When you restart the build (which I've just done), it usually passes on the second attempt.
(and annoyingly, unlike on Travis CI where you can restart just one arch-specific build, on Appveyor you have to restart all arch builds)

lurch

With all the hardcore hacking @jviotti has been doing on npm-shrinkwrap.json these past few months, I hope he doesn't get poached by the NPM project :-D

lurch · 2017-06-28T12:40:28Z

scripts/clean-shrinkwrap.js

+ * const object = getShrinkwrapDependencyObject(require('./npm-shrinkwrap.json'), [
+ *   'drivelist',
+ *   'lodash'
+ * ]);


Should these examples also show what the returned object might contain?

Sure, I'll add some more example code.

lurch · 2017-06-28T12:54:16Z

scripts/clean-shrinkwrap.js

+ * @private
+ *
+ * @description
+ * This function transforms the output of `getShrinkwrapDependencyObject()` to


minor nit: could you change transforms the output of to wraps ? The current description makes it sounds like getPrettyShrinkwrapDependencyObject takes the output of getShrinkwrapDependencyObject as an input parameter, which it doesn't.

lurch · 2017-06-28T12:56:11Z

scripts/clean-shrinkwrap.js

+ *
+ * @param {Object} shrinkwrap - the shrinkwrap file contents
+ * @param {String[]} shrinkwrapPath - path to shrinkwrap dependency
+ * @returns {Object} shrinkwrap object


Perhaps this should be @returns {Object} pretty shrinkwrap object?

lurch · 2017-06-28T13:01:38Z

scripts/clean-shrinkwrap.js

+ * @private
+ *
+ * @param {String[]} shrinkwrapPath - path to shrinkwrap dependency
+ * @returns {Object} dependency manifest


I've got no idea what a dependency manifest is - an example might be great ;)

EDIT: Ah, now I see that it's the package.json for that dependency :)

Haha, I clarified it anyways.

lurch · 2017-06-28T13:06:10Z

scripts/clean-shrinkwrap.js

+    .value();
+
+  try {
+    return require(`.${path.sep}${manifestPath}`);


And an inline example comment of what this manifestPath might look like would be useful too.

lurch · 2017-06-28T13:40:02Z

scripts/clean-shrinkwrap.js

+ * @example
+ * const shrinkwrapFile = require('./npm-shrinkwrap.json');
+ * const dependencyTree = getDependencyTree(shrinkwrapFile, [ 'drivelist' ]);
+ * const filteredShrinkwrap = removeDependencies(shrinkwrapFile, dependencyTree, {


I think this example is wrong, because getDependencyTree returns Object[] but the blacklist parameter here is String[] ?

Oops, the type declaration was indeed wrong. Thanks for the heads up!

lurch · 2017-06-28T13:42:51Z

scripts/clean-shrinkwrap.js

+  return _.compact(_.concat(accumulator, [
+    getPrettyShrinkwrapDependencyObject(shrinkwrapFile, dependencyPath)
+  ], getDependencyTree(shrinkwrapFile, dependencyPath)));
+}, []);


Should this chunk of code be moved into e.g. a buildBlacklistFromDependencyTree function or something?
I'm afraid it looks pretty impenetrable to a non-Lodash-expert ;)

Haha, what this does is basically taking every dependency in the black list, and for each of them, get an array of its dependencies + the dependency itself (hence _.concat). We then flatten the whole thing as we go by using a fold.

I guess we can rename the internal bit to something like "get dependency tree AND the dependency itself". Maybe that could be just getTree()?

lurch · 2017-06-28T13:45:52Z

scripts/clean-shrinkwrap.js

+ *
+ * @param {Object} shrinkwrap - the shrinkwrap file contents
+ * @param {String[]} blacklist - dependency blacklist
+ * @param {Object} qualifiers - qualifiers


Given the single-purpose nature of this script, I wonder if it'd be worth renaming removeDependencies to removeOptionalDevDependencies and then getting rid of the qualifiers parameter? *shrug*

Makes sense, done!

lurch · 2017-06-28T13:47:22Z

scripts/clean-shrinkwrap.js

+}), null, JSON_INDENTATION_SPACES);
+
+fs.writeFileSync(NPM_SHRINKWRAP_FILE_PATH, `${result}\n`);
+console.log('Done');


console.log('Done, thank f***!'); 😄

lurch · 2017-06-28T13:51:08Z

scripts/clean-shrinkwrap.js

+ * level object, remove the blacklisted dependencies, and recurse
+ * if possible.
+ *
+ * @param {Object} shrinkwrap - the shrinkwrap file contents


Does this parameter description need to be updated to reflect the recursive nature of this function? (or is that an internal-only detail?)

jviotti · 2017-06-28T14:47:31Z

Yeah, it keeps popping up now and then, but only on Appveyor, and we've no idea why :-( When you restart the build (which I've just done), it usually passes on the second attempt.
(and annoyingly, unlike on Travis CI where you can restart just one arch-specific build, on Appveyor you have to restart all arch builds)

It looks like a MinGW bug. Moving stuff using Node.js file-system works fine. We only use this for the CLI generation, which we will move to pkg, so it should be gone soon.

lurch · 2017-06-29T12:34:06Z

./node_modules/.bin/npx eslint lib tests scripts bin versionist.conf.js
C:\projects\etcher\scripts\clean-shrinkwrap.js
  232:16  error  'removeDependencies' is not defined  no-undef
✖ 1 problem (1 error, 0 warnings)

jviotti · 2017-06-29T13:57:36Z

Fixed!

lurch · 2017-06-29T15:59:15Z

> node ./scripts/clean-shrinkwrap.js
Removing: 7zip-bin-mac, 7zip-bin-win, 7zip-bin-linux
Done
There are unstaged npm-shrinkwrap.json changes. Please commit the result of:
    npm shrinkwrap --dev
diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
index 2c2240b..44d9863 100644
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -53,6 +53,13 @@
       "resolved": "https://registry.npmjs.org/7zip-bin/-/7zip-bin-2.1.0.tgz",
       "dev": true
     },
+    "7zip-bin-win": {
+      "version": "2.1.0",
+      "from": "7zip-bin-win@>=2.1.0 <3.0.0",
+      "resolved": "https://registry.npmjs.org/7zip-bin-win/-/7zip-bin-win-2.1.0.tgz",
+      "dev": true,
+      "optional": true
+    },
     "abbrev": {
       "version": "1.1.0",
       "from": "abbrev@>=1.0.0 <2.0.0",
warning: LF will be replaced by CRLF in npm-shrinkwrap.json.
The file will have its original line endings in your working directory.
make: *** [sanity-checks] Error 1
Command exited with code 2

> node ./scripts/clean-shrinkwrap.js
Removing: 7zip-bin-mac, 7zip-bin-win, 7zip-bin-linux
Done
There are unstaged npm-shrinkwrap.json changes. Please commit the result of:
    npm shrinkwrap --dev
diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json
index 2c2240b..dc1282c 100644
--- a/npm-shrinkwrap.json
+++ b/npm-shrinkwrap.json
@@ -53,6 +53,13 @@
       "resolved": "https://registry.npmjs.org/7zip-bin/-/7zip-bin-2.1.0.tgz",
       "dev": true
     },
+    "7zip-bin-linux": {
+      "version": "1.1.0",
+      "from": "7zip-bin-linux@>=1.1.0 <2.0.0",
+      "resolved": "https://registry.npmjs.org/7zip-bin-linux/-/7zip-bin-linux-1.1.0.tgz",
+      "dev": true,
+      "optional": true
+    },
     "abbrev": {
       "version": "1.1.0",
       "from": "abbrev@>=1.0.0 <2.0.0",
make: *** [sanity-checks] Error 1
The command "./scripts/ci/test.sh -o $HOST_OS -r $TARGET_ARCH" exited with 2.

😢

lurch · 2017-06-29T13:25:02Z

scripts/clean-shrinkwrap.js

+
+    // For example
+    // ./node_modules/drivelist/node_modules/package.json
+    return require(`.${path.sep}${manifestPath}`);


Why not return require(path.join('.', manifestPath)); ?

lurch · 2017-06-29T13:26:46Z

scripts/clean-shrinkwrap.js

 const path = require('path');
-const os = require('os');
 const packageJSON = require('../package.json');


As (I think) I asked before, why use '../package.json' here, but then path.join(__dirname, '..', 'npm-shrinkwrap.json') a few lines below?

We only need to require the package.json, however we need to shrinkwrap path in two places: a require call and the output of fs.writeFile, so better to extract it as a constant to avoid path issues (require() has special logic to understand ./ on Windows, but its not safe to assume that for fs.writeFile as well)

but its not safe to assume that for fs.writeFile as well)

Ahh, that's the bit I was missing, thanks. So require definitely works with both slash-types on Windows?

lurch · 2017-06-29T16:04:31Z

scripts/clean-shrinkwrap.js

+ * @example
+ * const shrinkwrapFile = require('./npm-shrinkwrap.json');
+ * const dependencyTree = getDependencyTree(shrinkwrapFile, [ 'drivelist' ]);
+ * const filteredShrinkwrap = removeDependencies(shrinkwrapFile, dependencyTree);


function-name in the example needs updating ;)

Really? The example still says removeDependencies whereas the function is named removeOptionalDevelopmentDependencies

I forgot to push 😆

lurch · 2017-06-29T16:13:34Z

scripts/clean-shrinkwrap.js

+  try {
+
+    // For example
+    // ./node_modules/drivelist/node_modules/package.json


Umm, should this be ./node_modules/drivelist/package.json ? Or maybe ./node_modules/drivelist/node_modules/lodash/package.json ?

Right, fixed!

jviotti · 2017-06-30T15:14:49Z

Looks like I made a couple of subtle issues while addressing the review comments. It should work now.

If we include a platform specific optional dependency in the shrinkwrap file, then npm will insist in installing it even if the platform doesn't match. As a solution, we figured out we can avoid putting this platform specific optional dependencies in the npm-shrinkwrap.json file. In order to do this, we currently have a script called `clean-shrinkwrap.js` that runs *before* any `npm shrinkwrap` file (its a `preshrinkwrap` npm script) that deletes all the platform specific modules we know about using `npm rm`. The problem with this approach is that `npm rm` will remove the module's code from `node_modules`, which means that if we run `npm shrinkwrap`, we will lose certain optional dependencies, that may be needed at a later stage. The solution is to modify the `clean-shrinkwrap.js` script to parse `npm-shrinkwrap.json`, and manually delete the entries that we want to omit. Also, the script needs to be run *after* `npm shrinkwrap`, so we change the npm script name to `postshrinkwrap`. Signed-off-by: Juan Cruz Viotti <[email protected]>

lurch · 2017-06-30T15:22:35Z

scripts/clean-shrinkwrap.js

+
+    // For example
+    // ./node_modules/drivelist/node_modules/lodash/package.json
+    return require(`.${path.sep}${manifestPath}`);


Given your comment about require in a comment just now, does this need to be .${path.sep} instead of just ./ ?

It would work. Since manifestPath is generated using path.join, it may include backslashes, so I decided to use path.sep just for consistency purposes (so we don't have paths like ./node_modules\package.json).

...so this line could also use path.join for consistency purposes? ;)

Yeah, I really wanted to, but:

> path.join('.', 'foo') 'foo'

(path.join omits the period, which require needs to work properly, so we need to preppend path.sep anyway)

Cool, thanks for the explanation! 👍

jviotti · 2017-07-03T14:30:00Z

Let's merge this to check if the logs are reduced enough to make Travis CI builds complete.

jviotti added the in progress label Jun 27, 2017

jviotti mentioned this pull request Jun 27, 2017

chore: don't remove optional dependencies in clean-shrinkwrap.js #1514

Closed

jviotti force-pushed the smart-shrinkwrap-traverse branch from 7d79302 to 6132500 Compare June 27, 2017 17:13

jhermsmeier reviewed Jun 27, 2017

View reviewed changes

jviotti force-pushed the smart-shrinkwrap-traverse branch from 6132500 to 2a000ae Compare June 27, 2017 22:05

jhermsmeier reviewed Jun 28, 2017

View reviewed changes

lurch reviewed Jun 28, 2017

View reviewed changes

jviotti force-pushed the smart-shrinkwrap-traverse branch from 2a000ae to adac8c2 Compare June 28, 2017 15:26

jviotti force-pushed the smart-shrinkwrap-traverse branch from adac8c2 to 0d9bad5 Compare June 29, 2017 13:57

lurch reviewed Jun 29, 2017

View reviewed changes

jviotti force-pushed the smart-shrinkwrap-traverse branch 2 times, most recently from e789a69 to 2cc7b1f Compare June 30, 2017 15:14

lurch reviewed Jun 30, 2017

View reviewed changes

jviotti force-pushed the smart-shrinkwrap-traverse branch from 2cc7b1f to 11f8127 Compare June 30, 2017 15:31

jviotti merged commit 53d8118 into master Jul 3, 2017

jviotti deleted the smart-shrinkwrap-traverse branch July 3, 2017 14:30

jviotti removed the in progress label Jul 3, 2017

chore: don't remove optional dependencies in clean-shrinkwrap.js #1551

chore: don't remove optional dependencies in clean-shrinkwrap.js #1551

Conversation

jviotti commented Jun 27, 2017

jviotti commented Jun 27, 2017

jviotti commented Jun 27, 2017

jviotti commented Jun 27, 2017

jhermsmeier left a 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

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

jhermsmeier commented Jun 28, 2017

lurch commented Jun 28, 2017

lurch left a 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

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

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

jviotti commented Jun 28, 2017

lurch commented Jun 29, 2017

jviotti commented Jun 29, 2017

lurch commented Jun 29, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviotti commented Jun 30, 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

jviotti commented Jul 3, 2017