Remove path-array by mhart · Pull Request #990 · nodejs/node-gyp

mhart · 2016-07-12T19:47:09Z

This reverts commits 81eadc5 and ff88e5f.

Of the 5.5MB footprint of node-gyp, path-array makes up 3.6MB.

Removing this reduces the weight of npm (and ultimately node) appreciably (by 20%)

This reverts commit ff88e5f. Of the 5.5MB footprint of node-gyp, path-array makes up 3.6MB. Removing this reduces the weight of npm (and thus node) appreciably.

This reverts commit 81eadc5.

rvagg · 2016-07-13T02:44:37Z

Does this actually work though? I was looking at this a while back because the dependency depth here bothers me too but I wasn't sure that the same functionality could be replicated quite as easily.

Perhaps it's really this easy but since we don't have test cases around this I'm not overly confident! We can certainly give it a try in v4 pre-releases which I'm planning on spinning up soon but tests would be nice. Any chance you'd like to write us some tests for this?

mhart · 2016-07-13T03:31:45Z

Sure – I mean, I believe it was only added by @TooTallNate originally because he thought it was a good fit for the path-array module – not because the original code, written by @timbertson was faulty in any way: https://github.com/nodejs/node-gyp/pull/509/files#r20182682

I just reverted those lines to how they were when #509 was merged.

Kinda seems like overkill to add a module for just a single string join, no?

mhart · 2016-07-13T03:34:18Z

-      var pypath = new PathArray(process.env, 'PYTHONPATH')
-      pypath.unshift(path.join(__dirname, '..', 'gyp', 'pylib'))
+      var pypath = process.env.PYTHONPATH ? [process.env.PYTHONPATH] : []
+      pypath.unshift(path.resolve(__dirname, '..', 'gyp', 'pylib'))


Ah, just noticed this should stay as path.join – will add that.

mhart · 2016-07-13T03:39:38Z

I'll add some tests tomorrow if I can figure out how 👍

bnoordhuis · 2016-07-13T03:40:26Z

-      var pypath = new PathArray(process.env, 'PYTHONPATH')
+      var pypath = process.env.PYTHONPATH ? [process.env.PYTHONPATH] : []
      pypath.unshift(path.join(__dirname, '..', 'gyp', 'pylib'))
+      process.env.PYTHONPATH = pypath.join(process.platform === 'win32' ? ';' : ':')


It's arguably a bit nicer to wrap it in a if ('PYTHONPATH' in process.env) { ... }.

Can you replace the process.platform === 'win32' ? ';' : ':' with win ? ';' : ':'?

When you say wrap in an if, what did you have in mind?

I can certainly do:

var pypath = 'PYTHONPATH' in process.env ? [process.env.PYTHONPATH] : []

Or would you prefer something like:

var pypath = [path.join(__dirname, '..', 'gyp', 'pylib')] if ('PYTHONPATH' in process.env) { pypath.push(process.env.PYTHONPATH) } process.env.PYTHONPATH = pypath.join(win ? ';' : ':')

(I kinda prefer that over an unshift anyway tbh)

I mean the second one. I wrote 'PYTHONPATH' in process.env but perhaps checking for truthiness / non-emptiness (i.e., just process.env.PYTHONPATH) is better.

var pypath = [path.join(__dirname, '..', 'gyp', 'pylib')] if (process.env.PYTHONPATH) pypath.push(process.env.PYTHONPATH) process.env.PYTHONPATH = pypath.join(win ? ';' : ':')

Preferences on indentation, braces?

node-gyp uses two spaces and (predominantly, I think) braces.

TooTallNate · 2016-07-13T03:46:37Z

It's only so big because of es6-symbol... I wonder if there's a smaller polyfill.

mhart · 2016-07-13T15:02:57Z

@rvagg added some tests 👍

This saves 3.6MB of cruft. See: nodejs/node-gyp#990

mhart · 2016-07-19T15:30:55Z

@rvagg let me know if those tests look ok, or if there's anything you need further to get this through

mhart · 2016-08-18T03:53:36Z

Hi all – any movement on this?

FWIW, I've been building a tool to track down size regressions (and other issues with sub-deps) – you can see an example of it coming to life here: http://pkgsize.com.s3.amazonaws.com/node-gyp.html (hovering over should show you what events caused certain size changes)

Obviously, path-array and request are the big offenders. Considering that path-array is literally just used for one string join here and has such a deep dep tree, it really will reduce the risk of even further bloat to remove it.

bnoordhuis · 2016-08-18T07:15:08Z

LGTM. CI: https://ci.nodejs.org/view/All/job/nodegyp-test-commit/12/

This probably also needs a CI run with v0.10 and v0.12.

mhart · 2016-08-31T17:02:49Z

@bnoordhuis would love to get this in – how do I get a CI run with v0.10 and v0.12?

bnoordhuis · 2016-09-02T09:08:55Z

https://ci.nodejs.org/view/All/job/nodegyp-test-commit/13/ - v0.10.46
https://ci.nodejs.org/view/All/job/nodegyp-test-commit/14/ - v0.12.15

@nodejs/build Can someone take a look at why the win2012r2 bot complains about this:

c:\workspace\nodegyp-test-commit\nodes\win2012r2\local_node>mkdir npm-cache 

c:\workspace\nodegyp-test-commit\nodes\win2012r2\local_node>call "c:\workspace\nodegyp-test-commit\nodes\win2012r2\local_node\npm.cmd" config set cache "c:\workspace\nodegyp-test-commit\nodes\win2012r2\local_node\npm-cache" 
The system cannot execute the specified program.

