Upgrade meataxe and fix some build issues

[jdemeyer]

1. The actual command lines when compiling are hidden, all I see is compile foo.c. Seeing the actual command line passed to gcc is very useful for debugging.

2. user-defined CFLAGS are not passed.

3. meataxe assumes that char is signed char but that is not guaranteed by the C standard. This causes breakage on powerpc64le. A work-around with GCC is -fsigned-char but it's better to fix the C sources.

4. the test-suite has race conditions: testing with MAKE="make -j48" fails while MAKE="make -j1" works.

[SimonKing]

Hi Jeroen,

Thank you for locating a couple of issues and opening the ticket! I am about to contact upstream

Replying to jdemeyer:

3. meataxe assumes that char is signed char but that is not guaranteed by the C standard. This causes breakage on powerpc64le. A work-around with GCC is -fsigned-char but it's better to fix the sources.

OK, but that would also require to either fix the sources (namely the Makefile), either by using -fsigned-char or by passing CFLAGS.

4. the test-suite has race conditions: testing with MAKE="make -j48" fails while MAKE="make -j1" works.

I have seen before that the test suite is flaky. Meataxe's test suite is about Meataxe executables that read and write data from files. Could file i/o be the reason for race conditions? Hm. Let's see what upstream has to say.

[jdemeyer]

Replying to SimonKing:

3. meataxe assumes that char is signed char but that is not guaranteed by the C standard. This causes breakage on powerpc64le. A work-around with GCC is -fsigned-char but it's better to fix the sources.

OK, but that would also require to either fix the sources (namely the Makefile)

I meant changing the C sources. This could mean changing char to signed char or fix the places where it is assumed that char is signed.

