Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: update test-npm to use absolute paths for tmp/cache/prefix #3309

Closed
wants to merge 1 commit into from

Conversation

iarna
Copy link
Member

@iarna iarna commented Oct 10, 2015

This gives npm absolute paths to use for its cache, prefix and tmp folders. Using relative paths seems to work, but makes me feel veeery edgy, as we do sometimes run subshells with copies of npm and we do sometimes change the working directory in tests. It seems to me that using relative paths here is just asking for really hard to track down trouble.

r: @Fishrock123
r: @chrisdickinson

@mscdex mscdex added npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory. labels Oct 10, 2015
@@ -24,9 +24,9 @@ rm -rf npm-cache npm-tmp npm-prefix
mkdir npm-cache npm-tmp npm-prefix

# set some npm env varibles to point to our new temporary folders
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also s/varibles/variables while you're at it? :D

@rvagg
Copy link
Member

rvagg commented Oct 12, 2015

lgtm pending @Fishrock123's request

@Fishrock123
Copy link
Contributor

Ping @iarna

LGTM, but the title exceeds 50 characters. Also the commit prefix should probably be tools:, or tools,test:.

These are minor nits I can fix on landing if need be, though. :)

@Fishrock123
Copy link
Contributor

Landed in f5445db with the above nits fixed

Fishrock123 pushed a commit that referenced this pull request Oct 22, 2015
Updated test-npm to use absolute paths for tmp/cache/prefix

PR-URL: #3309
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Oct 22, 2015
Updated test-npm to use absolute paths for tmp/cache/prefix

PR-URL: #3309
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins
Copy link
Contributor

should this be added to LTS?

/cc @jasnell

@Fishrock123
Copy link
Contributor

It could be, but I'm not worried about this being necessary in npm@2.

@MylesBorins
Copy link
Contributor

fair enough.

I guess we can hold off on this assuming npm3 will not be backported

@Fishrock123
Copy link
Contributor

@thealphanerd npm @ 3 is a breaking change. :)

@jasnell
Copy link
Member

jasnell commented Oct 23, 2015

Don't see a pressing need to get this into lts
On Oct 23, 2015 12:22 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

@thealphanerd https://github.com/TheAlphaNerd npm @ 3 is a breaking
change. :)


Reply to this email directly or view it on GitHub
#3309 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants