FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Put the path to node-pre-gyp in quotes in install.js by ksambhi · Pull Request #951 · nodegit/nodegit · GitHub

  1. nodegit
  2. nodegit

Repository navigation

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
ConversationCommits (13)ChecksFiles changed
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 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 mentioned this pull request Mar 15, 2016
Closed

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
Bump nan to 2.2.0
6be31fe 
This should fix building for node 5.8.0+
Use node 5.8.x on CI
e9d50fa

johnhaley81 commented Mar 15, 2016

Copy link
Copy Markdown
Collaborator

Do "\"" instead of '"'

ksambhi commented Mar 15, 2016

Copy link
Copy Markdown
Contributor Author

Ok

Fix issue where " is required
1af2386 
When building on appveyor, the linter exited with 1.
To fix this, we are putting an \ before "

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 commented Mar 15, 2016

Copy link
Copy Markdown
Member

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

johnhaley81 commented Mar 15, 2016

Copy link
Copy Markdown
Collaborator

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

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
Bump nan to 2.2.0
74ff595 
This should fix building for node 5.8.0+
Use node 5.8.x on CI
6642a76
Skip broken http test
4f35617
Merge branch 'fix-install-issue-username-space-windows' of https://gi…
30587d8 
…thub.com/Gum-Joe/nodegit into fix-install-issue-username-space-windows

Get ready for a merge and rebase for pull request nodegit#951
Put the path to node-pre-gyp in quotes in install.js
612540e 
This is to help resolve nodegit#950
Fix issue where " is required
f451b68 
When building on appveyor, the linter exited with 1.
To fix this, we are putting an \ before "
Merge branch 'fix-install-issue-username-space-windows' of https://gi…
4c7661d 
…thub.com/Gum-Joe/nodegit into fix-install-issue-username-space-windows

For pull request nodegit#951

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Just rebased. Now pull requests will not merge

johnhaley81 commented Mar 16, 2016

Copy link
Copy Markdown
Collaborator

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

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

How can I fix this?

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Oh... Now GitHub says it can merge.

tbranyen commented Mar 16, 2016

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
Merge pull request #951 from 'Gum-Joe/fix-install-issue-username-spac…
9789e01 
…e-windows'

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

johnhaley81 commented Mar 16, 2016

Copy link
Copy Markdown
Collaborator

Merged manually

johnhaley81 commented Mar 16, 2016

Copy link
Copy Markdown
Collaborator

Thanks @Gum-Joe!

ksambhi commented Mar 16, 2016

Copy link
Copy Markdown
Contributor Author

Your welcome @johnhaley81

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL