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 checking of executable info. #13303

Merged
merged 1 commit into from
Feb 23, 2019
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 27, 2019

PR Summary

Supersedes #9639. In particular, an exception is now thrown when the executable exists but is too old, which avoids the issue described in #9639 (comment).

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 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

if not shutil.which("tex"):
_log.warning("usetex mode requires TeX.")
return False
try:
Copy link
Member

Choose a reason for hiding this comment

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

We should try all three (even if one fails) so users can know to go install all 3 if they need them, not going to install things one-at-a-time (this is a pet peeve of mine...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is actually only used as a test marker (for non-test use, we don't run checkdep_usetex and instead just... do whatever is explained in #13285, which you already approved :)), so that doesn't really matter.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@tacaswell tacaswell added this to the v3.1 milestone Jan 28, 2019
Raises
------
FileNotFoundError
If the executable is not found or older that the oldest version
Copy link
Member

Choose a reason for hiding this comment

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

that->than

@jklymak
Copy link
Member

jklymak commented Jan 28, 2019

I'm not quite sold on the design here, given that each executable basically gets its own check inside the main check. Is that really better than what we have now, and why?

Should this be private since its all subject to change anyhow?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 28, 2019

The original discussion was in #9571, where there was additionally a single helper for retrieving all infos at once was suggested (and then we would make the all-at-once helper public and the one-at-a-time private). No real strong opinion there; personally the main annoying point IMO is the difference in return value types between the different checkdep_foos (checkdep_ghostscript returns (exec_name, version) whereas the others just return the version.

Typo fixed, thanks for noticing it.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Seems good. I'm sold that the refactor is useful, and having all the check code in one spot is probably helpful if operating system issues are at play...

@timhoffm
Copy link
Member

timhoffm commented Feb 2, 2019

IMO this could be private. I don't think we need to provide this functionality as part of our public API.

We can still make it public later on should the need arise.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 2, 2019

Made the replacement private.

@tacaswell tacaswell merged commit ea3be54 into matplotlib:master Feb 23, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Feb 23, 2019
tacaswell added a commit that referenced this pull request Feb 23, 2019
…303-on-v3.1.x

Backport PR #13303 on branch v3.1.x (Unify checking of executable info.)
@anntzer anntzer deleted the execcheck branch February 23, 2019 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants