Unify querying of executable versions#9639
Unify querying of executable versions#9639anntzer wants to merge 2 commits intomatplotlib:masterfrom
Conversation
|
Is the minimal gs version in the docs anyplace that also needs to be update? |
|
Not as far as I can see. |
anntzer
force-pushed
the
moredepcheck
branch
from
0805a5b to
15dc6a7
Compare
anntzer
force-pushed
the
moredepcheck
branch
from
2a8e544 to
d3f796c
Compare
anntzer
force-pushed
the
moredepcheck
branch
from
d3f796c to
152f43f
Compare
|
It looks like this needs a little bit of updating to remove Py2.7 compatibility, but overall it looks good. |
anntzer
force-pushed
the
moredepcheck
branch
from
152f43f to
c1d8bbb
Compare
|
py2.7 issues are handled. (I'm leaving the import reordering as is to keep the PR focused(ish).) |
anntzer
force-pushed
the
moredepcheck
branch
2 times, most recently
from
3e14e50 to
3bfca79
Compare
anntzer
force-pushed
the
moredepcheck
branch
5 times, most recently
from
9c842f1 to
3bfdb27
Compare
anntzer
force-pushed
the
moredepcheck
branch
3 times, most recently
from
64f2b28 to
61983bb
Compare
| pass | ||
| checkdep_inkscape.version = str(get_executable_info("inkscape").version) | ||
| return checkdep_inkscape.version | ||
| checkdep_inkscape.version = None |
There was a problem hiding this comment.
That's just a caching mechanism, which is obsolete because get_executable_info() ls lru_cached.
There was a problem hiding this comment.
I know, but it doesn't cost me anything to keep it around until checkdep_inkscape itself gets removed.
lib/matplotlib/__init__.py
Outdated
| ------- | ||
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good points. I'll try to come up with a better API.
There was a problem hiding this comment.
What about
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;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 ampl.get_debug_info()similar topandas.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.
|
Superseded by #13303. |
5 participants
PR Summary
Provide
get_executable_infoto replace the various checkdep_foo checkers (now deprecated), as suggested in #9571. Provideget_all_executable_infosfor 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