-
Notifications
You must be signed in to change notification settings - Fork 30
eclass: add ShadowedEclassPhase for multiple-eclass defined phases #762
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
base: master
Are you sure you want to change the base?
Conversation
2cfe29e to
91afa80
Compare
Detect the basic case of an ignored eclass phase where two eclasses are inherited, both defining the same phase, and the ebuild does not define a custom implementation of that phase at all. This means one of the exported phases from the eclasses is being ignored. Ignore some eclasses with a blacklist where they are known to vary their API by EAPI or eclass variables, at least for now, because the eclass cache we have accessible here isn't keyed by EAPI or the context of the sourcing ebuild. Bug: https://bugs.gentoo.org/516014 Bug: https://bugs.gentoo.org/795006 Closes: pkgcore#377 Signed-off-by: Sam James <sam@gentoo.org>
Signed-off-by: Sam James <sam@gentoo.org>
91afa80 to
10489c0
Compare
|
There's a lot of noise from things like One might argue this really means we should have We can also have this be off-by-default, but nonetheless, better heuristics would be useful.. |
ferringb
left a comment
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.
random code poking. Take what makes sense, discard what doesn't. Y'all know this code better than I do.
| # We could maybe make this more finely-grained for phases we know | ||
| # are conditionally exported if this list is impacting coverage | ||
| # severely. | ||
| blacklisted_eclasses = ["pypi", "vala", "xdg"] |
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.
This should be a class level frozenset, so it can be varied to test in the testcases. Plus it's more obvious.
| def __init__(self, phase, providers, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self.phase = phase | ||
| self.providers = tuple(providers) |
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.
self.providers = tuple(sorted(providers)); expect the invokers of this class will forget to sort, so just force it on them. Mark providers as typing.Iterable[str]- assuming it's a string?
And yes, I'm now brutally anal about typing. The syntax sucks, but it's better devex and I'd like to eventually enforce mypy.
| phase for phase, eclass in exported_phases.items() if len(eclass) > 1 | ||
| ) - set(defined_phases) | ||
|
|
||
| for missing in missing_custom_phases: |
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 code is equivalent to above.
# use the pkg's eapi, not latest. Conflicts in EAPI's the pkg isn't, don't matter for it until it is that EAPI/
possible_conflicts = set(pkg.eapi.phases_rev)
# strip out what the pkg defines
for node in bash.func_query.captures(pkg.tree.root_node).get("func", ()):
func_name = pkg.node_str(node.child_by_field_name("name"))
possible_conflicts.discard(func_name)
for phase in possible_conflicts:
# iterate the remaining phases, finding where there are multiple eclasses providing this phase.
eclass_sources = []
for eclasses, _ in inherits:
for eclass in eclasses:
# I assume .functions supports __contains__, and isn't strictly typing.Iterable[str]
if f'{eclass}_phase' in eclass.functions:
eclass_sources.append(eclass)
if len(eclass_sources) > 1:
yield ShadowedEclassPhase(phase, eclass_sources, pkg=pkg)
I'd also rename providers to eclasses in ShadowedEclassPhase to be clearer.
| # Strip out phases we already define (even if inside of those, we don't | ||
| # actually call exported phases from all eclasses inherited). Assume that | ||
| # a custom phase in the ebuild is intentionally omitting them. | ||
| missing_custom_phases = set( |
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.
We should also filter out eclasses where of those defining the phase, they have @PROVIDES which has the phase (because it might wrap the eclass and call that phase for you).
Detect the basic case of an ignored eclass phase where two eclasses are
inherited, both defining the same phase, and the ebuild does not define
a custom implementation of that phase at all. This means one of the exported
phases from the eclasses is being ignored.
Ignore some eclasses with a blacklist where they are known to vary their
API by EAPI or eclass variables, at least for now, because the eclass
cache we have accessible here isn't keyed by EAPI or the context of the
sourcing ebuild.
Bug: https://bugs.gentoo.org/516014
Bug: https://bugs.gentoo.org/795006
Closes: #377
Signed-off-by: Sam James sam@gentoo.org