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

Only install scripts starting with "pil" #2257

Closed
wants to merge 2 commits into from

Conversation

floppym
Copy link

@floppym floppym commented Nov 26, 2016

This reverts 545154a.
This avoids a file collision with cgkit.

Bug: https://bugs.gentoo.org/600694

This reverts 545154a.
This avoids a file collision with cgkit.

Bug: https://bugs.gentoo.org/600694
@radarhere
Copy link
Member

Fair enough. Let me throw this suggestion out there - should we rename the Scripts files that don't start with pil so that they are still installed?

@floppym
Copy link
Author

floppym commented Nov 26, 2016

That sounds like a good idea to me.

@wiredfool
Copy link
Member

How many of these scripts are actually used out in the wild? They almost seem to be a pollution of a public namespace with pretty generic names, as evidenced here. The change to pil* is going to prevent some of that, but at the risk of causing breakage to people actually using the scripts. Do we even have a way of detecting usage? My gut feeling is that maybe 1% of pillow users use the scripts, and everyone else uses it as a library. (These w.a.g. numbers based on the support requests, not actual data)

OTOH, if no one complains, maybe we should separate them into a pillow-scripts python package. Or perhaps, maybe even prior to complaints.

We've got something of a breaking change release coming up. @aclark4life @hugovk @radarhere @homm ??

@aclark4life
Copy link
Member

I have no idea how many are used, but I'd certainly like to take some time to think about what the right thing to do here is.

@hugovk
Copy link
Member

hugovk commented Nov 27, 2016

It's very hard to know, occasionally we get issues about some of them, for example gifmaker seems to ring a bell. We could search the issues and see which are most commonly referred to in reports form users (other than in general house-keeping/improvements).

I'd be inclined to only rename those that are demonstrably causing a problem (viewer.py in this case?), and where renaming is the only or best option.

We made the change from including pil*.py to *.py over a year ago.

Some things to consider: How central is viewer.py to cgkit? How long has it been included in the package causing a collision? Is it possible or better to rename the file in cgkit? How many users are there of cgkit? (Looks like about 300 downloads/week here vs. Pillow's ~60k/week, but it's not such a central file for Pillow.)

Where's cgkit's source code hosted? I couldn't find it.

@floppym
Copy link
Author

floppym commented Nov 27, 2016

I'd be inclined to only rename those that are demonstrably causing a problem (viewer.py in this case?), and where renaming is the only or best option.

Installing a bunch of generally named .py files in /usr/bin is not a very nice thing to do regardless.

For example, how is the user supposed to know that /usr/bin/explode.py is intended for use on GIF image files?

@floppym
Copy link
Author

floppym commented Nov 27, 2016

Actually, after reading explode.py, I see it doesn't have anything to do with GIFs; I suppose I saw the word "animation" and my brain made a leap.

@aclark4life
Copy link
Member

Installing a bunch of generally named .py files in /usr/bin is not a very nice thing to do regardless.

@floppym No argument there, but we're not really discussing PIL's many annoyances here in the Pillow project, we're mostly discussing "how and when can we fix PIL's annoyances without anyone getting angry." The good news is we've managed to fix a lot of them over the years …

@wiredfool
Copy link
Member

Right. Pissing people off means there are still users.

There are two distinct populations this is going to affect:

  1. Distro packagers. I'm sure that distro policies vary but in general they're responsible for conflicts between packages and forward compatibility in general. The good thing there is that whatever we do it's a small notification problem, then it's taken care of downstream.

  2. Pip installers. If we rename, then we're going to break someone where they have to change a name in their scripts (If we don't, then we should have deleted, since no one's using it) If we add a separate package, we've then got an easy fix, but it's notification problem (If this breaks, install new recommendeds dependency). Though, if someone upgrades an existing install, I don't know if pip removes old scripts, or if it just overwrites with newer ones. That could lead to an upgrade from 3.x to 4.x succeeding, but a clean install of 4.x failing.

@wiredfool
Copy link
Member

Ok, We have a major, breaking release coming up.

Personally, I'm not sure that these are terribly useful anymore.

My vote is to move them into an alternate package/repo, and release them whenever there's a change.

At least one (createfontdatachunk.py) should have never been installed globally because it's almost guaranteed to not work. It should either be put in Tests/, or somewhere else where curious people won't accidentally run it and get an error.

@hugovk
Copy link
Member

hugovk commented May 13, 2017

The original report about a collision with cgkit has been resolved: https://bugs.gentoo.org/show_bug.cgi?id=600694

dev-python/pillow: don't install example scripts in /usr/bin

We already install them in /usr/share/doc/${PF}/examples.
This resolves a file collision with dev-python/cgkit.

However, the root problem remains in Pillow.


I'm fine with moving them to another repo (there's a suggestion to add tests for them in #2502, which may be cleaner to do in another repo): that shifts the collision problems from a large number of Pillow users to a small number of pillow-scripts users, which is good, but doesn't entirely solve it.

Move or not move, how about we review all the non-pil*.py scripts and decide whether to:

  • stop installing globally (and maybe move the file), or
  • rename to pil*.py, but keep a file with the old name for a release or two, that calls the new one but with deprecation warnings
    • then maybe the old gives errors without calling the new one for a release or two
    • then remove the old one

I doubt the deprecation needs to be as long as that for production code.

@hugovk
Copy link
Member

hugovk commented Aug 16, 2017

At least createfontdatachunk.py has been moved to no longer be installed globally: #2645

@hugovk
Copy link
Member

hugovk commented Dec 27, 2017

Note: PR #2901 will move these scripts to another repo.

@wiredfool
Copy link
Member

If they're installed separately, presumably there's no issue with conflicts, as anything that is installed is explicitly requested.

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