tools: Copying contents of deps/npm to test-npm#1853
tools: Copying contents of deps/npm to test-npm#1853thefourtheye wants to merge 2 commits intonodejs:masterfrom
Conversation
In Ubuntu, `cp -r deps/npm/ test-npm/` was copying `npm` directory under `test-npm` and the structure became `test-npm/npm`. But the test requires the contents of `deps/npm` to be under `test-npm`. Using `deps/npm/.` fixes it.
|
LGTM. @mhdawson does this also fix your Ubuntu issue with |
|
LGTM. The fix is also needed on Fedora. |
|
Would |
|
@thefourtheye what Ubuntu version is this? I can't seem to reproduce by running |
|
@silverwind |
|
@silverwind Mine are as following I just tried again and I am able to reproduce the problem. |
|
@thefourtheye reproduced now too. It does only manifest when the target directory exists. Confirmed now on Ubuntu and Arch. |
|
I am really wondering why this |
|
It's not shell expansion at least. Also confirming this works on BSD |
|
Can't we just omit a slash? $ cp -r deps/npm test-npm/Edit, output from Linux iojs-build-ubuntu1404-64-1 3.13.0-37-generic: $ mkdir -p deps/npm/foo test-npm
$ cp -r deps/npm test-npm/
$ ls -R test-npm
test-npm:
npm
test-npm/npm:
foo
test-npm/npm/foo: |
|
@jbergstroem that won't work, I think we want |
|
@silverwind hm, I thought that was what we wanted since we created $ cp -r deps/npm test-npmEdit: You're right. Looks like the script assumes we're in that folder. Then I'd suggest above -- remove the mkdir and just use cp to create it. |
|
Yeah, that |
Instead of creating `test-npm` first and then copying the contents of `deps/npm`, we just create `test-npm` from `deps/npm` with the `cp -r`
|
@silverwind @jbergstroem Updated the PR. Can you guys review this now? |
|
LGTM |
1 similar comment
|
LGTM |
This fixes a platform inconsistency between BSD and GNU `cp` where `deps/npm` would be copied into a subdirectory of `test-npm` on Linux, but not on OS X. PR-URL: #1853 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
|
Thanks, landed in 1baba05 |
|
@thefourtheye Would it be possible for you to sign your commits with your real name? You show up as 'thefourtheye' in Author fields now but I see you have a couple of Reviewed-By tags under your full name. |
|
@bnoordhuis Oh sorry. I changed it in my local |
|
They're in the history now so that's that. I speculate that the GH editor uses the name and email that are visible in your profile but I don't know for sure, I never use it. Can I suggest you just make your changes from the command line? Almost every change requires that you run |
|
@bnoordhuis Sure, next time onwards I ll do it from a local branch and sorry about this time. But before submitting the PR, I locally tested the changes and since the change was very small I directly edited it. |
|
No problem, it's not a big thing, just a small incongruity I noticed. |
This fixes a platform inconsistency between BSD and GNU `cp` where `deps/npm` would be copied into a subdirectory of `test-npm` on Linux, but not on OS X. PR-URL: nodejs/node#1853 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
In Ubuntu,
cp -r deps/npm/ test-npm/was copyingnpmdirectoryunder
test-npmand the structure becametest-npm/npm. But the testrequires the contents of
deps/npmto be undertest-npm. Usingdeps/npm/.fixes it.