8000 BLD : improve how six dependency is handled by tacaswell · Pull Request #3482 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

BLD : improve how six dependency is handled #3482

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.

Sign up for GitHub

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

Jump to bottom
Merged
pelson merged 1 commit into from
Feb 1, 2015
Merged

BLD : improve how six dependency is handled #3482

pelson merged 1 commit into from
Feb 1, 2015

Conversation

@tacaswell
Copy link
Member
@tacaswell tacaswell commented Sep 6, 2014

if an old version of six is installed try to update it rather than fail.

This change seems to only work with pip, not with easy-install

If you have an old version of six installed and you use pip to install matplotlib (either via making a tar-ball with sdist and pip install path/to/tar or pip -e ./matplotlib) this works correctly.

If you do python setup.py install with this patch it successfully updates six, but then falls over saying the version of six is too low. If you then start up python and import six, it finds the newly installed version and if you re-run python setup.py install it will find the new version and work correctly.

I suspect that this may be due to pip vs easy install vs distutils vs setuptools vs black magic issues that is probably a bug in some external code, but I have no idea which.

I did this testing in a venv with pip install six==1.0 on an ubuntu box.

@tacaswell
BLD : improve how six dependency is handled
73cc574 
if an old version of six is installed try to
update it rather than fail.c
@tacaswell tacaswell added this to the v1.4.x milestone Sep 6, 2014
@WeatherGod
Copy link
Member
WeatherGod commented Sep 6, 2014

I really don't see the point in adding confusion here. The error message as it stands is clear and easy to take care of. This is the same as any other package dependency issue, right? Adding more black magic is probably not the right thing to do here.

@mdboom
Copy link
Member
mdboom commented Sep 8, 2014

@WeatherGod: The issue is that if your six is too old, and you install matplotlib with pip, pip will upgrade your six, but matplotlib's setup.py won't see it right away, and raise an exception and refuse to install. This change is fundamentally about turning that exception into a warning so that the installation of matplotlib will proceed without the new six. I think that makes sense. A lot of this version checking stuff predates our use of setuptools, so I'm not opposed to pushing more and more of it off to setuptools etc. as setuptools becomes mature enough to handle such things.

@WeatherGod
Copy link
Member
WeatherGod commented Sep 9, 2014

Ah... lovely. I would also like to see all the version checking stuff get
moved to setuptools as well, but I won't stand in the way of this PR, then.

On Mon, Sep 8, 2014 at 11:00 AM, Michael Droettboom <
notifications@github.com> wrote:

@WeatherGod https://github.com/WeatherGod: The issue is that if your six
is too old, and you install matplotlib with pip, pip will upgrade your six,
but matplotlib's setup.py won't see it right away, and raise an exception
and refuse to install. This change is fundamentally about turning that
exception into a warning so that the installation of matplotlib will
proceed without the new six. I think that makes sense. A lot of this
version checking stuff predates our use of setuptools, so I'm not opposed
to pushing more and more of it off to setuptools etc. as setuptools becomes
mature enough to handle such things.

—
Reply to this email directly or view it on GitHub
#3482 (comment)
.

@tacaswell
Copy link
Member Author
tacaswell commented Sep 9, 2014

That is what this does. Most of code in the checker is just to provide sensible output. The only part that really matters is the

def get_install_requires(self):
    return ['six>={0}'.format(self.min_version)]

which works, but does not play nice with easy_install (which is apparently how dependencies get installed if you use python setup.py install. From looking at the logs from the install process, I suspect that is where the problem is (as it does things like claim to be installing 1.7.3 into the 1.0.0 path or something like that).

The best-case against this PR is that in can clobber user install six if there is something that has a maximum version of six as I don't think pip does dependency evaluation.

@WeatherGod
Copy link
Member
WeatherGod commented Sep 10, 2014

why do I get a headache whenever I want to do something beyond the basic
things in setuptools? I love it for the basic, easy stuff. But this... I
guess I'll just defer to you guys on this.

On Tue, Sep 9, 2014 at 11:43 AM, Thomas A Caswell notifications@github.com
wrote:

That is what this does. Most of code in the checker is just to provide
sensible output. The only part that really matters is the

def get_install_requires(self):
return ['six>={0}'.format(self.min_version)]

which works, but does not play nice with easy_install (which is apparently
how dependencies get installed if you use python setup.py install. From
looking at the logs from the install process, I suspect that is where the
problem is (as it does things like claim to be installing 1.7.3 into the
1.0.0 path or something like that).

The best-case against this PR is that in can clobber user install six if
there is something that has a maximum version of six as I don't think
pip does dependency evaluation.

—
Reply to this email directly or view it on GitHub
#3482 (comment)
.

@tacaswell
Copy link
Member Author
tacaswell commented Sep 10, 2014

I am not convinced there is any 'basic simple' stuff in setuptools/distutils....

@cimarronm
Copy link
Contributor
cimarronm commented Sep 10, 2014

LGTM 👍

@pelson
Copy link
Member
pelson commented Feb 1, 2015

@tacaswell - where is this at?

@tacaswell
Copy link
Member Author
tacaswell commented Feb 1, 2015

I think this is ready to go.

We li A276 st six as a requirement in setup.py and hence pip will install it. This just removes raising an exception so that setuptools can do it's job.

pelson added a commit that referenced this pull request Feb 1, 2015
@pelson
Merge pull request #3482 from tacaswell/six_dep
1550439 
BLD : improve how six dependency is handled
@pelson pelson merged commit 1550439 into matplotlib:v1.4.x Feb 1, 2015
@tacaswell tacaswell deleted the six_dep branch May 16, 2015 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.4.x

Development

Successfully merging this pull request may close these issues.

5 participants

@tacaswell @WeatherGod @mdboom @cimarronm @pelson
0