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
Make optional backends respect setup.cfg #2331
Conversation
Thanks. I think the cleaner solution is to call The |
Actually I first started using |
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. |
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 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
…check() methods by a super() call
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
I did some refactoring as outlined above. Most importantly the checking the config file and checking the dependencies is separated cleanly now:
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:
|
""" | ||
# check configuration file | ||
if self.get_config() in [True, 'auto']: | ||
self.check = "installing" |
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 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
.
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.
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 overriddencheck_requirements()
]" - normal install ->
check_requirements()
returns string -> "yes ['installing' + ', ' + custom message bycheck_requirements()
]" - install forced ->
check_requirements()
returns string -> "yes ['installing forced (config override)' + ', ' + custom message bycheck_requirements()
]"
Let me know if I am missing something here..
Actually.. while looking at this again.. I admit that storing
|
I did some more cleaning up. At first I did not want to touch I checked the install with the same situation as above (config opt-out on |
Works for me on macos 10.8. I made a random sampling of auto/True/False on different backends and they all detected correctly. |
Make optional backends respect setup.cfg
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.