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

Make optional backends respect setup.cfg #2331

Merged
merged 7 commits into from Sep 9, 2013

Conversation

megies
Copy link
Contributor

@megies megies commented Aug 22, 2013

Several optional backends were not checking the setup.cfg before testing if they can be installed (which raised and broke the build for some, see #1949). For more info see comment in #1949 (comment).

This feels pretty ugly. I think it should be handled at the super class OptionalBackendPackage so it can't be overlooked when a new backend gets added. However it is a temporary fix. Implemented in super class now.

@mdboom
Copy link
Member

mdboom commented Aug 26, 2013

Thanks. I think the cleaner solution is to call super's check method for each of these. Then, as you say, it will be handled in the base class.

The setup.cfg.template should also be updated to include all of the backends that can be disabled in this way.

@megies
Copy link
Contributor Author

megies commented Aug 26, 2013

Actually I first started using super but changed my mind because I think python2's super is super ugly (with not defaulting to the current class -- no wonder this was changed in python3; and I saw the construct I use in one of the backends). Anyway, I'll change it to super then..

@mdboom
Copy link
Member

mdboom commented Aug 26, 2013

I agree about its ugliness, but it's sort of the standard way to handle it in the super class (as you suggested). I'm open to other ideas, though.

@megies
Copy link
Contributor Author

megies commented Aug 26, 2013

I'm at it right now.. Actually, while working on it, I think now that the check for a config-file opt-out should be done in a separate method that is then calling the custom backend check method (just passing in the base class) so that the super is not needed in overwritten backend check methods at all and thus preventing a possible overlook when new backends get added.

I'm just a bit concerned that this pull request might get too unclear to review (I'm also tripping over a few other minor things that should get changed. I'll try to keep them in separate commits).. Are there tests for building with different configuration files and so on?

Anyway, I'll try to come up with something that is better than before and we'll see about eventual merging later..

 - check for `if config is not None` before the try/except/pass adds
   nothing, just makes it less readable
 - much clearer to handle of the default value of the `install` variable
   with the base class and not nested in a try/except of the
   `get_config` method (and even duplicated outside of the try/except)
 - the `force` attribute (only used for BackendAgg right now it seems)
   should also have a more prominent default state in the base class
   rather than in just one subclass
also split up the if/else in 3 branches to show a separate message if an
configuration file backend opt-out was overridden by a forced backend
install
@mdboom
Copy link
Member

mdboom commented Aug 26, 2013

That all makes sense. We aren't currently testing different build configurations. I'd be happy to see some develop, of course. Just go ahead and submit what you've got when you've got it, and we'll go from there. Welcome!

 - config check is handled handled in optional backend base class and
   calls separate dependency check
 - dependency check is an empty stub in base class and can be overriden
   in subclasses
@megies
Copy link
Contributor Author

megies commented Aug 27, 2013

I did some refactoring as outlined above. Most importantly the checking the config file and checking the dependencies is separated cleanly now:

  • config check check() is left with base class and calls the..
  • dependency check stub check_requirements() which can be overridden by subclasses (avoiding the need for supers scattered all over the place

I think the separate commits show the way pretty clearly.

I have tested it with a setup.cfg that manually deactivates..

If there's any questions, let me know.

Some other points I noticed:

  • the setting the force attribute might still be handled cleaner
  • i assume the force for agg has to be set on more than just tkagg?!

"""
# check configuration file
if self.get_config() in [True, 'auto']:
self.check = "installing"
Copy link
Member

Choose a reason for hiding this comment

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

I think this message will be ultimately misleading. If setup.cfg is True or auto, but the dependency isn't found, it will read something like installing, gtk not found. I think in that case, the message from check_requirements should just override, but not append to, self.check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I just looked at it again, and I am kind of arriving at the same logic again (which was there before I made changes, I really only refactored it into two routines):
Looking at setup.py where the package checks are called, any raised CheckFailed Exception drops out of the installation (ignoring any install message assigned in there like "installing") and its Exception message is used exclusively to indicate the reason. Only if check_requirements() properly returns (no raise), this string is appended and the package is assigned for installing in setup.py.

Summary..

  • configuration opt out -> check() raises -> "no [skipping due to configuration]"
  • dependency check fails -> check_requirements() raises -> "no [exception msg as defined in overridden check_requirements()]"
  • normal install -> check_requirements() returns string -> "yes ['installing' + ', ' + custom message by check_requirements()]"
  • install forced -> check_requirements() returns string -> "yes ['installing forced (config override)' + ', ' + custom message by check_requirements()]"

Let me know if I am missing something here..

@mdboom mdboom mentioned this pull request Sep 3, 2013
@megies
Copy link
Contributor Author

megies commented Sep 3, 2013

Actually.. while looking at this again.. I admit that storing install and the message to show (badly named check) with the OptionalBackendPackage class directly is more confusing than helpful on a second look.. I'll change this and try to make it cleaner and clearer to read.
ÈDIT: It seems I was tricked into thinking a self.install attribute could be necessary because it is used in OptionalPackage. However I do not see that attribute accessed elsewhere (also not in setup.py) so I will get rid of it.

I think I see another flaw, namely that it seems to me that currently (and also on v1.3.x) there is no distinction between a True and a "auto" coming from setup.cfg. Am I not seeing something here? Otherwise I'll look into how to clean it up.
EDIT: Overlooked this line in setup.py.

@megies
Copy link
Contributor Author

megies commented Sep 3, 2013

I did some more cleaning up. At first I did not want to touch OptionalPackage to keep this PR as minimally intrusive as possible. But after looking at it several times, the OptionalBackendPackage class looked more and more obsolete and I saw only obscurity springing from the branching class hierarchy, code duplication and incoherent config handling (one was using a "auto"/True distinction, one was not).
So, I moved almost everything to OptionalPackage and made OptionalBackendPackage a (stub) subclass of it.

I checked the install with the same situation as above (config opt-out on agg which is then force-installed; opt-out on 2 broken backends (the problem being fixed in v1.3.x already)). Here's the full build log and setup.cfg for completeness.
Would be nice to get some other build checks with different configurations and platforms etc.

@mrterry
Copy link
Contributor

mrterry commented Sep 6, 2013

Works for me on macos 10.8. I made a random sampling of auto/True/False on different backends and they all detected correctly.

mdboom added a commit that referenced this pull request Sep 9, 2013
Make optional backends respect setup.cfg
@mdboom mdboom merged commit 8607047 into matplotlib:v1.3.x Sep 9, 2013
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.

None yet

3 participants