deps: remove unnecessary files

[mscdex]

This commit removes some unnecessary files introduced in 76cb81b whose filenames are also not valid on Windows, causing problems when checking out the repository on that platform.

Fixes#5094

[MylesBorins]

@mscdex have you confirmed that npm ls is still running clean?

I'm not sure our exact policy on this but I personally would prefer to see changes to the dep tree of npm come in as a patch from an npm contrib

/cc @nodejs/npm @jasnell

[MylesBorins]

@mscdex is there a test we could write that could check the npm tree for file lengths?

[mscdex]

@thealphanerd As far as I can tell, these files got added during the merge process, they were not part of the original npm 3.6.0 PR.

[MylesBorins]

super odd

This should be fixed by #5211 then.

[mscdex]

I don't think the problem in this case is filename length, but the characters in the name (e.g. :).

[MylesBorins]

Ahhhhh... I've been dealing with filename length fun all day so I was seeing what I wanted to see. Both seem like reasonable tests to add though 😄

[mscdex]

Also I'm not sure that merging newer versions of npm would actually remove these existing problematic files.

[MylesBorins]

@mscdex is this something that we could test for in the future?

[mscdex]

Short of having a pre-commit hook or something, I'm not sure what could have stopped this particular case since the files seemingly did not come from the original npm PR.

[MylesBorins]

we would have caught it before shipping though

[jasnell]

This is extremely odd indeed. PR LGTM

[thefourtheye]

Can we check if npm tests are passing with this change?

[mscdex]

@thefourtheye Is there a way to do that via CI or ?

[thefourtheye]

In CI we don't run that. We need to manually run the tests with make test-npm. cc @nodejs/testing @thealphanerd

[MylesBorins]

for landing npm I usually do a local test + citgm.

citgm: https://ci.nodejs.org/job/thealphanerd-smoker/72/

[tflanagan]

Oh this issue has been a nightmare for me, and I have no idea how to clean it up. If this PR doesn't land, or this issue isn't fixed via npm, is there a resource, or does anyone have steps, on cleaning this up?

It oddly is only affecting my windows machine.

[iarna]

This is extra weird because npm PRs are produced by removing deps/npm and then extracting a specially prepared version of the distribution tarball (we remove the symlinked bins from npms deps) and then adding that back.

I would expect any of the npm updates to have fixed this...

[mscdex]

citgm again after rebasing: https://ci.nodejs.org/job/thealphanerd-smoker/80/

The stylus and eslint failures are unrelated and will be fixed soon by a different PR. The lodash failure seems to be an upstream problem?

[mscdex]

citgm yet again after pushing the fix for the stylus and eslint errors: https://ci.nodejs.org/job/thealphanerd-smoker/81/

It looks like the only failure (for lodash) is unrelated and is something wrong with its npm scripts. I've also ran make test-npm locally and the only problems I can see from that is tests that fail because they're checking stderr output and the new re-evaluation warning is being printed there.

@thealphanerd@thefourtheye LGTY?

[rvagg]

lgtm

I've landed this on v5.x as 18c94e5 but suggest we don't land it on master and wait for the npm update instead. Testing suggests that the problem only impacts on people using the source, either from git or the full source tarball. While that's inconvenient for Windows users doing dev off master, unless the npm update is delayed by too much I think it'd probably be cleaner to hold off. npm@3 update is @ #5369

Unless anyone's super put out by the breakage? From what I understand it's just a warning when using git so it's the source tarballs that are causing real grief.

[MylesBorins]

closing as this is no longer relevant