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

Fixes #950: Add default npm directory to PATH. #951

Merged
merged 0 commits into from Jan 9, 2014

Conversation

mwtian-zz
Copy link
Contributor

Fixes #950: Add default npm directory to PATH.

If [Program Files (x86)]\npm[Default npm version]\npm.cmd exists, then add its directory to PATH for the build and debug console environment.

@davidebbo
Copy link
Member

@pranavkm would be good if you could take quick look at this one. The code looks reasonable to me.

I tried just it in Azure using Private Extension, and the correct npm was on the path.

/// </remarks>
internal static string ResolveNodePath()
{
string programFiles = SystemEnvironment.GetFolderPath(SystemEnvironment.SpecialFolder.ProgramFilesX86);

string nodePath = Path.Combine(programFiles, "nodejs", "0.10.5", "node.exe");
string nodePath = Path.Combine(programFiles, "nodejs", DefaultNodeVersion, "node.exe");
Copy link
Member

Choose a reason for hiding this comment

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

Can't help but notice DefaultNodeVersion and DefaultNpmVersion are hardcoded in the constant in this file. For DefaultNodeVersion, we should have a tracking bug to adjust it in Public Kudu once we ship S25QFE. However, Private Kudu should already use the latest one (0.10.21).

For DefaultNpmVersion, we have two choices. 1. hardcode it as well but with a comment to adjust together with DefaultNodeVersion. 2. look at npm text of corresponding default Node. 1 is more optimized but room for human error. thought?

Copy link
Member

Choose a reason for hiding this comment

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

We talked with Pranav yesterday, and it's a tricky code path. For now, we decided to keep it as 0.10.5 (both in public & private). But note that this is different from the Azure default of 0.10.21 (and matching npm), which are used in different scenarios. Yes, it's a bit complicated :)

@mwtian-zz mwtian-zz merged commit 994afb1 into master Jan 9, 2014
@mwtian-zz mwtian-zz deleted the 950-set-current-npm-version branch January 9, 2014 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When running in the debug console, the path should point to the correct version of npm
3 participants