search

conda-forge / libtiff-feedstock

A conda-smithy repository for libtiff.
BSD 3-Clause "New" or "Revised" License
1 stars 25 forks source link

Patching up 4.1 for pillow compatibility on windows #51

Closed hmaarrfk closed 4 years ago

hmaarrfk commented 4 years ago

This is a second attempt at fixing some windows incompatibility with Pillow that is definitely affecting some of our users since the 4.1 migration

conda-forge/lcms2-feedstock#3 (comment) My previous attempt didn't include downstream completeness tests and so I was testing the finished product on the user's end machine. https://github.com/conda-forge/libtiff-feedstock/pull/49 Which caused some failures to occur downstream.

Talking to @cgohkle the easiest thing to do is simply to build the package the same unintended way the upstream developers had been doing it prior to 4.1

https://github.com/python-pillow/Pillow/issues/4237#issuecomment-570757948

xref:https://github.com/hmaarrfk/libtiff-feedstock/pull/1

Checklist

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

patricksnape commented 4 years ago

Seems everything passes? So I think this patch is probably good?

hmaarrfk commented 4 years ago

Yeah, generally I would like to have some green light from an upstream dev. Is there anything else to test with?

djhoese commented 4 years ago

I ran this PR (build 4 on @hmaarrfk anaconda.org channel) against all of Satpy's tests which include a few tests using pylibtiff on top of C libtiff to write custom TIFF files. Tests where build 2 from #49 seemed to seg fault now seem to pass on appveyor. Build log for the interested: https://ci.appveyor.com/project/pytroll/satpy/builds/29905441

hmaarrfk commented 4 years ago

@conda-forge-admin please rerender

conda-forge-linter commented 4 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to re-render for you but ran into some issues, please ping conda-forge/core for further assistance. You can also try re-rendering locally.

hmaarrfk commented 4 years ago

Thanks for doing that! I removed much of the regression tests from here so as to avoid a circular dependency. We can try them again once 4.2 is likely released with a patched up CMakeLists file.

ocefpaf commented 4 years ago

@hmaarrfk sorry for not following up on your work here. Is this ready to be merged?

hmaarrfk commented 4 years ago

Yeah. It is. I commented out the circular dependency testing.

ocefpaf commented 4 years ago

@hmaarrfk sorry but I ended up merging #48 first and now this one needs a rebase. If you don't have time for it I can port you patch later in a new PR.

hmaarrfk commented 4 years ago

seems like you pulled a package. do you still want me to rebase?

hmaarrfk commented 4 years ago

do you want me to test thing with gdal? can you provide the tests for the test section?

ocefpaf commented 4 years ago

Yeah, let's rebase and roll both changes at once. We'll have to put a migrator first though.

hmaarrfk commented 4 years ago

@ocefpaf did you want to merge this again with things cleaned? Seems like OSX is failing my downstream tests, but that seems to be due to some changes in tifffile that are kinda out of my control at this stage.

ocefpaf commented 4 years ago

Yeah, let's give it a go. Thanks @hmaarrfk!

hmaarrfk commented 4 years ago

opps. did you want me to remove the downstream tests?

ocefpaf commented 4 years ago

opps. did you want me to remove the downstream tests?

Nope. I kind of liked them ;-p

hmaarrfk commented 4 years ago

then i think i need a build number bump.

ocefpaf commented 4 years ago

then i think i need a build number bump.

Missed that! Sorry.