Also keep in mind that perhaps code linking to meataxe might break because of this (maybe not, I don't know). I don't think you can require such code to use -fsigned-char.

4. the test-suite has race conditions: testing with MAKE="make -j48" fails while MAKE="make -j1" works.

I have seen before that the test suite is flaky.

An easy workaround is to use $MAKE -j1 then.

[SimonKing]

Replying to jdemeyer:

I meant changing the C sources. This could mean changing char to signed char or fix the places where it is assumed that char is signed.

Also keep in mind that perhaps code linking to meataxe might break because of this (maybe not, I don't know). I don't think you can require such code to use -fsigned-char.

Good point.

4. the test-suite has race conditions: testing with MAKE="make -j48" fails while MAKE="make -j1" works.

I have seen before that the test suite is flaky.

An easy workaround is to use $MAKE -j1 then.

Is there really no *proper* solution for such a race condition, assuming that it really is caused by delays in file i/o?

[jdemeyer]

Replying to SimonKing:

Is there really no *proper* solution for such a race condition, assuming that it really is caused by delays in file i/o?

Of course there is, I was just giving the easiest possible solution. Everything depends on the exact nature of the race condition.

Just guessing... maybe different tests write to the same file. Then the fix is easy: use a different file for each test.

[jdemeyer]

This really looks like meataxe is using the same filename (x2) for different tests, especially because the calculated checksum is the same each time:

File x2: checksum error
         expected:   41484.1750919850
         calculated: 41484.2717504566
Makefile:134: recipe for target 'tmp/t-0115.done' failed
make[2]: *** [tmp/t-0115.done] Error 1
File x2: checksum error
         expected:   1251.4048248722
         calculated: 41484.2717504566
Makefile:134: recipe for target 'tmp/t-0116.done' failed
make[2]: *** [tmp/t-0116.done] Error 1
File x2: checksum error
         expected:   384.1740937932
         calculated: 41484.2717504566
Makefile:134: recipe for target 'tmp/t-0111.done' failed
make[2]: *** [tmp/t-0111.done] Error 1

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing: Just guessing... maybe different tests write to the same file. Then the fix is easy: use a different file for each test.

Could several processes READING from the same file be a problem, too? Or is reading safe?

[SimonKing]

Replying to jdemeyer:

This really looks like meataxe is using the same filename (x2) for different tests, especially because the calculated checksum is the same each time:

OK, that would of course be a bad design.

[jdemeyer]

Replying to SimonKing:

Could several processes READING from the same file be a problem, too?

No, that should be safe. This is in fact extremely common: if you execute several instances of gcc in parallel, each of them will probably read the same .h files.

[SimonKing]

Meanwhile Michael Ringe has replied:

• He deems my patches useful extensions and/or corrections.
• He has difficulties to use my code in the current development version, since the code base has changed.
• The current source code is ​at github. He plans to include my patches there and create a new 2.4.x-version, but it is unclear when he will find the time to do so.
• In the current development version, some but not all test problems are solved.

Concerning the issues raised by Jeroen:

• The problem with (unsigned) char should be solved in the development version; however, he didn't try building on AIX recently.
• Full output of the compiler commands can be obtained by make V=1. So, we should change spkg-install accordingly.
• The tests are not parallelisable (so, we should resort to make -j1), but should become in future.

[SimonKing]

Jeroen, I have developed my patches with a git repository on my laptop. Since the new meataxe upstream code is at github, shouldn't it be possible to do a merge more or less smoothly (using kdiff3 I mean)? How do I use github, i.e., how do I pull the recent upstream changes so that I can merge with my patches?

Or: SHOULD I do the merge? Should an optional meataxe package be based on the latest official meataxe release (which is currently the case) or on a non-official development branch?

[jdemeyer]

Replying to SimonKing:

Should an optional meataxe package be based on the latest official meataxe release (which is currently the case) or on a non-official development branch?

A stable release is preferred, but if there are good reasons to use a development version, you can use that (like we do for the standard package PARI for example). From what I understand, this would qualify as a good reason.

But above all: document things clearly. A blackbox tarball with no documentation where it came from is not acceptable. It should be possible for a third party to obtain exactly the sources that you used (from github say).

[jdemeyer]

Replying to SimonKing:

• Full output of the compiler commands can be obtained by make V=1. So, we should change spkg-install accordingly.
• The tests are not parallelisable (so, we should resort to make -j1), but should become in future.

Good. Remember to replace make by $MAKE in the above.

[SimonKing]

Replying to jdemeyer:

But above all: document things clearly.

How? Do I simply state in SPKG.txt the address of the github repository and the commit I am using?

And the tarball created from the repository should not contain .git/ and .gitignore, right?

[SimonKing]

Sigh. The upstream development version has changed considerably wrt the official release. Filenames have changed, which makes rebasing my patches uncomfortable.

ALL c-source files have changed. It seems that in most cases the change is in how comments are formatted and whether tabs are used. This makes rebasing my patches a mess, and for no good reason (I mean, it isn't so much about codes but about formatting!).

There are some source files that look like being dedicated to the "large" kernel (i.e., field sizes > 255), but in the Makefile is stated that the large kernel is not available --- which I find inconsistent.

But my main frustration comes from the fact that using the latest upstream development version would require an awful lot of additional work.

[jdemeyer]

Replying to SimonKing:

It seems that in most cases the change is in how comments are formatted and whether tabs are used. This makes rebasing my patches a mess, and for no good reason (I mean, it isn't so much about codes but about formatting!).

If it's only a matter of spacing, git merge has options for dealing with that:

           ignore-space-change, ignore-all-space, ignore-space-at-eol
               Treats lines with the indicated type of whitespace change as unchanged for the sake of a three-way merge. Whitespace changes mixed with
               other changes to a line are not ignored. See also git-diff(1)-b, -w, and --ignore-space-at-eol.

               ·   If their version only introduces whitespace changes to a line, our version is used;

               ·   If our version introduces whitespace changes but their version includes a substantial change, their version is used;

               ·   Otherwise, the merge proceeds in the usual way.

[jdemeyer]

Replying to SimonKing:

Replying to jdemeyer:

But above all: document things clearly.

How? Do I simply state in SPKG.txt the address of the github repository and the commit I am using?

If you literally took a snapshot from that commit: yes

And the tarball created from the repository should not contain .git/ and .gitignore, right?

No, it should not contain that. Ideally, upstream has a defined procedure for making a source tarball (something like make dist or make snapshot or python setup.py sdist), that should be used.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

It seems that in most cases the change is in how comments are formatted and whether tabs are used. This makes rebasing my patches a mess, and for no good reason (I mean, it isn't so much about codes but about formatting!).

Unfortunately, it is more complicated. Here is an example obtained with diff -b:

1,8c1,7
< /* ============================= C MeatAxe ==================================
<    File:        $Id: matcmp.c,v 1.1.1.1 2007/09/02 11:06:17 mringe Exp $
<    Comment:     Compare matrices.
<    --------------------------------------------------------------------------
<    (C) Copyright 1998 Michael Ringe, Lehrstuhl D fuer Mathematik,
<    RWTH Aachen, Germany  <mringe@math.rwth-aachen.de>
<    This program is free software; see the file COPYING for details.
<    ========================================================================== */
---
> ////////////////////////////////////////////////////////////////////////////////////////////////////
> // C MeatAxe - Compare matrices
> //
> // (C) Copyright 1998-2015 Michael Ringe, Lehrstuhl D fuer Mathematik, RWTH Aachen
> //
> // This program is free software; see the file COPYING for details.
> ////////////////////////////////////////////////////////////////////////////////////////////////////
10,11c9
< 
< #include "meataxe.h"
---
> #include <meataxe.h>
14c12,13
< MTX_DEFINE_FILE_INFO
---
> ////////////////////////////////////////////////////////////////////////////////////////////////////
> // Local data
15a15
> MTX_DEFINE_FILE_INFO
16a17,18
> /// @addtogroup mat
> /// @{
18,42c20,38
< /**
<  ** @addtogroup mat
<  ** @{
<  **/
< 
< /**
<  ** Compare two matrices
<  ** If the matrices are equal, the return value is 0. Otherwise the return value is positive,
<  ** if @em a is "greater" than @em b and negative, if @em a is "less" than @em b. The ordering
<  ** matrices is defined as follows:
<  **
<  ** - If the matrices are over different fields, the matrix over the smaller field is smaller.
<  ** - Otherwise, if the matrices have different number of columns, the matrix with the smaller
<  **   number of columns is smaller.
<  ** - Otherwise, if the matrices have different number of rows, the matrix with the smaller
<  **   number of rows is smaller.
<  ** - Otherwise, the relation is determined by the return value of FfCmpRow() on the first row
<  **   that is not equal in both matrices.
<  **
<  ** In case an error occurs, the return value is -1. But note that a return value of -1 does
<  ** not necessarily mean that an error has occured.
<  ** @param a First matrix.
<  ** @param b Second matrix.
<  ** @return 0 if the matrices are equal, nonzero otherwise (see description).
<  **/
---
> ////////////////////////////////////////////////////////////////////////////////////////////////////
> /// Compare two matrices
> /// If the matrices are equal, the return value is 0. Otherwise the return value is positive,
> /// if @em a is "greater" than @em b and negative, if @em a is "less" than @em b. The ordering
> /// matrices is defined as follows:
> ///
> /// - If the matrices are over different fields, the matrix over the smaller field is smaller.
> /// - Otherwise, if the matrices have different number of columns, the matrix with the smaller
> ///   number of columns is smaller.
> /// - Otherwise, if the matrices have different number of rows, the matrix with the smaller
> ///   number of rows is smaller.
> /// - Otherwise, the relation is determined by the return value of FfCmpRow() on the first row
> ///   that is not equal in both matrices.
> ///
> /// In case an error occurs, the return value is -1. But note that a return value of -1 does
> /// not necessarily mean that an error has occured.
> /// @param a First matrix.
> /// @param b Second matrix.
> /// @return 0 if the matrices are equal, nonzero otherwise (see description).
50,51c46
<     if (!MatIsValid(a) || !MatIsValid(b))
<     {
---
>    if (!MatIsValid(a) || !MatIsValid(b)) {
58c53
<     if ((i = a->Field - b->Field) != 0)
---
>    if ((i = a->Field - b->Field) != 0) {
60c55,56
<     if ((i = a->Noc - b->Noc) != 0)
---
>    }
>    if ((i = a->Noc - b->Noc) != 0) {
62c58,59
<     if ((i = a->Nor - b->Nor) != 0)
---
>    }
>    if ((i = a->Nor - b->Nor) != 0) {
63a61
>    }
70,71c68
<     for (i = 0; i < a->Nor; ++i)
<     {
---
>    for (i = 0; i < a->Nor; ++i) {
75c72
<       if (diff != 0)
---
>       if (diff != 0) {
77a75
>    }
84,86c82,83
< /**
<  ** @}
<  **/
---
> 
> /// @}

The file has 83 lines, but even though the code didn't change the tiniest bit, the diff file has 117 lines --- which basically makes it so that all hunks for a patch will fail.

[jdemeyer]

I understand your problem, I also see no easy solution...

[SimonKing]

I am not so much of a git expert. The upstream repository's first commit is close to the sources my patches are based upon. Would it perhaps be feasible to apply my patches to them, and then try to rebase the full upstream branch? Can that be easier than the attempt to apply my patches directly to the latest upstream commit?

[jdemeyer]

Replying to SimonKing:

I am not so much of a git expert. The upstream repository's first commit is close to the sources my patches are based upon. Would it perhaps be feasible to apply my patches to them, and then try to rebase the full upstream branch?

I would not literally "rebase" but instead git merge the latest upstream branch and then fix the conflicts.

Can that be easier than the attempt to apply my patches directly to the latest upstream commit?

Perhaps yes. You can try and see what it gives.

[tscrim]

If you merge into patches into an older version in the git history, then do git merge, then it is more likely to succeed automatically. You should also try to employ the patience or minimal merge strategies as well, e.g.,:

git merge -X ignore-space-change -X patience master

patience is known to at least produce better differences wrt things like braces.

[SimonKing]

I succeeded to create a new package version based on the current snapshot. At least it installs and does some computations --- I didn't do extensive tests yet.

Apparently I misunderstood one point: My understanding was that the current snapshot would write/read the multiplication tables to/from a directory that is either prescribed by an environment variable at built time or a global C parameter at run time. But I stand corrected: Michael Ringe told me that MeatAxe would still create the tables in the current directory, but would first try to read the tables from a prescribed directory.

Moreover, the "prescribed directory" defaults to MTX_ROOT/lib, where MTX_ROOT is the path into which MeatAxe is installed. In the case of a Sage package, we would have MTX_ROOT=SAGE_LOCAL, so that the binaries are installed in SAGE_LOCAL/bin and libmtx.a in SAGE_LOCAL/lib and meataxe.h in SAGE_LOCAL/include. But this can be overridden at run time, if I understood correctly.

I see the following options:

• Create all multiplication tables (it's only a few, for all finite fields up to order 255) during installation of the optional package (i.e., in a temporary directory) and then copy the tables into SAGE_LOCAL/lib, where presumably MeatAxe would find them by default.
• Rather than clobbering SAGE_LOCAL/lib with some dozens of files, choose a better location in the SAGE_LOCAL-folder, and copy the tables to that location. Override meataxe's default directory at run time.
• Do not create the multiplication tables at installation time, but at run time. During installation of the package, write permission is granted for SAGE_LOCAL, but not at run time. Hence, tables separately for each user ought to be created in DOT_SAGE. This is what the current version of the meataxe spkg is doing.

What option do you prefer? The last option requires to rebase another patch, but that's easy.

And another issue: MeatAxe has a make tar target to create a source tarball. Unfortunately, make tar does not copy the test suite into the tarball. Hence, I should probably create an spkg-src script.

Would spkg-src really do the exact steps that I did to get a new tarball? That would be as follows:

1. git clone the upstream repository.
2. git checkout a particular commit, which in later versions may be changed
3. make tar
4. Move the resulting tar file into a temporary directory, unpack it there, copy the test suite into it, and repack it.

[jdemeyer]

For the tables, I would prefer DOT_SAGE. Even if there are 255 tables, maybe that number will increase if larger fields are supported.

[jdemeyer]

Replying to SimonKing:

Unfortunately, make tar does not copy the test suite into the tarball.

Does upstream consider that a bug or a feature? I would try to convince upstream to either fix make tar or to add a new target (say, make dist) to create the full distribution including the testsuite.

[SimonKing]

Replying to jdemeyer:

For the tables, I would prefer DOT_SAGE. Even if there are 255 tables, maybe that number will increase if larger fields are supported.

That's unlikely, as support for larger fields (available by setting ZZZ=1 at compile time) in previous versions was dropped.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

Unfortunately, make tar does not copy the test suite into the tarball.

Does upstream consider that a bug or a feature?

Ringe somehow said that the tests are work in progress...

I would try to convince upstream to either fix make tar or to add a new target (say, make dist) to create the full distribution including the testsuite.

How could I do the latter? I could of course add a patch in build/pkg/meataxe/patches. But that patch would be applied to the already *existing* sources that include tests.

[SimonKing]

Replying to SimonKing:

I would try to convince upstream to either fix make tar or to add a new target (say, make dist) to create the full distribution including the testsuite.

How could I do the latter? I could of course add a patch in build/pkg/meataxe/patches. But that patch would be applied to the already *existing* sources that include tests.

Maybe by letting spkg-src act as follows:

• git clone the upstream repository
• git checkout the specific commit that we are working with
• patch ... applying a patch to the makefile
• make dist.

Does that sound right, even though it would mean that the source tar ball of the spkg is *not* identical with upstream?

[jdemeyer]

Replying to SimonKing:

Does that sound right, even though it would mean that the source tar ball of the spkg is *not* identical with upstream?

Sounds right, but of course it would be better if upstream would add this patch (that's what I meant in 25 too).

[SimonKing]

I think I will use the following patch to create a tarball containing the tests, with a version number that isn't just 2.5.0-SNAPSHOT but is based on the hash of the commit being used:

Makefile

@@ -43 +43 @@
 SILENT1=
 SILENT=${SILENT${V}}
 
-MTXVERSION = 2.5.0-SNAPSHOT
+MTXVERSION = 2.5.0-$(shell git log -1 --format="%h")
 
 CFLAGS=$(CFLAGS1) -I"${MTXROOT}/include" -Itmp \
  -DZZZ=${ZZZ} -DMTXLIB="${MTXLIB}" -DMTXBIN="${MTXBIN}"
@@ -221 +221 @@
         @touch "$@"
 
 
-.PHONY: tar clean install test
+.PHONY: tar dist clean install test
 .PRECIOUS: tmp/%.o
 
 
@@ -264 +264 @@
   Makefile README.md COPYING\
   src/meataxe.doc src/changelog.doc\
 
+TESTS = tests
+
 tar: all doc
         rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
         && ln -s . meataxe-${MTXVERSION} \
@@ -271 +273 @@
         && rm meataxe-${MTXVERSION} \
         && gzip meataxe-${MTXVERSION}.tar \
         && echo "Created meataxe-${MTXVERSION}.tar.gz"
+
+dist: all doc
+        rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
+        && ln -s . meataxe-${MTXVERSION} \
+        && tar cf meataxe-${MTXVERSION}.tar $(EXPORTED_FILES:%=meataxe-${MTXVERSION}/%) meataxe-${MTXVERSION}/${TESTS} \
+        && rm meataxe-${MTXVERSION} \
+        && gzip meataxe-${MTXVERSION}.tar \
+        && echo "Created meataxe-${MTXVERSION}.tar.gz including tests"

[SimonKing]

Gosh, I just realise that it won't work. It assumes that there is a git repository. Hence, that makefile did work for me, since I tested it in a clone of the upstream repository. But it won't work in the spkg, since it doesn't comprise the repository. Back to the drawing board.

[SimonKing]

The following should be a better idea for spkg-src:

• git clone upstream.
• git checkout the appropriate commit --- which is what one needs to change for upgrading the sources.
• Apply the following general patch, that I will suggest to upstream:

  Makefile

  @@ -221 +221 @@
           @touch "$@"
   
   
  -.PHONY: tar clean install test
  +.PHONY: tar dist clean install test
   .PRECIOUS: tmp/%.o
   
   
  @@ -264 +264 @@
     Makefile README.md COPYING\
     src/meataxe.doc src/changelog.doc\
   
  +TESTS = tests
  +
   tar: all doc
           rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
           && ln -s . meataxe-${MTXVERSION} \
  @@ -271 +273 @@
           && rm meataxe-${MTXVERSION} \
           && gzip meataxe-${MTXVERSION}.tar \
           && echo "Created meataxe-${MTXVERSION}.tar.gz"
  +
  +dist: all doc
  +        rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
  +        && ln -s . meataxe-${MTXVERSION} \
  +        && tar cf meataxe-${MTXVERSION}.tar $(EXPORTED_FILES:%=meataxe-${MTXVERSION}/%) meataxe-${MTXVERSION}/${TESTS} \
  +        && rm meataxe-${MTXVERSION} \
  +        && gzip meataxe-${MTXVERSION}.tar \
  +        && echo "Created meataxe-${MTXVERSION}.tar.gz including tests"

• Use sed to replace the word -SNAPSHOT in the Makefile by the current commit hash. That will work, because the spkg-src script is in a git repository at that point.
• When building the spkg, there will be no git repository. But now, the commit number is hardcoded in the Makefile. The hardcoded number, however, will change automatically when changing the choice of commit in spkg-src.

Question:

Is it acceptable to add a general spkg-src, that takes the chosen commit of the upstream repository as an argument? In that way, one could obtain meataxe-2.5.0.123456.tar.gz for commit 123456 simply by calling spkg-src 123456.

[jdemeyer]

Replying to SimonKing:

Is it acceptable to add a general spkg-src, that takes the chosen commit of the upstream repository as an argument? In that way, one could obtain meataxe-2.5.0.123456.tar.gz for commit 123456 simply by calling spkg-src 123456.

I would advise against that, since spkg-src is not only a script but also serves as some kind of documentation of how the tarball was obtained. With that in mind, it's best if spkg-src is completely self-contained.

[SimonKing]

Replying to jdemeyer:

I would advise against that, since spkg-src is not only a script but also serves as some kind of documentation of how the tarball was obtained. With that in mind, it's best if spkg-src is completely self-contained.

Then I suggest spkg-src be the following:

#!/usr/bin/env bash

COMMIT=7c0bcf9

git clone https://github.com/momtx/meataxe.git
cd meataxe
git checkout -b package $COMMIT
git apply ../spkg-src
git commit -a -m "Add make target *dist*"
sed -i 's/-SNAPSHOT/.'$COMMIT'/' Makefile
make dist
cd ..
mv meataxe/meataxe-2.5.0.$COMMIT.tar.gz .
rm -rf meataxe
exit 0

########################################

diff --git a/Makefile b/Makefile
index 935964b..49c2b86 100644
--- a/Makefile
+++ b/Makefile
@@ -221,7 +221,7 @@ tmp/test-%.done: tests/common.sh tests/%/run $(PROGRAMS:%=${MTXBIN}/%)
 	@touch "$@"
 
 
-.PHONY: tar clean install test
+.PHONY: tar dist clean install test
 .PRECIOUS: tmp/%.o
 
 
@@ -264,6 +264,8 @@ EXPORTED_FILES =\
   Makefile README.md COPYING\
   src/meataxe.doc src/changelog.doc\
 
+TESTS = tests
+
 tar: all doc
 	rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
 	&& ln -s . meataxe-${MTXVERSION} \
@@ -271,3 +273,11 @@ tar: all doc
 	&& rm meataxe-${MTXVERSION} \
 	&& gzip meataxe-${MTXVERSION}.tar \
 	&& echo "Created meataxe-${MTXVERSION}.tar.gz"
+
+dist: all doc
+	rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \
+	&& ln -s . meataxe-${MTXVERSION} \
+	&& tar cf meataxe-${MTXVERSION}.tar $(EXPORTED_FILES:%=meataxe-${MTXVERSION}/%) meataxe-${MTXVERSION}/${TESTS} \
+	&& rm meataxe-${MTXVERSION} \
+	&& gzip meataxe-${MTXVERSION}.tar \
+	&& echo "Created meataxe-${MTXVERSION}.tar.gz including tests"
-- 
2.5.0

It provides the concrete commit of the snapshot, adds a make dist target that includes the test suite into the tarball, makes it so that the version number provides the concrete commit, creates the tar ball, and removes the temporary meataxe repository.

The only assumption is that the script is executed in the same directory in which it is stored, since it serves as a diff file to be applied to the Makefile.

[jdemeyer]

See build/pkgs/ecl/spkg-src for a good example on how to deal with patches at packaging time. It also contains some boilerplate to protect against mistakes.

[SimonKing]

Replying to jdemeyer:

See build/pkgs/ecl/spkg-src for a good example on how to deal with patches at packaging time. It also contains some boilerplate to protect against mistakes.

So, basically you suggest that *all* my patches should be applied when creating the source ball, whereas I thought that the source ball should be as close to upstream as possible --- hence, only a small patch should be applied when creating the source ball, and everything else should be patched when the package will be installed.

Do I understand correctly that it is fine to apply all patches, including Strassen-Winograd multiplication, *before* installing the package?

[jdemeyer]

Replying to SimonKing:

Replying to jdemeyer:

See build/pkgs/ecl/spkg-src for a good example on how to deal with patches at packaging time. It also contains some boilerplate to protect against mistakes.

So, basically you suggest that *all* my patches should be applied when creating the source ball

No! I am suggesting to split up your patches in two directories: one to be applied at install-time as usual and one to apply at packaging-time. Please have a closer look at the build/pkgs/ecl/patches/ directory.

[SimonKing]

Replying to jdemeyer:

No! I am suggesting to split up your patches in two directories: one to be applied at install-time as usual and one to apply at packaging-time. Please have a closer look at the build/pkgs/ecl/patches/ directory.

Exactly. All patches in build/pkgs/ecl/patches/ (gmp.patch, implib.patch, write_error.patch) are applied by spkg-src, which contains the lines

# the patches are applied here, as it has to be done before
# running autoconf.
cd src
echo "applying patches in ecl-$ECLVERSION/src"
# For some of the patches, Cygwin also has upstream fixes that are
# closely related, keep track.  See Trac 11119, for example.
for patch in ../../patches/*.patch; do
    patch --verbose -p2 <"$patch"
    if [ $? -ne 0 ]; then
        echo >&2 "Error applying '$patch'"
        exit 1
    fi
done

That's why I was asking.

Unless there was a change since sage-7.1.

[jhpalmieri]

In the most recent version it says

for patch in ../patches/src/*.patch; do

patches/src contains gmp.patch and implib.patch.

[SimonKing]

Replying to jhpalmieri:

In the most recent version it says

for patch in ../patches/src/*.patch; do

patches/src contains gmp.patch and implib.patch.

In my previous suggestion, I did not provide a separate patch but did everything in spkg-src, since I thought spkg-src was supposed to be self-contained. But if ../patches/ is allowed to serve a double purpose and if spkg-src may be "incomplete": Be it!

[SimonKing]

I have an unexpected problem:

When I am in a normal shell, the line git clone https://github.com/momtx/meataxe.git works fine.

When I am in a Sage shell, the same line git clone https://github.com/momtx/meataxe.git results in

Klone nach 'meataxe' ...
fatal: Unable to find remote helper for 'https'

What the heck...?

[SimonKing]

Apparently Sage has built its own version of git. Why? I don't think I told it to.

And why Sage hasn't built git with http support? That's a bit awkward for Sage development.

[SimonKing]

I was told on sage-devel that it is fine to provide a spkg-src script for usage *outside* of a Sage shell. Apparently only spkg-install and spkg-check are required to be executed in a Sage shell.

A related question: Is it ok when spkg-install also executes MeatAxe's test suite if SAGE_CHECK is set? Or is spkg-check mandatory for tests?

[SimonKing]

Replying to jdemeyer:

For the tables, I would prefer DOT_SAGE. Even if there are 255 tables, maybe that number will increase if larger fields are supported.

Current status of my local branch is:

• I am using the latest upstream snapshot, which says "Fixes for AIX and 64-bit platforms, GCC 4.9 compiler warnings". So, hopefully the "unsigned versus signed char" issue is fixed.
• To change to a different commit in the upstream repository, it should be sufficient to change what is written in package-version.txt, and spkg-src will create the upstream tar ball (provided that it is executed in the directory in which package-version.txt is found).
• The error propagation introduced by my patches does not cover all potential cases. But it seems to me that it is sufficient for the intended applications.
• MeatAxe's test suite passes *after* applying my patches with Strassen multiplication and error propagation.
• The tests in sage.matrix pass after changing one line number that appears in an error message.
• The issue with multiplication tables polluting the current directory has yet to be fixed. According to your comment, I should rebase my old patch (which is easy), so that we can tell MeatAxe to put its tables into DOT_SAGE.

[jdemeyer]

Replying to SimonKing:

I was told on sage-devel that it is fine to provide a spkg-src script for usage *outside* of a Sage shell.

Apparently only spkg-install and spkg-check are required to be executed in a Sage shell.

Right, it's not required to run spkg-src in a Sage shell, it's just a sensible thing to do. For example, you would want to do `cd "$SAGE_ROOT/build/pkgs/meataxe" but that wouldn't work outside a Sage shell.

A related question: Is it ok when spkg-install also executes MeatAxe's test suite if SAGE_CHECK is set?

I think you already asked this: it's not a problem.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

Apparently only spkg-install and spkg-check are required to be executed in a Sage shell.

Right, it's not required to run spkg-src in a Sage shell, it's just a sensible thing to do. For example, you would want to do `cd "$SAGE_ROOT/build/pkgs/meataxe" but that wouldn't work outside a Sage shell.

Right. That's why originally I wanted it to run in a Sage shell. But alas, Sage thought it was a good idea to build its own git and thought it was an even better idea to not support http.

A related question: Is it ok when spkg-install also executes MeatAxe's test suite if SAGE_CHECK is set?

I think you already asked this: it's not a problem.

Right, sorry for asking twice.

[SimonKing]

Sigh. The patch that makes MeatAxe read and write its tables in a prescribed directory makes its test suite fail. Which has not been the case for the previous MeatAxe version. Namely, it can't find certain files created during the tests.

[SimonKing]

It should be fine now. As far as I see,

• I have addressed what we have discussed on the ticket,
• MeatAxe's test suite passes when installing it with the "-c" option,
• the test suite does not leave garbage behind in SAGE_LOCAL/lib nor in the current directory (it does temporarily create garbage in SAGE_LOCAL/lib, but spkg-install removes it),
• the MeatAxe wrapper in Sage sets a C variable MtxLibDir to $DOT_SAGE/meataxe/, and one of the patches makes MeatAxe use it for its multiplication tables. Thus, again there should be no garbage created,
• the tests in sage.matrix pass and indeed no garbage can be found afterwards --- the multiplication tables neatly appear in DOT_SAGE/meataxe.

Needs review, I'd say!

---

New commits:

​b6e1697

Fix various MeatAxe build issues

[jdemeyer]

Just looking at spkg-src for now...

1. I think it's better if it can be executed from $SAGE_ROOT, so I would add cd build/pkgs/meataxe in the beginning.

2. You should move the tarball to $SAGE_DISTFILES (I know this isn't a Sage shell, but you could do something like ./sage --sh -c 'mv .... "$SAGE_DISTFILES"'

3. Remove the # commented out code.

4. I don't see the point of git commit -a -m "Add make target *dist*" since the tarball shouldn't contain the git repo.

[jdemeyer]

Did you submit your patches upstream? If yes, how, and was there any reaction?

[SimonKing]

Replying to jdemeyer:

Did you submit your patches upstream? If yes, how, and was there any reaction?

My "old" patches have been submitted upstream, by mail to Prof. Ringe.

A part of my old patches has been applied upstream: The "tweaks" for echelon form computation. Main tool is a function that does "addmul" for a *part* of a matrix row (in the original upstream implementation, "addmul" has always added complete rows, which is a waste of time during echelon computation, as one knows that the row starts with zeroes).

However, upstream has changed my function: I gave an initial position of the row segment to be added, *and* its length. Upstream just gives the initial position of the row segment and assumed that *everything* after the initial position will be added. That's fine for echelon computation, but I need to be able to operate on row segments of a given length, simply since Strassen multiplication acts on matrix windows.

Hence, I kept upstream's modification of my "partial addmul" function for echelon computation, and I put my original "partial addmul" function into src/window.c (which is where I implemented Winograd-Strassen multiplication).

Apparently upstream sees no reason to prescribe a directory for multiplication at run time. They think it is enough to try to read it from "$MTX_ROOT/lib/", not even from a *subdirectory* of "$MTX_ROOT/lib" (where MTX_ROOT typically is /local/ or $SAGE_ROOT), and create the tables in the current directory if it cannot be found in "$MTX_ROOT/lib". Here I strongly disagree. /local/lib/ is a place for libraries, not for computational data. And the current directory should be kept clean.

Also I don't know if upstream really cares about error propagation, since it seems that the default way of dealing with errors is to print an error message and exit (I would qualify that as "crash").

In any case, I will send my rebased patches to Ringe soon.

[SimonKing]

Replying to jdemeyer:

Just looking at spkg-src for now...

1. I think it's better if it can be executed from $SAGE_ROOT, so I would add cd build/pkgs/meataxe in the beginning.

As long as it is not to be executed in a sage shell...

2. You should move the tarball to $SAGE_DISTFILES (I know this isn't a Sage shell, but you could do something like ./sage --sh -c 'mv .... "$SAGE_DISTFILES"'

Interesting usage of "sage -sh"! So, "$SAGE_DISTFILES" is "$SAGE_ROOT/upstream/"?

I tried ./sage --sh -c COMMIT=$COMMIT 'mv build/pkgs/meataxe/meataxe/meataxe-2.5.0.$COMMIT.tar.gz "$SAGE_DISTFILES"', but

1. it didn't work
2. it didn't exit with an error although it didn't work.

So, what am I doing wrong?

3. Remove the # commented out code.

Sorry, I guess it was "WIP"...

4. I don't see the point of git commit -a -m "Add make target *dist*" since the tarball shouldn't contain the git repo.

Right.

[jdemeyer]

Replying to SimonKing:

So, what am I doing wrong?

I don't think you can do sh -c VAR=something "command", that's just invalid. What should work is

export VAR=something
sh -c "command"

That's also more readable.

[SimonKing]

It seems that ./sage --sh -c 'mv build/pkgs/meataxe/meataxe/meataxe-2.5.0.$COMMIT.tar.gz "$SAGE_DISTFILES"' COMMIT=$COMMIT works. But if it is better readable, I'll export COMMIT.

[jdemeyer]

Just add export on this line

COMMIT=$(cat package-version.txt | cut -d'.' -f4)

[SimonKing]

I am a bit puzzled by Sage's behaviour. I forgot to do ./sage -fix-pkg-checksums after creating the new tarball. Rather than aborting and complaining about a wrong checksum, Sage *deleted* the tarball that I just put into SAGE_ROOT/upstream!

[jdemeyer]

Replying to SimonKing:

Rather than aborting and complaining about a wrong checksum

I think this is mainly meant to make automatic testing easier... but I agree it's not really user-friendly.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​96fc74f

Sanitise meataxe's spkg-src script

[SimonKing]

Better?

[tscrim]

I've been trying to help install Sage with meataxe in a multiuser environment at UMN, and we ran into problems with meataxe looking for files in each user's DOT_SAGE folder. So I feel we should change the behavior to install/lookup the necessary files in some subdirectory of SAGE_LOCAL.

[jhpalmieri]

It depends on whether meataxe will be writing to those files after installation, because at that point it won't necessarily have write access to SAGE_LOCAL. I don't know the answer to this, though. Can it look in both places?

[tscrim]

In order to use meataxe, you need to rebuild Sage because of the optional extension. So I would think you would need a certain amount of write access. Although if that is an issue, I think one way we can fix it all is during installation, there could be a flag which we set that determines where the data files are written to and read from. Otherwise I am not sure how to make this really usable in a multiuser environment.

[SimonKing]

Replying to tscrim:

In order to use meataxe, you need to rebuild Sage because of the optional extension. So I would think you would need a certain amount of write access.

During installation of the package, of course it is granted that the person who is installing it has write access to SAGE_LOCAL. However, during usage of Sage, it is granted that the user has read access to SAGE_LOCAL and write access to DOT_SAGE, but not necessarily write access to SAGE_LOCAL.

Although if that is an issue, I think one way we can fix it all is during installation, there could be a flag which we set that determines where the data files are written to and read from. Otherwise I am not sure how to make this really usable in a multiuser environment.

Of course it is usable in a multiuser environment, because it is not a problem if each user has her own copy of the multiplication tables.

I think I have explained above the three possible approaches that one could take:

• Current approach I take: Patch MeatAxe, so that it becomes possible to set a flag at run time telling MeatAxe to read tables from and write tables to a specified directory. Since it is during run time, it must be a directory where the user has write access, hence, DOT_SAGE but not SAGE_LOCAL.

• Alternative approach: By a relatively recent upstream change, MeatAxe would finally read from, but still not write into, the directory MTX_ROOT/lib, where MTX_ROOT is the directory into which MeatAxe is installed; if a table cannot be found there, upstream would write into the current directory, which may be forbidden for the Sage user, resulting in an error (or without my patches, it wouldn't give an error but just crash). Of course we would have MTX_ROOT=$SAGE_LOCAL. Consequences:
  1. It would become mandatory to write all multiplication tables during installation of the package, since otherwise write permission cannot be granted and since we don't want the current directory to be polluted.
  2. We would never be able to use the "big" MeatAxe kernel for large field sizes, simply since it wouldn't be feasible to compute and write ten thousands of multiplication tables during installation. However, the big kernel isn't officially supported by upstream nowadays.
  3. As I said, upstream would use MTX_ROOT/lib, i.e. SAGE_LOCAL/lib, for its tables --- but that's awful. To the very least, it should use a single sub-directory such as MTX_ROOT/lib/meataxe_tables for that purpose. Hence, we would need yet another patch to upstream.
• Mixed approach: Patch MeatAxe as in the current approach, use a single location to which all users have read but not necessarily write permission, and create the tables during package installation.

[tmonteil]

I reported the following issue on sage-devel ​https://groups.google.com/forum/?fromgroups#!topic/sage-devel/aMuSDOjCbR0 I am not sure whether this upgrade will fix it.

[SimonKing]

Apparently the branch doesn't apply.