setup.py: Recommend installation command for pkgs #6575

Merged
merged 1 commit into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
Contributor

AbdealiJK commented Jun 11, 2016

If a package is not found, provide a better error message
that also explains how to install the package or gives the
user a link explaining what to do to install the package.

The supported methods are:

  • Provide a link for windows
  • Provide a command using a package manager for the OS
    (For example: apt-get for Ubuntu/Debian, dnf and yum for
    Fedora/CentOS/RHEL, brew and port for OSX)

Fixes #6546

mdboom added the needs_review label Jun 11, 2016

Contributor

AbdealiJK commented Jun 11, 2016

This is still a work in progress, I haven't tested it to my satisfaction - but would love some feedback on the method used as I've never contributed to matplotlib before.

tacaswell added this to the 2.1 (next point release) milestone Jun 12, 2016

@QuLogic QuLogic commented on an outdated diff Jun 23, 2016

@@ -531,6 +540,38 @@ def do_custom_build(self):
"""
pass
+ def install_help_msg(self):
+ """
+ The help message to show if the package is not installed. The help
+ message shown depends on whether some class variables are present.
+ """
+ def _try_managers(*managers):
+ for manager in managers:
+ pkg_name = self.pkg_names.get(manager, None)
+ if pkg_name:
+ try:
+ _ = check_output(["which", manager],
@QuLogic

QuLogic Jun 23, 2016

Member

There is a shutil.which, but unfortunately, it's 3.3+. It's probably not worth it to conditionally call it, but can you add a comment about switching to it when we drop 2.7?

@QuLogic QuLogic commented on the diff Jun 23, 2016

setupext.py
+ return ('Try installing {0} with `{1} install {2}`'
+ .format(self.name, manager, pkg_name))
+ except subprocess.CalledProcessError:
+ pass
+
+ message = None
+ if sys.platform == "win32":
+ url = self.pkg_names.get("windows_url", None)
+ if url:
+ message = ('Please check {0} for instructions to install {1}'
+ .format(url, self.name))
+ elif sys.platform == "darwin":
+ message = _try_managers("brew", "port")
+ elif sys.platform.startswith("linux"):
+ release = platform.linux_distribution()[0].lower()
+ if release in ('debian', 'ubuntu'):
@QuLogic

QuLogic Jun 23, 2016

Member

What does this do on Debian/Ubuntu derivatives like Linux Mint?

@AbdealiJK

AbdealiJK Jun 23, 2016

Contributor

The supported distros in that function are:


_supported_dists = (
    'SuSE', 'debian', 'fedora', 'redhat', 'centos',
    'mandrake', 'mandriva', 'rocks', 'slackware', 'yellowdog', 'gentoo',
    'UnitedLinux', 'turbolinux')

If this is run on a non-supported distro, it returns empty strings ("", "", "").

There are also some issues with Ubuntu btw ... Some Ubuntu systems are detected as "debian" - http://bugs.python.org/issue9514

@QuLogic QuLogic commented on an outdated diff Jun 23, 2016

+
+ message = None
+ if sys.platform == "win32":
+ url = self.pkg_names.get("windows_url", None)
+ if url:
+ message = ('Please check {0} for instructions to install {1}'
+ .format(url, self.name))
+ elif sys.platform == "darwin":
+ message = _try_managers("brew", "port")
+ elif sys.platform.startswith("linux"):
+ release = platform.linux_distribution()[0].lower()
+ if release in ('debian', 'ubuntu'):
+ message = _try_managers('apt-get')
+ elif release in ('centos', 'redhat', 'fedora'):
+ message = _try_managers('dnf', 'yum')
+ return message
@QuLogic

QuLogic Jun 23, 2016

Member

Need two blank lines between classes.

@AbdealiJK AbdealiJK commented on the diff Jun 23, 2016

setupext.py
+ stderr=subprocess.STDOUT)
+ return ('Try installing {0} with `{1} install {2}`'
+ .format(self.name, manager, pkg_name))
+ except subprocess.CalledProcessError:
+ pass
+
+ message = None
+ if sys.platform == "win32":
+ url = self.pkg_names.get("windows_url", None)
+ if url:
+ message = ('Please check {0} for instructions to install {1}'
+ .format(url, self.name))
+ elif sys.platform == "darwin":
+ message = _try_managers("brew", "port")
+ elif sys.platform.startswith("linux"):
+ release = platform.linux_distribution()[0].lower()
@AbdealiJK

AbdealiJK Jun 23, 2016

Contributor

I had a note I wanted to mention here. platform.linux_distribution() is deprecated in Py 3.5 and is planned to be removed in Py 3.7. The related bug can be found at https://bugs.python.org/issue1322

I found the package https://pypi.python.org/pypi/distro which makes a pypi package out of this, but I am not sure of a good way to install this to use it in the setup.py. Suggestions ? (setup_requires is meant for this, but it's known to be buggy IIRC)

@tacaswell

tacaswell Jul 12, 2016

Owner

That is a 'fun' bug report.

Contributor

AbdealiJK commented Jul 12, 2016

Bump - reminder for review of this PR

@tacaswell tacaswell and 1 other commented on an outdated diff Jul 12, 2016

@@ -531,6 +540,41 @@ def do_custom_build(self):
"""
pass
+ def install_help_msg(self):
+ """
+ The help message to show if the package is not installed. The help
+ message shown depends on whether some class variables are present.
@tacaswell

tacaswell Jul 12, 2016

Owner

Can you add a note to this docstring explaining how to add the feature to sub-classes?

@AbdealiJK

AbdealiJK Jul 12, 2016

Contributor

done

@AbdealiJK AbdealiJK setup.py: Recommend installation command for pkgs
If a package is not found, provide a better error message
that also explains how to install the package or gives the
user a link explaining what to do to install the package.

The supported methods are:
 - Provide a link for windows
 - Provide a command using a package manager for the OS
   (For example: apt-get for Ubuntu/Debian, dnf and yum for
    Fedora/CentOS/RHEL, brew and port for OSX)
b82586b
Owner

tacaswell commented Jul 12, 2016

👍 I'll merge this as soon as CI passes again (just to be paranoid).

Contributor

AbdealiJK commented Jul 12, 2016

@tacaswell Do you recommend we use distro instead of platform.linux_distribution as users will get a DeprecationWarning when they run this on py3.5 Similar issue: pypa/pip#3823

Owner

tacaswell commented Jul 12, 2016

It will only be seen if things go wrong anyway 😈 .

Given the amount of sound and fury in the upstream bug, I am inclined to let this shake it self out a bit more before jumping on a solution.

On the other hand, better to deal with it now than later (when we have forgotten).

Contributor

AbdealiJK commented Jul 12, 2016

Maybe we could merge the PR but keep the issue open or create a new tracking issue to keep track of it ?

@tacaswell tacaswell merged commit dedcbd6 into matplotlib:master Jul 12, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 70.351%
Details

tacaswell removed the needs_review label Jul 12, 2016

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