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

Support libvips < 8.8 #185

Merged
merged 1 commit into from
Jun 10, 2023
Merged

Conversation

iainbeeston
Copy link
Contributor

If you try to validate images while using libvips 8.7 every image fails validation with an "is not a valid image" error. This was helpfully investigated by @marckohlbrugge in #170 and seems to be caused because libvips can't report which file types it supports in versions below 8.8.

I've changed the code so it checks to see whether libvips can report the file types. I've assumed that if we have libvips installed and it can't report the supported file types then we should still use libvips. This might mean we raise errors if we give vips a file it does not support but I can't see another (easy) way to support older versions of libvips.

If you try to validate images while using libvips 8.7 every image fails validation with an "is not a valid image" error. This was helpfully investigated by @marckohlbrugge in igorkasyanchuk#170 and seems to be caused because libvips can't report which file types it supports in versions below 8.8.

I've changed the code so it checks to see whether libvips can report the file types. I've assumed that if we have libvips installed and it can't report the supported file types then we should still use libvips. This might mean we raise errors if we give vips a file it does not support but I can't see another (easy) way to support older versions of libvips.
@marckohlbrugge
Copy link

+1 on this. It resolved the issue described in #170

@adenta-grandstand
Copy link

Any updates?

@igorkasyanchuk
Copy link
Owner

Need help for this. If you or anyone can work on this it would be great

@iainbeeston
Copy link
Contributor Author

@igorkasyanchuk what help do you need? Are there any changes needed to the code in this PR before it can be merged?

@igorkasyanchuk
Copy link
Owner

igorkasyanchuk commented Jun 8, 2023

@iainbeeston yes, help to review PR and test if all is good, and it's ready to merge. Just need more 👀 on this

@igorkasyanchuk igorkasyanchuk added the help wanted Extra attention is needed label Jun 8, 2023
@iainbeeston
Copy link
Contributor Author

Maybe @marckohlbrugge @adenta-grandstand could confirm if they have tried this, and review the code to see if they agree with the changes?

@marckohlbrugge
Copy link

This did solve the issue for me and the code change seems straight forward.

I think the only downside is that for older versions of vips, when using an incompatible file extension, the code will now fail differently.

Maybe someone can test this to see the difference in behavior?

Either way, this PR seems like an improvement over the existing code.

@igorkasyanchuk igorkasyanchuk merged commit 2b1ed8a into igorkasyanchuk:master Jun 10, 2023
@igorkasyanchuk
Copy link
Owner

looks like libvips 8.8 was released long time ago, so I think we are fine. People can just upgrade it. Thanks for help, will release gem soon

@igorkasyanchuk
Copy link
Owner

oh, meanwhile if anyone has time to help to review other PRs please help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants