Removing prim, small and trans directories

[olexandr-konovalov]

This PR completes the switch from data libraries contained in the prim, small and trans directories to the three new packages, called PrimGrp, SmallGrp and TransGrp:

• PrimGrp
  • repository: https://github.com/gap-packages/primgrp
  • website: http://gap-packages.github.io/primgrp/
• SmallGrp
  • repository: https://github.com/gap-packages/smallgrp
  • website: https://gap-packages.github.io/smallgrp/
• TransGrp:
  • repository: https://github.com/hulpke/transgrp
  • website: http://www.math.colostate.edu/~hulpke/transgrp/

This switch consists of two phases.

The first one has been implemented in #1714 and allowed to load either a new package for a data library, if present, or load the existing prim, small or trans directory otherwise.

This PR #1650 implements the 2nd stage. It removes that provisional code, together with removing prim, trans, and small directories and parts of documentation that has been migrated to the packages. It also makes PrimGrp, SmallGrp and TransGrp packages needed to run GAP. This ensures that a proper error message will be displayed in case any of these packages is missing, and that the code that expects the relevant functionality to be available, will still work.

Warning: when this PR will be merged, GAP will not start if any of PrimGrp, SmallGrp or TransGrp packages are missing. It will be necessary either to remove pkg directory and call make bootstrap-pkg-full to get the latest collection of GAP packages for the master branch, or to install these packages manually from the URLs above.

Remark 1: I've checked that all latest changes in prim directory are already contained in PrimGrp. For the same check for the trans directory, see hulpke/transgrp#17. Commit 3d86926 has been applied to SmallGrp in gap-packages/smallgrp@423ee7f.

Remark 2: an early version of this PR also removed declaration of (undocumented) PrimitiveGroupsAvailable from the library - that change has been later reverted.

[codecov]

Codecov Report

Merging #1650 into master will increase coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1650      +/-   ##
==========================================
+ Coverage   63.12%   63.49%   +0.36%     
==========================================
  Files         967      952      -15     
  Lines      290317   286802    -3515     
  Branches    12746    12746              
==========================================
- Hits       183272   182104    -1168     
+ Misses     104243   101898    -2345     
+ Partials     2802     2800       -2

Impacted Files	Coverage Δ
hpcgap/lib/system.g	66.03% <ø> (ø)	⬆️
hpcgap/lib/package.gi	36.72% <ø> (-0.47%)	⬇️
hpcgap/lib/hpc/thread1.g	29.46% <0%> (-0.9%)	⬇️
src/iostream.c	46.24% <0%> (-0.4%)	⬇️
src/hpc/traverse.c	78.29% <0%> (-0.39%)	⬇️
lib/package.gi	36.33% <0%> (-0.31%)	⬇️
hpcgap/lib/hpc/stdtasks.g	38.61% <0%> (-0.26%)	⬇️
src/stats.c	79.01% <0%> (-0.14%)	⬇️
src/read.c	83.56% <0%> (ø)	⬆️
... and 21 more

[Stefan-Kohl]

Should this go into GAP 4.9, GAP 4.10 or GAP 5.0?

What is the point of splitting off the primitive groups library and moving it into a package if that package is then still necessary to run GAP? Wouldn't it make sense to use this kind of modularization to reduce the amount of code which is needed by GAP? -- Unlike e.g. GAPDoc, the primitive groups library is rather not a central part of GAP, and there are so many applications of GAP which just don't benefit from it being loaded.

Certainly, having PrimGrp not always loaded would require updating package dependencies accordingly. This suggests to perform such change together with splitting off the transitive groups library and the small groups library, and also together with multiple other planned changes with limited backwards compatibility, say for GAP 5.0.

[markuspf]

This is planned to go into 4.9; we decided to make the package required to give package authors time to add this package to their list of dependencies and to not just break packages because we can (the same holds true for the transitive groups and the small groups libraries).

[fingolfin]

This looks pretty good to me (only some minor remark/question below), but I have not actually tried to use it.

[fingolfin]

The commit "Use package archives for the master branch for bootstrapping" really should be first in this PR.

Also, what exactly is the difference between those two tarballs? Is bootstrap-pkg-full.tar.gz th packages from "the last stable release", and packages-master.tar.gz "the latest versions of packages we have", or what? If so, shouldn't bootstrap-pkg-full.tar.gz better be called packages-stable.tar.gz?

[olexandr-konovalov]

@fingolfin do you want to squash commits or rebase interactively and reorder them? Commits are atomic and should work in the order you suggest too.

bootstrap-pkg-full.tar.gz is a symlink pointing to packages-v4.8.8.tar.gz, so it contains packages from "the last stable release". We also have packages-stable-4.8.tar.gz which is at this moment same as packages-v4.8.8.tar.gz.

It may be sensible idea to point bootstrap-pkg-full.tar.gz to packages-stable-4.8.tar.gz since we may be able to put there also some package updates which are not yet in the the last stable release.

