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

RFC: Source layout reorg #2911

Merged
merged 23 commits into from
Dec 29, 2017
Merged

RFC: Source layout reorg #2911

merged 23 commits into from
Dec 29, 2017

Conversation

wiredfool
Copy link
Member

We've had a bunch of issues with running tests and whatnot from the project directory because our PIL directory doesn't contain the compiled extensions unless you develop in-place. (which I object to, because it leads to problems with having the correct binaries for multiple python versions.)

This PR places all the source in a src directory below the project directory.

This:

  • Makes it so that you don't need the --installed on selftest.
  • Pytests' xdist extension works out of the box
  • Individual tests can be run with python Tests/test_foo.py
  • python setup.py develop still works.

In short, it makes make install work in a sane manner, without breaking other people's workflow.

Unfortunately, it also:

  • Moves everything.
  • Breaks history for individual files unless you use --follow

I'd rather do this at a major release than some other time, so time is short to say yes or no to this.

@wiredfool wiredfool force-pushed the src-reorg branch 2 times, most recently from 18d98ee to 62181f0 Compare December 27, 2017 22:41
@wiredfool wiredfool mentioned this pull request Dec 27, 2017
9 tasks
@aclark4life
Copy link
Member

@wiredfool Thanks for doing this! I'm OK with breaking changes for the sake of moving forward, with less confusion and less historical baggage.

@hugovk
Copy link
Member

hugovk commented Dec 27, 2017

Pytest strongly recommend this https://docs.pytest.org/en/latest/goodpractices.html and point to https://blog.ionelmc.ro/2014/05/25/python-packaging/#the-structure for more benefits.

Not really an issue for me as I usually use a Mac, but is this going to make things harder to develop on Windows, where it's difficult to set up the compiler? As it is, if you don't need to modify C code, you can edit Python code and tests and it'll happily use the compiled C stuff from the the last release pip installed.

@aclark4life
Copy link
Member

@hugovk We can probably clear that up by asking @cgohlke to comment

@radarhere
Copy link
Member

No objections. I'll just say the obvious though - this will break almost every other PR.

@wiredfool
Copy link
Member Author

@hugovk It looks like the first build on windows did succeed, it's just that it failed to move the PIL directory out of the way to force test runs against the installed version because the directory isn't there anymore. Similarly, the tests on linux failed due to the PIL directory having been moved, and coverage not being able to find the PIL directory.

I think this matches at least one of the pytest recommended source layouts, certainly better than the old layout.

@radarhere Yes, it does break PRs, but it also solves a long running pain point. We're at the lowest inventory of PRs in a long while, and we can do this one after the big ones we're merging in this window.

@wiredfool
Copy link
Member Author

This branch works on all the docker images OMM, with the pytest branch merged into this one.

@aclark4life
Copy link
Member

@radarhere Yeah, actually I appreciate you pointing that because I wasn't thinking about it; but I think it's OK too, because it should be easy for PR authors to update their PRs after this change.

@wiredfool wiredfool merged commit 81c88b1 into python-pillow:master Dec 29, 2017
@hugovk hugovk mentioned this pull request Dec 30, 2017
@hugovk
Copy link
Member

hugovk commented Jan 2, 2018

Shall we move _tkmini.h and tkImaging.c from Tk/ as well?

https://github.com/python-pillow/Pillow/tree/master/Tk

@wiredfool
Copy link
Member Author

wiredfool commented Jan 2, 2018

Yeah, that would make sense. (moving the whole directory at least)

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

Successfully merging this pull request may close these issues.

None yet

5 participants