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

Patching up 4.1 for pillow compatibility on windows #51

Merged
merged 21 commits into from
Mar 16, 2020

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Jan 4, 2020

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.
#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

python-pillow/Pillow#4237 (comment)

xref:hmaarrfk#1

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@conda-forge-linter
Copy link

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.

@hmaarrfk hmaarrfk changed the title Patching up Patching up 4.1 for pillow compatibility Jan 4, 2020
@hmaarrfk hmaarrfk changed the title Patching up 4.1 for pillow compatibility Patching up 4.1 for pillow compatibility on windows Jan 4, 2020
@patricksnape
Copy link

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

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jan 5, 2020

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

@hmaarrfk hmaarrfk mentioned this pull request Jan 5, 2020
5 tasks
@djhoese
Copy link

djhoese commented Jan 5, 2020

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
Copy link
Contributor Author

hmaarrfk commented Jan 5, 2020

@conda-forge-admin please rerender

@conda-forge-linter
Copy link

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
Copy link
Contributor Author

hmaarrfk commented Jan 5, 2020

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
Copy link
Member

ocefpaf commented Jan 7, 2020

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

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jan 7, 2020

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

@ocefpaf
Copy link
Member

ocefpaf commented Jan 8, 2020

@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
Copy link
Contributor Author

hmaarrfk commented Jan 8, 2020

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

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Jan 8, 2020

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

@ocefpaf
Copy link
Member

ocefpaf commented Jan 8, 2020

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

@hmaarrfk
Copy link
Contributor Author

@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
Copy link
Member

ocefpaf commented Mar 16, 2020

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

@ocefpaf ocefpaf merged commit 419b7e4 into conda-forge:master Mar 16, 2020
@hmaarrfk
Copy link
Contributor Author

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

@ocefpaf
Copy link
Member

ocefpaf commented Mar 16, 2020

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

Nope. I kind of liked them ;-p

@hmaarrfk
Copy link
Contributor Author

then i think i need a build number bump.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 16, 2020

then i think i need a build number bump.

Missed that! Sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants