[arrowd]

No description provided.

[vstakhov]

Don't we have topological sort for that purposes?

[arrowd]

Not sure what topological sorting has to do with this issue. In the test I added there are just two packages depending on each other. Without this fix the pkg add command just goes OOM due to infinite recursion.

[vstakhov]

Not sure what topological sorting has to do with this issue

That's the main purpose of topological sort. You're proposing to use O(N^2) check for names lookup that doesn't look nice. If you need something like "seen" definition, you need a simple set.

If you need dependency graph lookup to find cycles, pkg already has topological sort for that somewhere in the solver. For sure, O(N^2) here is usually not a problem once number of packages that are added is low, but this sort of things are usually just waiting to blow up for someone in the future.

I'm also pretty sure that pkg_add should add packages with their dependencies in reversed topological order. On the other hand, I think it was already solved on pkg install level, leaving pkg_add a simple and straightforward call just to install package in system without any sense of deps and other complexities. So what is the initial motivation for this change I wonder?

[arrowd]

That's the main purpose of topological sort. You're proposing to use O(N^2) check for names lookup that doesn't look nice. If you need something like "seen" definition, you need a simple set.

Yes, ideally we want a set there, but since this is not C++ and there is no <unordered_set> in the standard library, I just picked whatever was at hand.

For sure, O(N^2) here is usually not a problem once number of packages that are added is low, but this sort of things are usually just waiting to blow up for someone in the future.

That was my assumption that the number of packages we go over when doing pkg add is usually low. But I agree, we might get into trouble with this, because I never remove items from that vector.

So what is the initial motivation for this change I wonder?

I just wanted to add a safety belt. We haven't seen cycles in the official repos until now, but with shlib provides/requires being enabled it is now possible to pick up a wrong provider package, which in turn will lead to a cycle. This change allows for a graceful failure in this case.

[vstakhov]

I just wanted to add a safety belt.

Got it, then I guess it will be suitable to use a set and be sure it's not leaking memory on error paths. Upper interfaces, such as pkg-install, provides safeguards from cycles but it's always good to have some extra layer of safety if it's cheap :)

[arrowd]

I'll employ stringset here, it seems to fit perfectly.