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

Add warning if sharp bindings aren't built correctly #1638

Merged
merged 2 commits into from Apr 2, 2019

Conversation

Projects
None yet
3 participants
@sidharthachatterjee
Copy link
Contributor

commented Apr 2, 2019

Added a friendly error message in case the require to the bindings throws for any reason. The error message looks like this:

"sharp" does not seem to have been built or installed correctly.

- Try to reinstall packages and look for errors during installation
- Consult "sharp" installation page at http://sharp.pixelplumbing.com/en/stable/install/

If neither of the above work, please open an issue in https://github.com/lovell/sharp/issues

Fixes #1624

@sidharthachatterjee sidharthachatterjee referenced this pull request Apr 2, 2019

Closed

Improve error messaging for sharp #13026

2 of 3 tasks complete
@coveralls

This comment has been minimized.

Copy link

commented Apr 2, 2019

Coverage Status

Coverage decreased (-0.2%) to 98.282% when pulling 7de714c on sidharthachatterjee:master into 0a3512d on lovell:master.

@sidharthachatterjee

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Tests passed but it seems like the Travis job on Linux (musl) - Node 8 failed with

Segmentation fault (core dumped)
npm ERR! Test failed.  See above for more details.
The command "sudo docker exec sharp sh -c "npm test"" exited with 1.

@lovell lovell merged commit 2e0fbbb into lovell:master Apr 2, 2019

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 98.282%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lovell

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

Thanks Sidhartha, this is a great improvement.

@lovell lovell added this to the v0.22.1 milestone Apr 2, 2019

lovell added a commit that referenced this pull request Apr 2, 2019

Enhancement to and changelog entry for #1638
Remove bindings dependency as this isn't really... required
@lovell

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

I've added some more specific help in commit 687795c based on common error messages. Further additions/improvements are always welcome.

(I've also added commit 90a0382 to hopefully prevent/reduce the occurrence of the unrelated test failure you experienced.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.