[olexandr-konovalov]

In addition, packages-master.tar.gz has some more packages that require GAP 4.9 and not redistributed with GAP 4.8 - at the moment, besides PrimGrp those are crypting, uuid and ZeroMQinterface.

[fingolfin]

I meant an "interactive rebase": The point is that logically, the commits before the "Use package archives [...]" depend on it (w/o it, their Travis tests would fail).

[fingolfin]

Huh, wait -- that would then also affect the next stable release -- unless we change it back on the stable branch, or even change it in each release... Is that the plan?
In that case, it might be better to factor out the master bit into a separate variable, say, PKG_BRANCH and then do something like this (untested):

PKG_BRANCH = master  # change to stable-X.Y in stable branch
PKG_MINIMAL = packages-required-$(PKG_BRANCH).tar.gz
PKG_FULL = packages-$(PKG_BRANCH).tar.gz

[olexandr-konovalov]

Yes, I consider that change to be temporary, otherwise it will affect the next stable release of GAP 4.9.1 (not of GAP 4.8.9 if it will happen). The idea of $PKG_BRANCH$ seems good to me. I will try it and then will do an "interactive rebase".

Indeed, I've managed to do it in reverse order because I had all packages locally, and then pushed the whole branch with 4 commits.

[fingolfin]

Shouldn't this also extract the tests for the primitive groups library into package tests? In particular, parts or all of these files:

• tst/testextra/grpperm.tst
• tst/testextra/primsan.tst
• perhaps also others in which PrimitiveGroup occurs...

[fingolfin]

@alex-konovalov tells me he knows about the tests and plan to migrate them later. While strictly speaking it would be nice to migrate them right away, I of course fully understand this requires yet more work. It is also not strictly necessary. So I don't think we need to hold this PR back for it.

[hulpke]

