Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

Correctly set extraneous flag on extraneous dependency cycles #20

Merged
merged 1 commit into from
Mar 19, 2014
Merged

Correctly set extraneous flag on extraneous dependency cycles #20

merged 1 commit into from
Mar 19, 2014

Conversation

LaurentVB
Copy link
Contributor

Fixes #16 and hopefully doesn't break anything else ;-)
At least all tests still pass.

@isaacs
Copy link
Contributor

isaacs commented Mar 15, 2014

It looks like test/fixtures/extraneous-detected/node_modules/ghjk is missing?

There are some style fixups that are probably rather straightforward. Also, I think I grok the algorithm here, but it'd be good to write up in pseudocode how this is working. Would you mind adding that to the commit message?

You don't need to do a new pull request or branch. You can just make all the changes and then do this:

# change stuff, git add, git commit, etc.
git remote add npm git://github.com/npm/read-installed
git fetch -a npm
git rebase -i npm/master

This will open it up in a text editor, where you'll see some instructions. Then you can use the squash option to squash your commits into one. When you do that, it'll then also give you the option to change the commit message.

Thanks!

@LaurentVB
Correctly set the extraneous flag on dependencies.
6e45076 
The readInstalled_ function sets extraneous=true for every dependency not
required by its direct parent and leaves it to undefined for others.

After the dependency tree has been built, the markExtraneous function will
walk it depth-first and set extraneous=false on dependencies that are not
extraneous and then recurse until all dependencies have been flagged:

for d in dependencies:
  if d.parent is not extraneous:
    set d.extraneous = false
    execute recursively on d.dependencies

This will ensure that extraneous dependencies will not be marked as
extraneous=false incorrectly because of a dependency cycle.
@LaurentVB
Copy link
Contributor Author

Struggled a little bit with the rebasing, but it's done.
Happy merging.

@isaacs
Copy link
Contributor

isaacs commented Mar 18, 2014

Failing tests:

$ npm test

