Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Unify querying of executable versions#9639
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Is the minimal gs version in the docs anyplace that also needs to be update? |
Not as far as I can see. |
It looks like this needs a little bit of updating to remove Py2.7 compatibility, but overall it looks good. |
py2.7 issues are handled. (I'm leaving the import reordering as is to keep the PR focused(ish).) |
3e14e50 to3bfca79Compare9c842f1 to3bfdb27Compare64f2b28 to61983bbCompare| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AlsoNone if the version smaller than the minimal required version. Which can crash later usage patterns likestr(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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 getRemove 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. |
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