changes in lib/oper1.g

[ThomasBreuer]

Yesterday I tried to make a harmless little change in lib/oper1.g.
(Of course, experienced developers know that changes in this file are never harmless.)

When I had created the pull request, the CI tests reported failures.

• The first one showed that also the file src/c_oper1.c must be updated (which can be done by calling make docomp). The failure happened in a strange place, a call to bin/BuildPackages.sh. Namely, the function run_configure_and_make tries to determine the name of a package by calling GAP, reading the PackageInfo.g file, extracting the name, and printing it. However, the first printed value was a message of the form #W Static module lib/oper1.g has CRC mismatch, ignoring, which is currently not a valid package name.
  Since GAP works nicely without src/c_oper1.c, I think the installation of packages should not fail because this file is not up to date. Perhaps GAP could simply be started with the -M option, in order to omit the warning message?

• The next question is why the generated file src/c_oper1.c must be under version control at all. It can be created when GAP gets installed. This would save some developers' work.

• After updating src/c_oper1.c, the HPCGAP related CI tests complained, since also the file src/hpc/c_oper1.c must be updated. For this update, one has to run make docomp for an installed HPCGAP.
  Again, the natural situation for creating src/hpc/c_oper1.c is installation time (this time the installation of HPCGAP), and the file would not need to be under version control.
  (Is it really expected from a GAP developer to install HPCGAP, just because of a harmless little change in lib/oper1.g?)

• The next failure was reported from the following test in tst/testinstall/varargs.tst.

  gap> Display(InstallMethod);
  function ( arg... )
      <<compiled GAP code>> from GAPROOT/lib/oper1.g:338
  end
  

  The aim of this test is to show an example of the <<compiled GAP code>> case, but the test actually checks that the code of a particular function occurs in a particular line of lib/oper1.g.
  Thus we are forced to adjust this testfile because of changes in lib/oper1.g. Is this acceptable?

  We could extend the code of Test such that the two outputs get modified according to some rules before they get compared, in order to admit predefined differences in tests.
  A rudimentary version of such an approach is taken in the tests for the JuliaInterface package --there are other instances where this problem arises.
  I could create a pull request for this functionality if others regard it as reasonable.

[fingolfin]

I agree that in principle it would be better if no generated files were in the repository, including the src/c_*.c files we generated from GAP code. And then also src/ffdata.h and src/ffdata.c.

Thing is, when I rewrote our build system, I contemplated doing all that, but ultimately decided against it because it seemed like a somewhat convoluted and risky task (at least if one wants to do it right... more on that later) and at the same didn't seem worth the effort, esp. compared to other needs. All in all I think it was the right call, as else I might not have finished the build system overhaul and I don't want to imagine the consequence. In practice, those GAP files are rarely modified, and we are already doing much better than in the distant past, when sometimes changes to those GAP files were not reflected by changes in the corresponding C files for months.

Anyway, the rest mostly follows from this decision:

• The CI tests are meant to fail if one changes lib/oper1.g, precisely so that we notice any oversights in this regard immediately. However, that it fails in the way you described (also visible in this build log) is a regression; there is a test later on which explicitly checks for this condition and reports it with a more helpful error. I think but have not verified that this patch would fix the regression:

