search

gap-system / gap

Main development repository for GAP - Groups, Algorithms, Programming, a System for Computational Discrete Algebra
https://www.gap-system.org
GNU General Public License v2.0
805 stars 161 forks source link

Utils should not have any dependencies #5207

Closed cdwensley closed 1 year ago

cdwensley commented 1 year ago

There have been emails from Bill Allombert to gap-request concerning the dependencies of the Utils package and the need to avoid circular dependencies. There were 3 packages listed: GAPDoc, AutoDoc and Polycyclic. The first two of these have been removed.

@fingolfin commented on Nov.14 that "A quick "grep" reveals that Polycyclic is a dependency for (at least) function PcGroupToMagmaFormat which supports Pcp groups, not just pc groups." and this was a function originally supplied by Max which does not appear to be used anywhere in the library.

Bill's latest email is as follows: "Problematic might not be the word, but it would make more sense if utils would only depend on GAP. This would simplify the dependency graph and avoid circular dependencies. As I understand, the goal of utils is to share code between packages. If it starts to depend on other packages, then this leads a a situation when every packages depend on each other. For example PcGroupToMagmaFormat could be moved to Polycyclic (and if it makes it necessary, Polycyclic would require utils). Or PcGroupToMagmaFormat would only support Pcp if Polycyclic is installed too. To be honest, it is the dependency on AutoDoc that worried me. Without it I would probably not have written this email."

So this seems serious enough to be turned into an issue. Are there any opinions on the proposal to move PcGroupToMagmaFormat to Polycyclic?

fingolfin commented 1 year ago

PcGroupToMagmaFormat is, as the name suggest, primarly for pc groups. The support for pcp groups is purely for convenience. Just remove it, and then you don't need polycyclic (at least not for that).

ThomasBreuer commented 1 year ago

I agree with Bill's conceptual verdict: The idea behind the utils package is that it offers functionality that is quite general and is expected to be useful for other packages or for interactive sessions, and thus utils should better not depend on other packages.

A possible solution for the PcGroupToMagmaFormat method would be to split it into two methods, one for IsPcGroup and one for IsPcGroup and IsPcpGroup, and to install the latter only if the Polycyclic package is available.

fingolfin commented 1 year ago

I am not sure why this is an issue on the GAP repository, though -- isn't this an issue specific to the utils package?

fingolfin commented 1 year ago

A possible solution for the PcGroupToMagmaFormat method would be to split it into two methods, one for IsPcGroup and one for IsPcGroup and IsPcpGroup, and to install the latter only if the Polycyclic package is available.

I really don't like this approach (although I am keenly aware it is used in multiple packages): it means that things work differently depending on the order in which packages are loaded. It means that the same code may work for one user but not another for an (to them) obscure reason. (See also issue #4834)

Anyway, see https://github.com/gap-packages/utils/pull/58 (I've not tried to remove polycyclic from the dependencies, as I did the edit from a web browser and can't verify right now whether there are other uses of polycyclic remaining)

ThomasBreuer commented 1 year ago

@fingolfin I do not think that the order of loading packages is an issue here. The recommended way to check inside the code whether another package will be available later in the session is to call IsPackageMarkedForLoading. If Polycyclic is listed as a suggested package of utils then loading utils will trigger that also Polycyclic will get loaded if possible. (Well, the latter does not hold if the user has decided to ignore suggested packages, and then there is a reason for that.)

Thus the question is whether we want to have suggested packages of utils. Currently utils suggests loading CurlInterface, because the Download function of utils benefits from CurlInterface, which is a situation different from that with PcGroupToMagmaFormat.

cdwensley commented 1 year ago

Utils PR #58 now merged and Polycyclic removed from the needed packages. Ready to release utils-0.78 (if I could get ReleaseTools to work).

The reason this is a GAP issue is that the underlying problem is that of circular dependencies. As mentioned by @ThomasBreuer, CurlInterface remains as a suggested package.

fingolfin commented 1 year ago

Then it is not clear to me from the description of this issue what the issue is. Please clarify.

cdwensley commented 1 year ago

This was initially Bill's email query, and I got the impression that circular dependencies was a more general problem than that with Utils - maybe I was mistaken. Now that Utils has been fixed, this issue can be closed.