From https://ci.nodejs.org/view/All/job/nodegyp-test-commit/13/nodes=win2012r2/console.

bnoordhuis · 2016-09-02T09:10:38Z

The ubuntu buildbot seems pretty happy though so I think this is good to go. @mhart Can you squash it into what you think are logical/reasonable commits?

mhart · 2016-09-02T13:34:23Z

@bnoordhuis Sure thing – I'll keep the original "revert" commits separate and squash the rest

mhart · 2016-09-02T13:38:27Z

@bnoordhuis done – let me know if you want me to squash further

Using push instead of the original unshift makes the logic a little cleaner to read here.

joaocgreis · 2016-09-08T18:48:45Z

The job was trying to download node.exe from the wrong place, so it was getting a html page instead of a exe, hence the "cannot execute" error. Also, npm was interfering with the CITGM job. Both fixed.

Started two jobs with similar parameters:

Seems good!

mhart · 2016-09-20T15:01:29Z

Anything more need to be done on this? If so lemme know 👍

mhart · 2016-10-06T23:09:38Z

Hey all, just checking in on this PR. Let me know if there are any more concerns?

This reverts commit ff88e5f. Of the 5.5MB footprint of node-gyp, path-array makes up 3.6MB. Removing this reduces the weight of npm (and thus node) appreciably. PR-URL: #990 Reviewed-By: Ben Noordhuis <[email protected]>

This reverts commit 81eadc5. PR-URL: #990 Reviewed-By: Ben Noordhuis <[email protected]>

Using push instead of the original unshift makes the logic a little cleaner to read here. PR-URL: #990 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis · 2016-10-07T08:40:51Z

I think we were (and are) just waiting for the next node-gyp release. I've landed it in 9c8d275...ddac348, thanks!

mhart · 2016-10-07T13:01:30Z

Great, thanks @bnoordhuis 👍

Once it makes it to Node.js it'll be the first drop in size we've seen in years – here's hoping!

jbergstroem · 2016-10-07T13:23:58Z

This is pretty awesome. Thanks for the effort!

mhart · 2017-01-13T00:23:33Z

Thanks for pushing this out – [email protected] has just included it and has reaped the benefits of an 18% reduction in size 👍

This saves 3.6MB of cruft. See: nodejs/node-gyp#990

mhart added 2 commits July 12, 2016 15:42

Revert "configure: use "path-array" for PYTHONPATH"

c9028dd

This reverts commit ff88e5f. Of the 5.5MB footprint of node-gyp, path-array makes up 3.6MB. Removing this reduces the weight of npm (and thus node) appreciably.

Revert "add "path-array" dep"

ea45c1e

This reverts commit 81eadc5.

mhart changed the title ~~Remove path array~~ Remove path-array Jul 12, 2016

mhart reviewed Jul 13, 2016
View reviewed changes

bnoordhuis reviewed Jul 13, 2016
View reviewed changes

mhart force-pushed the remove-path-array branch from c35dfd9 to 3d603ce Compare July 13, 2016 15:04

mhart added a commit to lambci/npm that referenced this pull request Jul 13, 2016

Remove path-array from node-gyp

145ee30

This saves 3.6MB of cruft. See: nodejs/node-gyp#990

mhart force-pushed the remove-path-array branch from 3d603ce to bb835d6 Compare September 2, 2016 13:38

Use push on PYTHONPATH and add tests

f658da7

Using push instead of the original unshift makes the logic a little cleaner to read here.

mhart force-pushed the remove-path-array branch from bb835d6 to f658da7 Compare September 2, 2016 13:42

bnoordhuis pushed a commit that referenced this pull request Oct 7, 2016

Revert "add "path-array" dep"

b182a19

This reverts commit 81eadc5. PR-URL: #990 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis pushed a commit that referenced this pull request Oct 7, 2016

Use push on PYTHONPATH and add tests

ddac348

Using push instead of the original unshift makes the logic a little cleaner to read here. PR-URL: #990 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis closed this Oct 7, 2016

Fishrock123 mentioned this pull request Nov 7, 2016

Propose release: v3.5.0 #1049

Closed

mhart added a commit to lambci/npm that referenced this pull request Jan 24, 2017

Update node-gyp to 3.5.0

e39f7e0

This saves 3.6MB of cruft. See: nodejs/node-gyp#990

Uh oh!

Conversation

mhart commented Jul 12, 2016

Uh oh!

rvagg commented Jul 13, 2016

Uh oh!

mhart commented Jul 13, 2016

Uh oh!

mhart Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

mhart commented Jul 13, 2016

Uh oh!

bnoordhuis Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

mhart Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

mhart Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jul 13, 2016

Choose a reason for hiding this comment

Uh oh!

TooTallNate commented Jul 13, 2016

Uh oh!

mhart commented Jul 13, 2016

Uh oh!

mhart commented Jul 19, 2016

Uh oh!

mhart commented Aug 18, 2016

Uh oh!

bnoordhuis commented Aug 18, 2016

Uh oh!

mhart commented Aug 31, 2016

Uh oh!

bnoordhuis commented Sep 2, 2016

Uh oh!

bnoordhuis commented Sep 2, 2016

Uh oh!

mhart commented Sep 2, 2016

Uh oh!

mhart commented Sep 2, 2016

Uh oh!

joaocgreis commented Sep 8, 2016

Uh oh!

mhart commented Sep 20, 2016

Uh oh!

mhart commented Oct 6, 2016

Uh oh!

bnoordhuis commented Oct 7, 2016

Uh oh!

mhart commented Oct 7, 2016

Uh oh!

jbergstroem commented Oct 7, 2016

Uh oh!

mhart commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants