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

_emerge: depgraph: lookup unrecognized atoms in updates #1234

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

thesamesam
Copy link
Member

@thesamesam thesamesam commented Jan 13, 2024

This isn't suitable for merging right now, just posting so I can ask for some help.

Bug: https://bugs.gentoo.org/910572


# TODO: Doing the lookup here unfairly penalises the success case
if atomarg_atom.package and not atomarg_atom.cp.startswith("null/"):
found_any_match = any(
Copy link
Member Author

@thesamesam thesamesam Jan 13, 2024

Choose a reason for hiding this comment

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

@zmedico Do you have an idea for how to best do this? In particular, I don't want to penalise the success case (so "dev-build/make" having to incur a lookup).

Of course, could do the lookup in ambiguous_package_name, but that would only then help for a warning, not for fixing it up...

Copy link
Member

@zmedico zmedico Jan 14, 2024

Choose a reason for hiding this comment

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

Maybe we can do this expansion after _dep_expand, and possibly only do it after category expansion has failed?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, pushed a fixup (not squashed it). I tried a few things, including maybe treating this like the old PROVIDE case for virtuals, but this seemed to be the most elegant.

What do you reckon?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like _select_package would be better than _iter_match_pkgs_any.

Copy link
Member

@zmedico zmedico Jan 15, 2024

Choose a reason for hiding this comment

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

Maybe just use _iter_match_pkgs_any to translate the category/package-name, and let _select_package handle the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, in 2a0f564 (#1234), it works, but I'm calling _add_pkg myself rather than relying on _select_package and then the _add_pkg later on in the function.

In 2cbc189 (#1234), I try to use the existing logic more, but that doesn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

_select_package ends up replacing it with an installed Package:

pkg=<Package ('binary', '/tmp/tmpruf_hub6/', 'dev-build/make-4.4', 'merge', 'None', '225', '0', '1705479700')>
   binary: dev-build/make-4.4::test_repo
installed: dev-build/make-4.4::test_repo
pkg=<Package ('installed', '/tmp/tmpruf_hub6/', 'dev-build/make-4.4', 'nomerge', 'installed')>

Copy link
Member

@zmedico zmedico Jan 18, 2024

Choose a reason for hiding this comment

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

So, _select_package operates on data from _set_args, so you need to call _set_args at some point with updated args. This loop isn't a good place to call _set_args, since it's supposed to be called before any packages are added to the graph (before _select_package is ever called), and called with all args at once rather than a single arg. Anyway, the tests pass with this diff:

--- a/lib/_emerge/depgraph.py
+++ b/lib/_emerge/depgraph.py
@@ -5321,8 +5321,12 @@ class depgraph:
 
                         if pkg:
                             atom = Atom(atom.replace(atom.cp, pkg.cp))
+                            arg.atom = dep.atom = atom
+                            arg.pset.replace([atom])
+                            self._set_args([arg])
 
-                            if not self._add_pkg(pkg, dep):
+                            # goto _select_package below
+                            if False:
                                 writemsg(
                                     f"\n\n!!! Problem resolving dependencies for {arg.arg=}, {arg.atom=}\n",

Copy link
Member

Choose a reason for hiding this comment

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

For example, you might modify args inside _select_files in the for x in myfiles: loop that creates the args list, or just after that loop. Then your transformations will already be present when args is passed to _set_args.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Zac, I'm going to try get back to this tonight.

@thesamesam thesamesam force-pushed the rename-suggest-bug910572 branch 5 times, most recently from 45f0c61 to 5182c75 Compare January 15, 2024 03:51
lib/_emerge/depgraph.py Outdated Show resolved Hide resolved
lib/_emerge/depgraph.py Outdated Show resolved Hide resolved
Signed-off-by: Sam James <sam@gentoo.org>
Signed-off-by: Sam James <sam@gentoo.org>
Signed-off-by: Sam James <sam@gentoo.org>
This reverts commit 2cbc189.
@thesamesam thesamesam force-pushed the rename-suggest-bug910572 branch from 9665c1e to ee1d83f Compare January 17, 2024 08:11
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.

2 participants