diff --git a/bin/BuildPackages.sh b/bin/BuildPackages.sh
index fc9cce437..bb9ad7502 100755
--- a/bin/BuildPackages.sh
+++ b/bin/BuildPackages.sh
@@ -194,7 +194,7 @@ run_configure_and_make() {
   then
     if grep Autoconf ./configure > /dev/null
     then
-      local PKG_NAME=$($GAP -q -T -A -r <<GAPInput
+      local PKG_NAME=$($GAP -q -T -A -r -M <<GAPInput
 Read("PackageInfo.g");
 Print(GAPInfo.PackageInfoCurrent.PackageName);
 GAPInput

• yes, one needs to update both the regular GAP and the HPC-GAP version of the generated files. This requires building both GAP and HPC-GAP. But that's just a minor nuisance, isn't it? You say that one needs to "install HPC-GAP"; I wouldn't call it that way, this suffices:
  mkdir -p out-of-tree/hpcgap && cd out-of-tree/hpcgap && ../../configure --enable-hpcgap && make -j8 docomp
  Improving the documentation to document this would of course be helpful -- but it's simply been needed so rarely that nobody bothered (as with so many things in GAP... or any other kind of software, I guess)
• the test in varargs is done this way out of laziness: it simply was convenient to use an existing function produced by gac. These days, this printing is also tested in tst/testspecial/backtrace.g / tst/testspecial/backtrace.g.out, so we could remove it from varargs. Alternatively one could add an equivalent test to a file (existing or new) in tst/test-compile/.
• Note that tst/testspecial/run_gap.sh already contains code to "filter out" line numbers for files in the GAPROOT, which is why testing the printing there has no issues

Back to the issue of not adding the results of the gac compilation to the repository: back then, I am happy I ignored it. Now that the "new" build system is not new anymore, it may be worth re-exploring the options here, as we have a much better starting position.

The core problem is that to produce those files, we first need to compile GAP without them, and also pass -DAVOID_PRECOMPILED to the compiler when compiling src/compstat.c. Then, once we managed to produce them, compile GAP again. This can be done, but it's a bit tricky to get right. Here's the best way I can think of right now to do it:

1. add a custom build rule for a file src/compstat-nocomp.lo which has src/compstat.c as input, but a custom CPPFLAGS override which adds -DAVOID_PRECOMPILED (difficulty: easy)
2. add a new target executable gap-nocomp$(EXEEXT) (or whatever else to call it, good names are hard) which would be identical to that for gap$(EXEEXT) except that the src/c_*.lo dependencies are removed; and src/compstat.lo is replaced by src/compstat-nocomp.lo (difficulty: easy if you ignore Windows; otherwise medium (?))
3. add build rules for the generated files (taking HPC-GAP vs normal GAP into account) which use a gac made to use gap-nocomp$(EXEEXT) (difficulty: easy? I think? we "only" need to invoke cnf/gap-c-gen.sh in a different set of rules). I would also move the generated files from src to build

I think that's it: trying to make gap involves checking whether e.g. src/c_oper1.c is there (or rather: build/c_oper1.c); it not, make executes the build rule for it, which would probably initially look something like this:

build/c_oper1.c: lib/oper1.g gap-nocomp$(EXEEXT)
	@"$(abs_srcdir)"/cnf/gap-c-gen.sh $(abs_srcdir)/lib build oper1 ./gap-nocomp $(EXEEXT)

(Later, we could probably either substantially simplify cnf/gap-c-gen.sh or even do away with it and directly call gac again)

[ThomasBreuer]

@fingolfin thanks for the information.

Your pull request #4421 has solved the problem with bin/BuildPackages.sh.

I am not sure that the test in tst/testinstall/varargs.tst (about displaying a function) is covered by some test in tst/testspecial/backtrace.g (which are tests of the break loop). Ignoring line numbers in GAP output in the tests in tst/testspecial is achieved by shell commands, this feature is not available for the function Test.

For the treatment of the files in src that are generated by GAC, I think that a recipe how to proceed in case that one of these files is involved in a set of changes would be enough for the moment.

[fingolfin]

Regarding tst/testinstall/varargs.tst: What I meant is that tst/testspecial/backtrace.g as an unintended side effect also tests the same printing; indeed, intst/testspecial/backtrace.g.out, we have:

gap> InstallMethod( Matrix, [IsFilter, IsSemiring, IsMatrixObj], {a,b,c} -> fail );
Error, FLAGS_FILTER: <oper> must be an operation (not a function) in
  <<compiled GAP code>> from GAPROOT/lib/oper1.g:LINE in function INSTALL_METHOD called from 
<<compiled GAP code>> from GAPROOT/lib/oper1.g:LINE in function InstallMethod called from

Now, I am not saying this is sufficient, as it is just a side effect. But one could add tst/testspecial/vararg.g and move the Display(InstallMethod); invocation there to settle the issue.

Or, one could completely uncouple this test from the fact that lib/oper1.g is compiled to C, by instead adding a test to tst/test-compile -- just let it Display one of the compiled functions.

Of course adding pattern based output filtering to Test would be more general, but my point is that the immediate issue at hand can already now be dealt with quite effectively. (As an offside, for pattern based output filtering, we suffer from the fact that there is no proper regex implementation available in GAP)

[ThomasBreuer]

@fingolfin Thanks for the explanation. Yes, I think the nicest way to improve the test is to extend tst/test-compile.