Skip to content

Be less tolerant of broken installs. #12246

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

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 24, 2018

If no user-defined matplotlibrc is present, require it to exists in
mpl-data. (We already throw when mpl-data cannot be found -- which is
also a sign of broken install -- so it's not much worse to require
matplotlibrc to exist as well.)

PR Summary

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 no user-defined matplotlibrc is present, require it to exists in
mpl-data.  (We already throw when mpl-data cannot be found -- which is
also a sign of broken install -- so it's not much worse to require
matplotlibrc to exist as well.)
@anntzer anntzer force-pushed the mpldatahasmatplotlibrc branch from 70bea2e to 7ef113b Compare September 24, 2018 09:55
@ImportanceOfBeingErnest
Copy link
Member

Can you comment on why it would be necessary for some rc file to exist? It's usually a completely commented out file, so it serves no purpose, and does no harm if it's not found.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 24, 2018

Because the build script creates it (at https://github.com/matplotlib/matplotlib/blob/master/setup.py#L204).
We have tooling to ensure that a file exists, we shouldn't need additional tooling to cover the case where it doesn't.

@ImportanceOfBeingErnest
Copy link
Member

I'd say a user-friendly software would try to repair itself if it finds itself in a currupted state. I mean it's not obvious from anywhere that this file needs to exist.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 24, 2018

You also really need to mess it up/do it on purpose to have an install where the file doesn't exist (it's like an install where axes/_base.py doesn't exist, sure you could have deleted it accidentally but other than that...).
(Alternatively I'd be happy to see a realistic case where the file would be missing.)

@jklymak
Copy link
Member

jklymak commented Sep 24, 2018

So what happens if I nuke ~/.matplotlib ? Does it get recreated by init?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 24, 2018

We already look in $prefix/matplotlib/mpl-data/matplotlibrc where $prefix/matplotlib is the place matplotlib is actually installed. And that file is created at install time, in fact it's the one that gets picked up when you haven't configured your own matplotlibrc.

@tacaswell tacaswell requested a review from QuLogic September 24, 2018 19:01
@tacaswell
Copy link
Member

We have tooling to ensure that a file exists, we shouldn't need additional tooling to cover the case where it doesn't.

I am not sure we want to impose this on our down-stream packagers. We know that various linux distributions significantly de-construct our installation.

attn @sandrotosi .

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Anyone can dismiss this, I just want to make sure that we check with down-stream packagers that this will not cause them headaches.

@QuLogic
Copy link
Member

QuLogic commented Sep 27, 2018

In fact, I ran into issues with the fallback when the rc file did not exist. We used to have an rc file that just specified the backend, but dropped that since mpl can pick one automatically now. Since we patch the system location to be in /etc/matplotlibrc but testing is done on a temporary build root, mpl could not find the defaults, and triggered the fallback code that's removed here. I did not investigate further, but that fallback code does not work.

Whatever happens when an rc file (even if the rc file just specifies a backend and nothing else) is loaded didn't happen with the fallback, and a lot of params were their unvalidated non-canonical forms that didn't work when actually used.

TLDR, we already patch gen_candidates to use the location we want, and it never triggered this fallback code before, so it doesn't matter if it's not there.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Seems fine to me.

@tacaswell tacaswell added this to the v3.1 milestone Oct 11, 2018
@tacaswell
Copy link
Member

I am going to merge this as is, if we find it to be a problem, we should add a fallback of reading an empty file from a temp location.

@tacaswell tacaswell merged commit ddbdd4a into matplotlib:master Oct 11, 2018
@anntzer anntzer deleted the mpldatahasmatplotlibrc branch October 11, 2018 06:52
@sandrotosi
Copy link
Contributor

sorry for the late reply, but this is fine even for Debian - we force /etc/matplotlib into the paths to check and we make sure that file is present by installing it in the deb package

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.

6 participants