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.

Sign up for GitHub

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

Jump to bottom

Skip libvips compilation with SHARP_IGNORE_GLOBAL_LIBVIPS env var #1165

Merged
merged 1 commit into from Mar 22, 2018
Merged

Skip libvips compilation with SHARP_IGNORE_GLOBAL_LIBVIPS env var #1165

merged 1 commit into from Mar 22, 2018

Conversation

thom4parisot
Copy link
Contributor

fixes #1164

@coveralls
Copy link

coveralls commented Mar 21, 2018 •
edited

Coverage Status

Coverage increased (+0.003%) to 98.997% when pulling 7c1df86 on oncletom:feature/prebuild-env-var into 8dac256 on lovell:master.

lovell
lovell reviewed Mar 22, 2018
View reviewed changes
Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Hello, thank you very much for this PR, especially as it has the double bonus of both tests and documentation updates! I've left one question inline about the test.

test/unit/libvips.js Outdated
@@ -12,6 +12,7 @@ const setPlatform = function (platform) {

const restorePlatform = function () {
setPlatform(originalPlatform);
delete process.env.SHARP_IGNORE_GLOBAL_LIBVIPS;
Copy link
Owner

Choose a reason for hiding this comment

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

The platform-agnostic tests don't (currently) call restorePlatform so I think this env var won't get deleted. Perhaps the delete could move to the new test itself, after the assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Good call 😅 That explains why I saw the process.env.SHARP_IGNORE_GLOBAL_LIBVIPS being used 5 times on coveralls. I could explain this to myself.

I'll move the delete to the test itself then unless if I find a cleaner solution :-)

@thom4parisot
Copy link
Contributor Author

@lovell I've made the change. There are 8 jobs, and the coverage mentions the new code branch has been used 8 times. I guess it's fixed :-)

By the way, thank you so much by this library, and for the underlying work you have put into it. I'm amazed by its quality and speed 👍

@lovell lovell merged commit 0d7c3fc into lovell:master Mar 22, 2018
@lovell
Copy link
Owner

lovell commented Mar 22, 2018

Thank you / merci beaucoup!

@lovell lovell added this to the v0.20.2 milestone Mar 22, 2018
@thom4parisot
Copy link
Contributor Author

You're welcome 🙂

lovell added a commit that referenced this pull request Apr 11, 2018
@lovell
Changelog entry and credit for #1165
8933f11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lovell lovell lovell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.20.2
Development

Successfully merging this pull request may close these issues.

Forcing the use of pre-built binaries anyway
3 participants
@thom4parisot @coveralls @lovell