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

Unify querying of executable versions #9639

Closed
wants to merge 2 commits into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 31, 2017

PR Summary

Provide get_executable_info to replace the various checkdep_foo checkers (now deprecated), as suggested in #9571. Provide get_all_executable_infos for the purpose of quickly checking installed versions of all "relevant" executables.

Bump minimal supported ghostscript version to 9.0 (released in 2010, https://www.ghostscript.com/doc/current/History9.htm) in order to remove a conditional path.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell
Copy link
Member

Is the minimal gs version in the docs anyplace that also needs to be update?

@anntzer
Copy link
Contributor Author

anntzer commented Oct 31, 2017

Not as far as I can see.

@efiring
Copy link
Member

efiring commented Feb 23, 2018

It looks like this needs a little bit of updating to remove Py2.7 compatibility, but overall it looks good.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 23, 2018

py2.7 issues are handled. (I'm leaving the import reordering as is to keep the PR focused(ish).)

@anntzer anntzer force-pushed the moredepcheck branch 2 times, most recently from 3e14e50 to 3bfca79 Compare March 5, 2018 00:29
@anntzer anntzer force-pushed the moredepcheck branch 5 times, most recently from 9c842f1 to 3bfdb27 Compare May 8, 2018 06:50
@anntzer anntzer force-pushed the moredepcheck branch 3 times, most recently from 64f2b28 to 61983bb Compare May 11, 2018 06:27
checkdep_inkscape.version = v
except (IndexError, ValueError, UnboundLocalError, OSError):
pass
checkdep_inkscape.version = str(get_executable_info("inkscape").version)
return checkdep_inkscape.version
checkdep_inkscape.version = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just a caching mechanism, which is obsolete because get_executable_info() ls lru_cached.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but it doesn't cost me anything to keep it around until checkdep_inkscape itself gets removed.

-------
If the executable is found, a namedtuple with fields ``executable`` (`str`)
and ``version`` (`distutils.version.LooseVersion`, or ``None`` if the
version cannot be determined); ``None`` if the executable is not found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also None if the version smaller than the minimal required version. Which can crash later usage patterns like str(get_executable_info("dvipng").version).

I think, this function does too much. The version check should not be part of getting the version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points. I'll try to come up with a better API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about

  1. require_executable(exec_name) which returns the correct name to invoke the executable if it exists and its version is recent enough, and throws some exception with a reasonable error message if either the executable doesn't exist, or its version is too old;
  2. list_executables() which lists install status and version info for all "relevant" executables (mostly to get debugging info; I actually don't particularly think that's needed but I promised that to get Remove LaTeX checking in setup.py. #9571 in :-) or we could even just have a mpl.get_debug_info() similar to pandas.show_versions() which just dumps all kinds of whatever we think may be relevant in bug reports: versions of dependencies, versions of IPython/Jupyter, version of external executables, etc.)

Note that while 1) may seem a bit overkill just for ghostscript, there's actually at least another tool that would benefit from this, which is convert (imagemagick) in the animation framework), which currently also involves a sophisticated dance on Windows.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 27, 2019

Superseded by #13303.

@anntzer anntzer closed this Jan 27, 2019
@anntzer anntzer deleted the moredepcheck branch January 27, 2019 22:55
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