> read-installed@2.0.0 test /Users/isaacs/dev/npm/read-installed
> tap ./test/*.js

ok ./test/basic.js ...................................... 6/6
ok ./test/depth-0.js .................................... 4/4
ok ./test/depth-1.js .................................... 4/4
not ok ./test/dev.js .................................... 5/6
    Command: "/usr/local/bin/node dev.js"
    TAP version 13
    ok 1 er should be bull
    ok 2 map should be data
    ok 3 array lengths are equal
    ok 4 arrays should be equal
    not ok 5 extraneous is set on devDep
      ---
        file:   /Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js
        line:   48
        column: 11
        stack:
          - getCaller (/Users/isaacs/dev/js/tap/lib/tap-assert.js:418:17)
          - Function.assert (/Users/isaacs/dev/js/tap/lib/tap-assert.js:21:16)
          - Test._testAssert [as ok] (/Users/isaacs/dev/js/tap/lib/tap-test.js:87:16)
          - /Users/isaacs/dev/npm/read-installed/test/dev.js:20:7
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:130:5
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
      ...
    ok 6 ./test/dev.js

    1..6
    # tests 6
    # pass  5
    # fail  1

ok ./test/extraneous.js ................................. 5/5
not ok ./test/noargs.js ................................. 5/6
    Command: "/usr/local/bin/node noargs.js"
    TAP version 13
    ok 1 er should be bull
    ok 2 map should be data
    ok 3 array lengths are equal
    ok 4 arrays should be equal
    not ok 5 extraneous is set on devDep
      ---
        file:   /Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js
        line:   48
        column: 11
        stack:
          - getCaller (/Users/isaacs/dev/js/tap/lib/tap-assert.js:418:17)
          - Function.assert (/Users/isaacs/dev/js/tap/lib/tap-assert.js:21:16)
          - Test._testAssert [as ok] (/Users/isaacs/dev/js/tap/lib/tap-test.js:87:16)
          - /Users/isaacs/dev/npm/read-installed/test/noargs.js:17:7
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:130:5
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
      ...
    ok 6 ./test/noargs.js

    1..6
    # tests 6
    # pass  5
    # fail  1

ok ./test/peer-dep-at-latest.js ......................... 2/2
total ................................................. 31/33

not ok
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

@LaurentVB
Copy link
Contributor Author

Weird, I can't reproduce (I'm on windows, if it's relevant...):

$ npm test

> read-installed@2.0.0 test c:\projects\read-installed
> tap ./test/*.js

ok ./test/basic.js ...................................... 6/6
ok ./test/depth-0.js .................................... 4/4
ok ./test/depth-1.js .................................... 4/4
ok ./test/dev.js ........................................ 6/6
ok ./test/extraneous.js ................................. 5/5
ok ./test/noargs.js ..................................... 6/6
ok ./test/peer-dep-at-latest.js ......................... 2/2
total ................................................. 33/33

ok

@LaurentVB
Copy link
Contributor Author

Just to be sure that I was testing the right thing, I did the following:

$ git clone https://github.com/npm/read-installed
$ cd read-installed/
$ git remote add LaurentVB https://github.com/LaurentVB/read-installed
$ git fetch LaurentVB
$ git merge LaurentVB/extraneous-detected
$ npm install
$ npm test

> read-installed@2.0.0 test c:\projects\temp\read-installed
> tap ./test/*.js

ok ./test/basic.js ...................................... 6/6
ok ./test/depth-0.js .................................... 4/4
ok ./test/depth-1.js .................................... 4/4
ok ./test/dev.js ........................................ 6/6
ok ./test/extraneous.js ................................. 5/5
ok ./test/noargs.js ..................................... 6/6
ok ./test/peer-dep-at-latest.js ......................... 2/2
total ................................................. 33/33

ok

Not really sure what's going on here...

@isaacs
Copy link
Contributor

isaacs commented Mar 19, 2014

Yeah, definitely failing here.

$ ghadd LaurentVB
remote: Counting objects: 22, done.
remote: Compressing objects: 100% (15/15), done.
remote: Total 22 (delta 0), reused 6 (delta 0)
Unpacking objects: 100% (22/22), done.
From git://github.com/LaurentVB/read-installed
 * [new branch]      extraneous-detected -> LaurentVB/extraneous-detected
 * [new branch]      master     -> LaurentVB/master
 * [new branch]      test-run-windows -> LaurentVB/test-run-windows

$ git merge LaurentVB/extraneous-detected
Updating 2c34b81..6e45076
Fast-forward
 read-installed.js                                                                | 27 +++++++++++++++++++++++----
 test/extraneous.js                                                               | 17 +++++++++++++++++
 test/fixtures/extraneous-detected/node_modules/asdf/package.json                 |  4 ++++
 test/fixtures/extraneous-detected/node_modules/bar/node_modules/baz/package.json |  8 ++++++++
 test/fixtures/extraneous-detected/node_modules/bar/package.json                  |  8 ++++++++
 test/fixtures/extraneous-detected/node_modules/foo/package.json                  |  4 ++++
 test/fixtures/extraneous-detected/node_modules/ghjk/package.json                 |  7 +++++++
 test/fixtures/extraneous-detected/package.json                                   |  7 +++++++
 8 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 test/extraneous.js
 create mode 100644 test/fixtures/extraneous-detected/node_modules/asdf/package.json
 create mode 100644 test/fixtures/extraneous-detected/node_modules/bar/node_modules/baz/package.json
 create mode 100644 test/fixtures/extraneous-detected/node_modules/bar/package.json
 create mode 100644 test/fixtures/extraneous-detected/node_modules/foo/package.json
 create mode 100644 test/fixtures/extraneous-detected/node_modules/ghjk/package.json
 create mode 100644 test/fixtures/extraneous-detected/package.json

$ git lg -n10
* 6e45076 Laurent VB (HEAD, LaurentVB/extraneous-detected, master) Correctly set the extraneous flag on dependencies. (4 days ago)
* 2c34b81 isaacs (tag: v2.0.0, origin/master, LaurentVB/master) v2.0.0 (5 days ago)
* 59f06bd isaacs (origin/dev-false, dev-false) Negative depth should be 0, not Infinity (6 days ago)
* 4ed39c8 isaacs Use util-extend module (6 days ago)
* 739b7a9 isaacs Invert the meaning of the "dev" option to correct value (7 days ago)
* 9ae275f isaacs tests for depth=0 and 1 (7 days ago)
* daeceb9 isaacs (tag: v1.0.1) v1.0.1 (7 days ago)
* 336d936 Robert Kowalski tests: use a fixture for tests (3 weeks ago)
* e7caad8 Laurent VB Allow tests to run on windows (3 weeks ago)
* 0ed9cb8 Robert Kowalski add gitignore (3 weeks ago)

$ npm t

> read-installed@2.0.0 test /Users/isaacs/dev/npm/read-installed
> tap ./test/*.js

ok ./test/basic.js ...................................... 6/6
ok ./test/depth-0.js .................................... 4/4
ok ./test/depth-1.js .................................... 4/4
not ok ./test/dev.js .................................... 5/6
    Command: "/usr/local/bin/node dev.js"
    TAP version 13
    ok 1 er should be bull
    ok 2 map should be data
    ok 3 array lengths are equal
    ok 4 arrays should be equal
    not ok 5 extraneous is set on devDep
      ---
        file:   /Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js
        line:   48
        column: 11
        stack:
          - getCaller (/Users/isaacs/dev/js/tap/lib/tap-assert.js:418:17)
          - Function.assert (/Users/isaacs/dev/js/tap/lib/tap-assert.js:21:16)
          - Test._testAssert [as ok] (/Users/isaacs/dev/js/tap/lib/tap-test.js:87:16)
          - /Users/isaacs/dev/npm/read-installed/test/dev.js:20:7
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:130:5
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
      ...
    ok 6 ./test/dev.js

    1..6
    # tests 6
    # pass  5
    # fail  1

ok ./test/extraneous.js ................................. 5/5
not ok ./test/noargs.js ................................. 5/6
    Command: "/usr/local/bin/node noargs.js"
    TAP version 13
    ok 1 er should be bull
    ok 2 map should be data
    ok 3 array lengths are equal
    ok 4 arrays should be equal
    not ok 5 extraneous is set on devDep
      ---
        file:   /Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js
        line:   48
        column: 11
        stack:
          - getCaller (/Users/isaacs/dev/js/tap/lib/tap-assert.js:418:17)
          - Function.assert (/Users/isaacs/dev/js/tap/lib/tap-assert.js:21:16)
          - Test._testAssert [as ok] (/Users/isaacs/dev/js/tap/lib/tap-test.js:87:16)
          - /Users/isaacs/dev/npm/read-installed/test/noargs.js:17:7
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:130:5
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
          - cb (/Users/isaacs/dev/npm/read-installed/node_modules/slide/lib/async-map.js:48:11)
          - /Users/isaacs/dev/npm/read-installed/read-installed.js:258:14
      ...
    ok 6 ./test/noargs.js

    1..6
    # tests 6
    # pass  5
    # fail  1

ok ./test/peer-dep-at-latest.js ......................... 2/2
total ................................................. 31/33

not ok
npm ERR! Test failed.  See above for more details.
npm ERR! not ok code 0

@isaacs
Copy link
Contributor

isaacs commented Mar 19, 2014

Huh, apparently there was something weird going on with my node_modules folder. I removed it and re-installed deps, and now it works.

@isaacs isaacs merged commit 6e45076 into npm:master Mar 19, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extraneous packages improperly detected
2 participants
@LaurentVB @isaacs