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
813 stars 161 forks source link

CI: check for some/all warnings when compiling the manuals? #4456

Open wilfwilson opened 3 years ago

wilfwilson commented 3 years ago

It's very easy for changes to the GAPDoc code to introduce slight problems into the compilation of the manual. Currently, I think our CI only catches problems with the manuals if they completely break compilation.

For example, when compiling the manuals on the current master branch, you get the following warning (which will be fixed by PR #4455) https://github.com/gap-system/gap/runs/2497246378?check_suite_focus=true:

#W There were LaTeX Warnings:
LaTeX Warning: Label `FactorCosetAction' multiply defined.

)
____________________
LaTeX Warning: There were multiply-defined labels.

Of course, these kinds of warnings are not serious, but it seems that we still try to get rid of them – in particular, the second run-through when compiling the manuals normally produces no warnings.

Therefore I pose the following question: do we want to check for such warnings in the CI tests on pull requests? Would it be too annoying to contributors to insist on this?

olexandr-konovalov commented 3 years ago

We want, but how? I stopped short of this in #156. Also, you can use another kind of check - make check-manuals in #3124 - I am not sure if our new CI is using it.

olexandr-konovalov commented 3 years ago

@wilfwilson it might be that instead of trying to detect issues from the LaTeX output you need to analyse the collected xml document, like make check-manuals does, and decide what's treated as a warning and what's as a failed test (and move soft errors to hard errors as the cleanup proceeds).

For the reference, when calling make check-manuals on the current master branch, I have

****************************************************************
*** Checking types in ManSections 
****************************************************************
./../../lib/matobj.gi:728 : \- uses Meth with no matching Oper/Attr/Prop
./../../lib/matobj.gi:957 : \- uses Meth with no matching Oper/Attr/Prop
./../../lib/magma.gd:673 : Centralizer uses Attr instead of Oper
./../../lib/grp.gd:3589 : GrowthFunctionOfGroup uses Oper instead of Attr
./../../lib/oprt.gd:1303 : Orbit uses Oper instead of Func
./../../lib/oprt.gd:1342 : Orbits uses Oper instead of Attr
./../../lib/oprt.gd:1376 : OrbitsDomain uses Oper instead of Attr
./../../lib/oprt.gd:1433 : OrbitLength uses Oper instead of Func
./../../lib/oprt.gd:1455 : OrbitLengths uses Oper instead of Attr
./../../lib/oprt.gd:1485 : OrbitLengthsDomain uses Oper instead of Attr
./../../lib/oprt.gd:1527 : OrbitStabilizer uses Oper instead of Func
./../../lib/oprt.gd:1029 : SparseActionHomomorphism uses Oper instead of Func
./../../lib/oprt.gd:
1031 : SortedSparseActionHomomorphism uses Oper instead of Func
./../../lib/oprt.gd:2142 : CycleLengths uses Oper instead of Func
./../../lib/oprt.gd:1804 : IsTransitive uses Oper instead of Prop
./../../lib/oprt.gd:1614 : Transitivity uses Oper instead of Attr
./../../lib/oprt.gd:1994 : RankAction uses Oper instead of Attr
./../../lib/oprt.gd:1911 : IsSemiRegular uses Oper instead of Prop
./../../lib/oprt.gd:1948 : IsRegular uses Oper instead of Prop
./../../lib/oprt.gd:1778 : Earns uses Oper instead of Attr
./../../lib/oprt.gd:1842 : IsPrimitive uses Oper instead of Prop
./../../lib/oprt.gd:1660 : Blocks uses Oper instead of Func
./../../lib/oprt.gd:1662 : Blocks uses Attr instead of Func
./../../lib/oprt.gd:1704 : MaximalBlocks uses Oper instead of Func
./../../lib/oprt.gd:1706 : MaximalBlocks uses Attr instead of Func
./../../lib/oprt.gd:
1742 : RepresentativesMinimalBlocks uses Oper instead of Func
./../../lib/oprt.gd:
1745 : RepresentativesMinimalBlocks uses Attr instead of Func
./../../lib/oprt.gd:1208 : ExternalSet uses Oper instead of Attr
./../../lib/oprt.gd:1259 : ExternalSubset uses Oper instead of Func
./../../lib/oprt.gd:1281 : ExternalOrbit uses Oper instead of Func
./../../lib/oprt.gd:1549 : ExternalOrbits uses Oper instead of Attr
./../../lib/oprt.gd:1575 : ExternalOrbitsStabilizers uses Oper instead of Attr
./../../lib/ratfun.gd:1401 : Discriminant uses Oper instead of Attr
./../../lib/ratfun.gd:
1351 : TaylorSeriesRationalFunction uses Attr instead of Func
./../../lib/tom.gd:1506 : CyclicExtensionsTom uses Oper instead of Attr
./../../lib/ctbl.gd:1110 : IsAlmostSimple uses Prop instead of Oper
./../../lib/ctbl.gd:1114 : IsMonomial uses Prop instead of Oper
./../../lib/ctbl.gd:1115 : IsNilpotent uses Prop instead of Oper
./../../lib/ctbl.gd:1116 : IsPerfect uses Prop instead of Oper
./../../lib/ctbl.gd:1117 : IsSimple uses Prop instead of Oper
./../../lib/ctbl.gd:1118 : IsSolvable uses Prop instead of Oper
./../../lib/ctbl.gd:1119 : IsSporadicSimple uses Prop instead of Oper
./../../lib/ctbl.gd:1120 : IsSupersolvable uses Prop instead of Oper
./../../lib/ctbl.gd:2469 : DecompositionMatrix uses Oper instead of Attr
./../../lib/ctbl.gd:3864 : CharacterTableIsoclinic uses Oper instead of Attr
./../../lib/ctbl.gd:3865 : CharacterTableIsoclinic uses Oper instead of Attr
./../../lib/ctbl.gd:3867 : CharacterTableIsoclinic uses Oper instead of Attr
./../../lib/ctblmaps.gd:
584 : FusionConjugacyClassesOp uses Oper instead of Attr
./../../lib/ctblmono.gd:608 : TestMonomial uses Oper instead of Attr
./../../lib/ctblmono.gd:610 : TestMonomial uses Oper instead of Attr
./../../lib/ctblmono.gd:763 : TestRelativelySM uses Oper instead of Attr
./../../lib/ctblmono.gd:765 : TestRelativelySM uses Oper instead of Attr
****************************************************************
*** Checking types in cross-references 
****************************************************************
Found 7979 Ref elements including 7907 within the Reference manual
./../../lib/tuples.gd:177 : Ref to CollectionsFamily uses Filt instead of Attr
./fixconsistency.sh 'CollectionsFamily' Filt Attr ./../../lib/tuples.gd 177
./../../grp/classic.gd:
1059 : Ref to GeneralOrthogonalGroup uses Oper instead of Func
./fixconsistency.sh 'GeneralOrthogonalGroup' Oper Func ./../../grp/classic.gd 
1059
./../../grp/classic.gd:
1095 : Ref to SpecialOrthogonalGroup uses Oper instead of Func
./fixconsistency.sh 'SpecialOrthogonalGroup' Oper Func ./../../grp/classic.gd 
1095
****************************************************************
Selected 3944 ManSections of the following types:
Attr - 785
Fam - 8
Filt - 370
Func - 1406
InfoClass - 21
Meth - 98
Oper - 975
Prop - 237
Var - 44
Found 52 errors in ManSection types 
Selected 6610 Ref elements referring to ManSections 
Found 0 errors and 3 warnings in Ref elements 
****************************************************************
olexandr-konovalov commented 3 years ago

P.S. The list of "Found 52 errors in ManSection types" above is not that simple - some entries related to orbitish operations are ambiguous. If we want to eliminate warnings completely, one would likely come up with some exceptions from default rules of deciding what the label should be.

ThomasBreuer commented 3 years ago

I think that the heuristic tests used by make check-manuals are not very useful. For example, several of the abovementioned messages of the form uses Oper instead of Attr are wrong; note that there are attributes which are also declared as operations (with more than one argument).

olexandr-konovalov commented 3 years ago

Yes @ThomasBreuer that's what I meant in my comment above - after fixing a number of cases when they were producing correct diagnosis, what's left is mostly wrong. If we want to use them to catch genuine cases of using wrong types (e.g like those fixed in #3105 and #3112, we would have to hardcode exceptions.

frankluebeck commented 3 years ago

Of course, some systematic checks of manuals, also in CI, would be useful.

I have added a utility to GAPDoc (see https://github.com/frankluebeck/GAPDoc/commit/a5a175e2c2a56e76c7fb5b0a83ca66723c6f2cfd) which checks the validity of XML and in particular of GAPDoc documents. (Detected a few errors, see "Fix XML errors in main manuals. #4472".) The IO package and the external program xmllint are needed for this.

The file doc/ref/testconsistency.g is in a strange place. It would be better to add some functions of this kind to the GAP library, such that they can easily be called for any help book.

Maybe the current Oper instead of ... messages which do not point to actual errors could be avoided by considering the ManSection for an attribute or property as of type "Oper" when the Arg attribute contains a comma (i.e., when the operation is called with multiple arguments).