Cleanup dependency classes #15178
Cleanup dependency classes #15178
Conversation
dcbaker
force-pushed
the
submit/cleanup-dependencies
branch
6 times, most recently
from
da8f36f to
5dd7cf3
Compare
mesonbuild/dependencies/dev.py
Outdated
| [DependencyMethods.PKGCONFIG, DependencyMethods.CMAKE, DependencyMethods.SYSTEM], | ||
| cmake_name='ZLIB', | ||
| system_class=ZlibSystemDependency, | ||
| cmake=DependencyCandidate.from_dependency('ZLIB', CMakeDependency), |
There was a problem hiding this comment.
This change doesn't seem obviously better. What's wrong with DependencyFactory having a parameter to take the string name directly, so that it doesn't need to inline this fairly wordy constructor?
There was a problem hiding this comment.
I looked at the complexity of the DependencyFactory vs the number of times that complexity reduces complexity outside of it, and found there are 4 cases where we used it. It seems like a tradeoff of a little more typing outside the initializer in a few cases or a lot of seldom used code in the initializer.
I could be convinced that the previous state of affairs was better I guess.
There was a problem hiding this comment.
DependencyFactory invocations would otherwise involve pure data, so yes I'd like to stay where it involves pure data.
I don't think it's meaningful complexity, and I do think that it makes it easier to read invocations.
I also think that a fundamental aspect of CMake is inherently that one will need to do this for CMake specifically, so we would probably end up using it more (but there aren't that many custom dependency classes to begin with at the moment).
There was a problem hiding this comment.
I hope that we won't ever add more CMake type dependencies, given that they are pushing so hard to use CPS and not use .cmake files or cmake-config files anymore.
There was a problem hiding this comment.
The pure data argument isn't true. We're passing in types and strings, and types aren't pure data, they're callbacks.
I'll type up adding the string for cmake back and see what it looks like.
There was a problem hiding this comment.
I put it back, we end up with twice as much code as before, and we end up with exclusive keyword arguments that need to be guarded, and we can't even use it in the majority of uses we have (OpenSSL), because we need to pass modules in those cases.
edit: so I'm pretty unconvinced it's better.
There was a problem hiding this comment.
Numerically it does cover more than half of the uses. In practice I don't think it makes enough of a difference to justify the extra checks in the constructor and the extra magic of cmake_name becoming self.cmake after __init__ has run.
dcbaker
force-pushed
the
submit/cleanup-dependencies
branch
3 times, most recently
from
891782d to
ab7af2c
Compare
This is really class constant for all dependencies, and by taking it out of the initializer we make the `__init__` call have a more consistent interface.
So we don't create a default name that is overwritten except in the case of appleframeworks. This allows for some cleanup, including deleting some initializers that were only setting the name.
We have type checking that ensures this is a string list already.
It's allowed in the `DependencyKeywordArguments` TypeDict already, so we now have two sources of truth. Additionally, it's often populated by reading from that dict, so we're just doing useless work.
…Dependency It's read out of the `kwargs`, then passed along with the kwargs. We can just get it in the class initializer for no cost
This is always derived from kwargs, so now we have two sources of truth that can go out of sync.
This simplifies a bunch of cases, and likely fixes some annoying bugs in cross compile situations where should have been passing this and weren't.
The goal is to have a single type for candidates that replaces having a mixture of class initializers, partials, and factories with a single wrapper class.
This makes the type checking more reasonable (including fixing an issue that newer mypy is pointing out to us), consolidating special cases, and improving code readability.
Avoid extra method calls and repeating ourselves.
It's now only used to populate the DependencyCandidate, so we can remove it and just calculate the same information from the `type_name` parameter. This reduces code and the number of method calls.
dcbaker
force-pushed
the
submit/cleanup-dependencies
branch
from
ab7af2c to
fb06413
Compare
3 participants
This started out as an attempt to clean up some mypy warnings that show up in new versions of mypy (but not in the version used in CI), and things got a bit out of hand.
That issue was specifically that there were attempts to call
log_tried()on a partial, because we ended up with a partial inside a partial. To clean that up I thought it would be useful to have a DependencyCandidate class that was used to hold the dependency before it was instantiated, and could hold some of that information about what method was being called. This allows using partials or wrapper functions to create the class itself, without the dependency class being needed. And in doing that I just kept finding little annoyances I wanted to fix, until it became this.The good news is the amount of code before and after is roughly the same, and there are no only 2 issues spotted by newer mypy that older mypy doesn't.