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

Fix AUR dependency resolving #2169

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

smolx
Copy link
Contributor

@smolx smolx commented May 18, 2023

Before this fix dependencies for AUR targets were added to the graph after each addition of a target node. Now dependencies are added only after all target nodes are added to the graph.

Also added some tests for previously bugged cases.

Fixes #2157.

Before this fix dependencies for AUR targets were added to the graph
after each addition of a target node. Now dependencies are added only
after all target nodes are added to the graph.

Also added some tests for previously bugged cases.
@smolx smolx requested a review from Jguer as a code owner May 18, 2023 14:57
@smolx
Copy link
Contributor Author

smolx commented May 18, 2023

I had the audacity to propose my solution to the issue.
Perhaps new and existing related methods could be named better. And there are definitely other things to improve.

I also found a similar pattern of dependency resolving in the GraphFromSrcInfo method. Maybe it should be fixed too. I haven't dug in it yet.

Copy link
Owner

@Jguer Jguer left a comment

Choose a reason for hiding this comment

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

From a first read it seems correct.

I had a doubt if it was working for recursive AUR dependencies but I haven't tried it out locally yet.

How does it handle yay -Syu ros-melodic-desktop?

The exists check that was removed had as an objective to prevent the computation of the dep graph for the same AUR package from repeating on each dependency layer

@smolx
Copy link
Contributor Author

smolx commented May 23, 2023

With this fix there is a significant difference only for cases with multiple targets, since it doesn't affect the recursive part.

yay -Syu ros-melodic-desktop and yay -S ros-melodic-desktop are handled well:

Sync Make Dependency (16)
Sync Dependency (18)
AUR Explicit (1)
AUR Dependency (136)
AUR Make Dependency (66)

It gives more or less the same result as the next branch. "More or less" is because amount of make and regular dependencies change on each run, even on the next branch. :)

The exists check that was removed had as an objective to prevent the computation of the dep graph for the same AUR package from repeating on each dependency layer

If I figured it out correctly, repeated computation during recursive traversal is prevented completely in the Grapher.addNodes method. So that removed check would help(in combination with this fix) only with cases of duplicate targets. Though performance effect is minimal. Target duplicates could be filtered out much more earlier.

@Jguer
Copy link
Owner

Jguer commented May 23, 2023

Still very happy to see this implemented. Let's merge it in

@Jguer Jguer merged commit c1aa71b into Jguer:next May 23, 2023
@Jguer
Copy link
Owner

Jguer commented May 23, 2023

I also found a similar pattern of dependency resolving in the GraphFromSrcInfo method. Maybe it should be fixed too. I haven't dug in it yet.

GraphFromSrcInfo has some low hanging fruit to fix if I remember correctly.

@smolx smolx deleted the fix-aur-dependency-graph branch May 26, 2023 13:38
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.

yay attempts to install net-new AUR package for dependencies when dependency is already satisfied
2 participants