-
Notifications
You must be signed in to change notification settings - Fork 698
Error message on libstdc missing. #972
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
Error message on libstdc missing. #972
Conversation
1dc7faa to
1c7ad24
Compare
|
Love it. Means I'll have less repetitive issues to close, and indirect users will have a better idea on how to solve any issues with installing. |
|
Testing on windows right now :) |
|
Does windows not run the post-install script? |
|
Ahh I didn't see the second commit I'll confirm locally then gtg IMO. |
postinstall.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| if [ -n "$(node lib/nodegit.js 2>&1 | grep libstdc++)" ]; then | ||
| echo "[ERROR] Seems like you the latest libstdc++ is missing on your system!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little grammatical error. Should read `"[ERROR] Seems like the latest libstdc++ is missing on your system!"
|
Confirmed that it works locally. I did make 2 minor comments though. |
|
@johnhaley81 Pushed the adjustments |
|
Thanks @martinheidegger! |
|
This is breaking builds in Windows, you can't invoke Bash scripts in npm packages. Can you rewrite this as a JS file? Should be super easy and you can bail out if |
nodegitis usually used as library for other projects. Nodegit is also, unfortunately, depends on a version of libstdc++ that is not installed on many linux systems. Any person that uses nodegit as a library would need to explain this problem in their issues. In order to reduce the work of libraries/tools that depend on nodegit this PR adds an error message on postinstall if the dependency is missing and adds steps on how to mitigate that issue.Note: This has been a follow-up on #969.