Why are you removing this function. It got added specifically for the fact that `prim' got moved into a package -- this way library functions that might use the primitive groups library -- e.g. normalizer in S_n -- have a way of testing cleanly whether the library is available, regardless whether it is through a package or other means.
This function is used e.g. in lib/gpprmsya.gi for exactly this purpose.

[olexandr-konovalov]

@hulpke I guess by "this" you mean PrimitiveGroupsAvailable. I thought of it as of merely a transitional measure, since it was first assigned to a dummy function in the GAP library, and then the package was re-declaring it with

# Availability test for the library
# ensure that the dummy binds from the library are killed
MakeReadWriteGlobal("PrimitiveGroupsAvailable");
Unbind(PrimitiveGroupsAvailable);
DeclareGlobalFunction("PrimitiveGroupsAvailable");

and then assigning it to the function that returns deg in PRIMRANGE. That's why I've decided that there is no need to keep it any more: since PrimGrp package will be needed to run GAP, the proper PrimitiveGroupsAvailable will be available from the start anyway.

Also, there was an outdated comment that PrimitiveGroupsAvailable "will, if needed, load basic data structures for this degree". It was removed, since return deg in PRIMRANGE; does not actually load anything.

If that's OK, would you still like some changes to this PR?

[hulpke]

@alex-konovalov
You asked for my opinion and I've given it, but it is your package and your PR -- so if you want to go ahead with it as is, then do so.

I look at this change as not resolving any issue but making it harder to get to a point where GAP could run without the primitive groups library, even if it would mean a loss of much of functionality.

(A similar function exists for the transitive groups library and for that I'm going to insist that it remains in the library)

[olexandr-konovalov]

It came to my mind that one may want to wait and replace all three directories by three new packages in a single PR. This is because at the moment master branch is tested with the set of packages from the latest stable release. If we merge 1650 now, it will use the archive for master branch for all tests. Hence our CI tests will start to fail as soon as e.g. TransGrp will be added to the archive, but there will be no merged PR which makes TransGrp needed to run GAP. Might happen that we will need a "fast-track" PR for that then. But since TransGrp is released and SmallGrp release seems close, might wait indeed.

[olexandr-konovalov]

@hulpke Thanks. I think with these libraries becoming proper packages the next step would be to use IsPackageMarkedForLoading for such things, like we are doing for AtlasRep, FactInt and CTblLib already.

[ChrisJefferson]

What are you using ispackagemarkedforloading for?

[fingolfin]

I actually agree with @hulpke as I just realized that the latest version of this package is not compatible with master before this PR gets merged, which is extremely annoying - and there seems no good reason for that either.

[olexandr-konovalov]

@ChrisJefferson IsPackageMarkedForLoading returns this:

IsBound( GAPInfo.PackagesLoaded.(name) ) 
      and CompareVersionNumbers( GAPInfo.PackagesLoaded.(name)[2], version, 
         equal );

It is used in several places (grep in lib and grp directories in GAP). If it returns false, then GAP uses fallback implementations in the GAP library. This, in particular, allows us to test these fallback implementations when GAP is started with -A.

[olexandr-konovalov]

@fingolfin I hoped that nobody would find that extremely annoying since the switch to using packages-master.tar.gz in CI tests is a part of this PR, so changes in the package and in the library are synchronised.

OK, that's my current plan: I will release new PrimGrp and update this PR to make requested changes. In the same time I will update packages-master.tar.gz to contain TransGrp and hopefully SmallGrp, which seems close to the 1st release. Then this PR will evolve and will see the removal of all three directories and relevant parts of documentation that were moved to corresponding packages. Probably I can take this opportunity to move some more tests there too.

This may take another week, but now sounds much more concrete and realistic than two weeks ago - I'm sure it will be completed before long, and we will have one more serious TODO less for GAP 4.9 beta.

[olexandr-konovalov]

@fingolfin by the way, even if I will follow the plan from the previous comment, master branch will still be broken: it has code

if TestPackageAvailability("primgrp")=fail then
  PRIM_AVAILABLE:=ReadPrim( "primitiv.gd","primitive groups" );
...

This PR removes it. But if one uses the package in the master branch with this code in there, then TestPackageAvailability("primgrp") returns true, the code is not loaded, but the package is not loaded either. Hence tests that require the primitive groups library will break anyway. Same happens with TransGrp.

[olexandr-konovalov]

I've made the change in gap-packages/primgrp@90f2b1f. This will be in the next release of PrimGrp, after which I will update the PR too.

Tests of the master branch will now show diffs if the package is not loaded, but at least there will be no break loop.

[olexandr-konovalov]

The next version of this PR also replaces trans by the TransGrp package. It takes into account suggestions by @fingolfin and @hulpke. I am now waiting for @markuspf completing the SmallGrp package, and then pick it up for the redistribution, add it to packages-master.tar.gz, and then remove small directory, make the package needed to run GAP and clean up the documentation. This will make this PR ready for merging.

Note that after this PR, GAP will not start if these will be missing. It will be necessary either to remove pkg directory and call make bootstrap-pkg-full to get the latest collection of GAP packages for the master branch, or to install these packages manually.

[fingolfin]

This is tough to review on the GitHub web interface, since there are 4281 removed files and over 3 million removed lines. But using command line, it seems that, other than the removed data files, just refs in the documentation are adjusted.

So assuming everything else works (if the required packages are present), then I have no objections to this PR.

Objections have been addressed

[fingolfin]

Note that this PR needs to be rebased.

[olexandr-konovalov]

@fingolfin thanks - rebased.

[olexandr-konovalov]

@hulpke you have requested changes - are you happy with the way how it looks now? If so, could you please change the status of your review?

[olexandr-konovalov]

@hulpke I've discovered that TransGrp breaks CTblLib, because it replaces TRANSDEGREES with the boolean list TRANSAVAILABLE. What is completely sensible, of course, since there is no more continuous availability of degrees, due to degree 32 requiring a separate download.

I've added TRANSDEGREES to obsolete.gd and explained the situation in the comment there. I will notify @ThomasBreuer subject to passing tests and merging this, so that he can eventually adjust CTblLib for the new TransGrp package.

(Update - this commit was later submitted as a separate PR #1852).

[fingolfin]

The description of this PR claims "It makes PrimGrp and TransGrp packages needed to run GAP". But that does not seem to be the case (also, smallgrp is missing from that list).

Please update the description of this PR to accurately explain what it contains.

Also, I am not sure we should merge this as-is -- with it, one can load packages that need transgrp but do not yet declare a dependency on it. I thought the plan was to make all three packages required for at least th e4.9 cycle (resp. till all packages which need it declare the dependency explicitly.

[olexandr-konovalov]

@fingolfin Ok, I will update description - it is a relic version from the 1st time this PR was submitted. Also, you do not see that they are made needed since this now has been moved to #1714 where a provisional arrangement has been made to make them needed only if prim/trans/small directories are missing. But now we're removing those directories, so those provisional arrangements may made into firm ones. I will do this here and update the PR. As we figured out, this will help users now having those packages to understand what happened, instead of seeing an unhelpful error.

[olexandr-konovalov]

@fingolfin: rebased; issue description updated; provisional code removed. Now three new packages are always needed to run GAP. If any of them is not available, GAP displays a proper error message telling that a package is missing.

Commit which adds TRANSDEGREES to obsoletes (used by CTblLib) was moved to #1852 and has been merged there.

[fingolfin]

Looks good to me now. The only thing I'd consider changing is making the last commit the first; I.e. first stop using trans/prim/small, then remove them.

[olexandr-konovalov]

@fingolfin: Rebased, reordered as suggested, and also extended with synchronising hpcgap/lib/galois.gi and taking out TRANSREGION which will appear in TransGrp. Tests passed - should be ready to merge now.