Skip to content

Put the path to node-pre-gyp in quotes in install.js - #951

Closed
ksambhi wants to merge 13 commits into
nodegit:masterfrom
ksambhi:fix-install-issue-username-space-windows
Closed

Put the path to node-pre-gyp in quotes in install.js#951
ksambhi wants to merge 13 commits into
nodegit:masterfrom
ksambhi:fix-install-issue-username-space-windows

Conversation

@ksambhi

@ksambhi ksambhi commented Mar 15, 2016

Copy link
Copy Markdown
Contributor

Hi,
When I was installing nodegit, I found that there was compilation issues (#950). On further inspection, I found out that the building was taking place due to it failing to install a S3 binary. This was due to my user folder having a space in.

To fix this, I have placed the command in quotes.

Current Issues:

  • install.js now assumes that the install is a local install, even though that part of code was untouched.

@ksambhi

ksambhi commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

The linting script appears to be failing (appveyor build) due to my usage of quotes:

lifecycleScripts\install.js: line 39, col 18, Strings must use doublequote.
lifecycleScripts\install.js: line 39, col 63, Strings must use doublequote.

John Haley added 2 commits March 15, 2016 12:28
This should fix building for node 5.8.0+
@johnhaley81

Copy link
Copy Markdown
Collaborator

Do "\"" instead of '"'

@ksambhi

ksambhi commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

Ok

When building on appveyor, the linter exited with 1.
To fix this, we are putting an \ before "
@ksambhi

ksambhi commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

Test status:
It appears to be one test is failing. As I did not touch the tests, I believe this is because of how the tests have been written, or an incomplete/buggy test. Please correct me if I am wrong.

@tbranyen

Copy link
Copy Markdown
Member

@Gum-Joe you are correct, my server is down which is causing the failure.

@johnhaley81

Copy link
Copy Markdown
Collaborator

@Gum-Joe can you rebase this on top of master please?

@ksambhi

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

:@johnhaley81 Rebasing now

John Haley and others added 7 commits March 16, 2016 18:08
This should fix building for node 5.8.0+
…thub.com/Gum-Joe/nodegit into fix-install-issue-username-space-windows

Get ready for a merge and rebase for pull request nodegit#951
When building on appveyor, the linter exited with 1.
To fix this, we are putting an \ before "
@ksambhi

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Just rebased. Now pull requests will not merge

@johnhaley81

Copy link
Copy Markdown
Collaborator

Yeah, it pulled in too many commits. You just need 612540e and f451b68

@ksambhi

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

How can I fix this?

@ksambhi

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Oh... Now GitHub says it can merge.

@tbranyen

Copy link
Copy Markdown
Member

It might be better to just merge locally @johnhaley

johnhaley81 pushed a commit that referenced this pull request Mar 16, 2016
…e-windows'

Put the path to node-pre-gyp in quotes in install.js
@johnhaley81

Copy link
Copy Markdown
Collaborator

Merged manually

@johnhaley81

Copy link
Copy Markdown
Collaborator

Thanks @Gum-Joe!

@ksambhi

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Your welcome @johnhaley81

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.

3 participants