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

Remove vendored version of olefile Python package in favor of upstream #2199

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Remove vendored version of olefile Python package in favor of upstream #2199

merged 1 commit into from
Dec 13, 2016

Conversation

jdufresne
Copy link
Contributor

The olefile Python package is now optional. Pillow will now use olefile if installed, otherwise will not support FlashPix and Microsoft Image Composer formats.

Updated docs with information on how to include support with a backwards incompatibility note.

This removes Pillow's double maintenance of this library by instead relying on and reusing the upstream version. No longer need to regularly update the vendored package and docs. olefile bug fixes and features can go directly upstream.

CCing @decalage2 as the maintainer of olefile.

For reference, the double maintenance and vendoring maintenance has been discussed to some degree in several PRs/issues:

@decalage2
Copy link

OleFileIO was originally part of PIL, when I forked it back in 2005. I agree now it makes more sense for Pillow to import it as a dependency, since it is a separate package used by quite a few other projects. It will also avoid the maintenance of two versions.

@hugovk
Copy link
Member

hugovk commented Nov 6, 2016

That would simplify things. A couple of points:

  • There are a number of changes to Pillow's OleFileIO.py since OleFileIO 0.42b was last integrated here. They should be merged upstream (also test_olefileio.py if anything changed there too).
  • OleFileIO's i8 and i32 originally came from Pillow. We should use Pillow's versions from _binary.py rather than importing OleFileIO's copies (e.g. here).

@jdufresne
Copy link
Contributor Author

There are a number of changes to Pillow's OleFileIO.py since OleFileIO 0.42b was last integrated here. They should be merged upstream (also test_olefileio.py if anything changed there too).

Created upstream PR decalage2/olefile#34. All feedback there is welcome.

OleFileIO's i8 and i32 originally came from Pillow. We should use Pillow's versions from _binary.py rather than importing OleFileIO's copies (e.g. here).

Done in latest commit.

Thanks!

@wiredfool
Copy link
Member

-1

This change is likely to cause other people's code to break. Furthermore, running the test suite will not alert on that breakage, because the tests won't run if olefile isn't installed.

At a minimum, we're going to need to add olefile as a requires entry in setup.py. We're also going to need to check on the downstream packagers so that they can either build an olefile package or pull it in during the packaging process.

We should also import something into PIL.OleFileIO that traps the call and issues a deprecation warning or similar, since it's part of the public interface that we've been exporting for years. (decades?).

@hugovk
Copy link
Member

hugovk commented Nov 7, 2016

Good point about tests. This PR installs it on Travis CI, so it'll be tested there for Linux and Mac. We should also install it for the Windows CI on AppVeyor.

Is OleFileIO an essential dependency? It's only needed for FlashPix and Microsoft Image Composer formats. Although it's not a big problem to install it everywhere for those who don't need it and could cause fewer headaches to do so. Perhaps require it and then not require it after some deprecation period.

OleFileIO's been part of PIL since at least 1.1.1, released in 2000.

@jdufresne
Copy link
Contributor Author

jdufresne commented Nov 8, 2016

Integrated feedback.

The olefile Python package is now included in setup.py with install_requires.

The FPX and MIC tests will now always run, including on all CI servers.

Users will automatically install the olefile Python package when installing Pillow through the pip command.

The PIL.OleFileIOmodule was restored with a DeprecationWarning. This warning shows the user that this module will eventually be removed. (Should I specify a specific version? If so, which version?) If anyone is relying on this module, they will not be surprised by its absence when upgrading.

@jdufresne
Copy link
Contributor Author

I noticed I still needed to include - "travis_retry pip install olefile" in the travis file. Is there a better way to instruct travis to install Pillow or Pillow's dependencies?

@wiredfool
Copy link
Member

I've rebased to fix the setup conflict and added something to do this:

We should also import something into PIL.OleFileIO that traps the call and issues a deprecation warning or similar, since it's part of the public interface that we've been exporting for years. (decades?).

so that we'll warn users, then silently redirect them to olefile without having to maintain two separate versions.

https://github.com/wiredfool/Pillow/tree/dep-olefile

As for the olefile dependency, when I run make install, it does install the vendored olefile. The in place stuff used in travis must not run the same setup loop.

…ream

Pillow now requires the olefile Python package through setup.py.

This removes Pillow's maintenance of this library by instead relying on
and reusing the upstream version. No longer need to regularly update the
vendored package and docs. olefile bug fixes and features can go
directly upstream.

During travis tests, now installs Pillow package before tests; this will
also install all dependencies (currently, only olefile).
@jdufresne
Copy link
Contributor Author

Thanks for the review and suggested changes. The sys.modules[__name__] = olefile trick is neat. Haven't seen that one before.

I have incorporated all changes and rebased the branch. I have handled the travis dependency issue by adding a pip install -e . to .travis.yml.

All additional feedback welcome. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
olefile Removal Removal of a feature, usually done in major releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants