Opened 6 years ago
Last modified 5 years ago
#20136 needs_work defect
Upgrade meataxe and fix some build issues
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.2 |
Component: | packages: optional | Keywords: | |
Cc: | SimonKing | Merged in: | |
Authors: | Simon King | Reviewers: | Jeroen Demeyer |
Report Upstream: | None of the above - read trac for reasoning. | Work issues: | |
Branch: | u/SimonKing/various_meataxe_build_issues (Commits, GitHub, GitLab) | Commit: | 96fc74f2b7d5aa908d28376673e014828375e4c7 |
Dependencies: | Stopgaps: |
Description (last modified by )
- The actual command lines when compiling are hidden, all I see is
compile foo.c
. Seeing the actual command line passed togcc
is very useful for debugging.
- user-defined
CFLAGS
are not passed.
- meataxe assumes that
char
issigned 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.
- the test-suite has race conditions: testing with
MAKE="make -j48"
fails whileMAKE="make -j1"
works.
Change History (67)
comment:1 in reply to: ↑ description ; follow-up: ↓ 2 Changed 6 years ago by
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 3 Changed 6 years ago by
Replying to SimonKing:
- meataxe assumes that
char
issigned 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
.
- the test-suite has race conditions: testing with
MAKE="make -j48"
fails whileMAKE="make -j1"
works.I have seen before that the test suite is flaky.
An easy workaround is to use $MAKE -j1
then.
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 6 years ago by
- Report Upstream changed from N/A to Reported upstream. No feedback yet.
Replying to jdemeyer:
I meant changing the C sources. This could mean changing
char
tosigned char
or fix the places where it is assumed thatchar
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.
- the test-suite has race conditions: testing with
MAKE="make -j48"
fails whileMAKE="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?
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 7 Changed 6 years ago by
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.
comment:5 Changed 6 years ago by
- Description modified (diff)
comment:6 follow-up: ↓ 8 Changed 6 years ago by
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
comment:7 in reply to: ↑ 4 ; follow-up: ↓ 9 Changed 6 years ago by
comment:8 in reply to: ↑ 6 Changed 6 years ago by
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.
comment:9 in reply to: ↑ 7 Changed 6 years ago by
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.
comment:10 follow-up: ↓ 13 Changed 6 years ago by
- Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.
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.
comment:11 follow-up: ↓ 12 Changed 6 years ago by
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?
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 6 years ago by
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).
comment:13 in reply to: ↑ 10 Changed 6 years ago by
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.
comment:14 in reply to: ↑ 12 ; follow-up: ↓ 17 Changed 6 years ago by
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?
comment:15 follow-up: ↓ 16 Changed 6 years ago by
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.
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 18 Changed 6 years ago by
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.
comment:17 in reply to: ↑ 14 Changed 6 years ago by
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.
comment:18 in reply to: ↑ 16 Changed 6 years ago by
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.
comment:19 Changed 6 years ago by
I understand your problem, I also see no easy solution...
comment:20 follow-up: ↓ 21 Changed 6 years ago by
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?
comment:21 in reply to: ↑ 20 Changed 6 years ago by
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.
comment:22 Changed 6 years ago by
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.
comment:23 follow-up: ↓ 25 Changed 6 years ago by
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 theSAGE_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 inDOT_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:
git clone
the upstream repository.git checkout
a particular commit, which in later versions may be changedmake tar
- Move the resulting tar file into a temporary directory, unpack it there, copy the test suite into it, and repack it.
comment:24 follow-ups: ↓ 26 ↓ 44 Changed 6 years ago by
For the tables, I would prefer DOT_SAGE
. Even if there are 255 tables, maybe that number will increase if larger fields are supported.
comment:25 in reply to: ↑ 23 ; follow-up: ↓ 27 Changed 6 years ago by
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.
comment:26 in reply to: ↑ 24 Changed 6 years ago by
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.
comment:27 in reply to: ↑ 25 ; follow-up: ↓ 28 Changed 6 years ago by
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.
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 6 years ago by
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 repositorygit checkout
the specific commit that we are working withpatch ...
applying a patch to the makefilemake dist
.
Does that sound right, even though it would mean that the source tar ball of the spkg is *not* identical with upstream?
comment:29 in reply to: ↑ 28 Changed 6 years ago by
comment:30 Changed 6 years ago by
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
diff --git a/Makefile b/Makefile index 935964b..9f14045 100644
a b SILENT0=@ 43 43 SILENT1= 44 44 SILENT=${SILENT${V}} 45 45 46 MTXVERSION = 2.5.0- SNAPSHOT46 MTXVERSION = 2.5.0-$(shell git log -1 --format="%h") 47 47 48 48 CFLAGS=$(CFLAGS1) -I"${MTXROOT}/include" -Itmp \ 49 49 -DZZZ=${ZZZ} -DMTXLIB="${MTXLIB}" -DMTXBIN="${MTXBIN}" … … tmp/test-%.done: tests/common.sh tests/%/run $(PROGRAMS:%=${MTXBIN}/%) 221 221 @touch "$@" 222 222 223 223 224 .PHONY: tar clean install test224 .PHONY: tar dist clean install test 225 225 .PRECIOUS: tmp/%.o 226 226 227 227 … … EXPORTED_FILES =\ 264 264 Makefile README.md COPYING\ 265 265 src/meataxe.doc src/changelog.doc\ 266 266 267 TESTS = tests 268 267 269 tar: all doc 268 270 rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \ 269 271 && ln -s . meataxe-${MTXVERSION} \ … … tar: all doc 271 273 && rm meataxe-${MTXVERSION} \ 272 274 && gzip meataxe-${MTXVERSION}.tar \ 273 275 && echo "Created meataxe-${MTXVERSION}.tar.gz" 276 277 dist: all doc 278 rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \ 279 && ln -s . meataxe-${MTXVERSION} \ 280 && tar cf meataxe-${MTXVERSION}.tar $(EXPORTED_FILES:%=meataxe-${MTXVERSION}/%) meataxe-${MTXVERSION}/${TESTS} \ 281 && rm meataxe-${MTXVERSION} \ 282 && gzip meataxe-${MTXVERSION}.tar \ 283 && echo "Created meataxe-${MTXVERSION}.tar.gz including tests"
comment:31 Changed 6 years ago by
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.
comment:32 follow-up: ↓ 33 Changed 6 years ago by
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
diff --git a/Makefile b/Makefile index 935964b..49c2b86 100644
a b tmp/test-%.done: tests/common.sh tests/%/run $(PROGRAMS:%=${MTXBIN}/%) 221 221 @touch "$@" 222 222 223 223 224 .PHONY: tar clean install test224 .PHONY: tar dist clean install test 225 225 .PRECIOUS: tmp/%.o 226 226 227 227 … … EXPORTED_FILES =\ 264 264 Makefile README.md COPYING\ 265 265 src/meataxe.doc src/changelog.doc\ 266 266 267 TESTS = tests 268 267 269 tar: all doc 268 270 rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \ 269 271 && ln -s . meataxe-${MTXVERSION} \ … … tar: all doc 271 273 && rm meataxe-${MTXVERSION} \ 272 274 && gzip meataxe-${MTXVERSION}.tar \ 273 275 && echo "Created meataxe-${MTXVERSION}.tar.gz" 276 277 dist: all doc 278 rm -f meataxe-${MTXVERSION} meataxe-${MTXVERSION}.tar meataxe-${MTXVERSION}.tar.gz \ 279 && ln -s . meataxe-${MTXVERSION} \ 280 && tar cf meataxe-${MTXVERSION}.tar $(EXPORTED_FILES:%=meataxe-${MTXVERSION}/%) meataxe-${MTXVERSION}/${TESTS} \ 281 && rm meataxe-${MTXVERSION} \ 282 && gzip meataxe-${MTXVERSION}.tar \ 283 && 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
.
comment:33 in reply to: ↑ 32 ; follow-up: ↓ 34 Changed 6 years ago by
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.
comment:34 in reply to: ↑ 33 ; follow-up: ↓ 35 Changed 6 years ago by
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 ifspkg-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.
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 6 years ago by
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.
comment:36 in reply to: ↑ 35 ; follow-up: ↓ 37 Changed 6 years ago by
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?
comment:37 in reply to: ↑ 36 ; follow-up: ↓ 38 Changed 6 years ago by
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.
comment:38 in reply to: ↑ 37 Changed 6 years ago by
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.
comment:39 follow-up: ↓ 40 Changed 6 years ago by
In the most recent version it says
for patch in ../patches/src/*.patch; do
patches/src
contains gmp.patch
and implib.patch
.
comment:40 in reply to: ↑ 39 Changed 6 years ago by
Replying to jhpalmieri:
In the most recent version it says
for patch in ../patches/src/*.patch; do
patches/src
containsgmp.patch
andimplib.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!
comment:41 Changed 6 years ago by
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...?
comment:42 Changed 6 years ago by
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.
comment:43 follow-up: ↓ 45 Changed 6 years ago by
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?
comment:44 in reply to: ↑ 24 Changed 6 years ago by
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
, andspkg-src
will create the upstream tar ball (provided that it is executed in the directory in whichpackage-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
.
comment:45 in reply to: ↑ 43 ; follow-up: ↓ 46 Changed 6 years ago by
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
andspkg-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 ifSAGE_CHECK
is set?
I think you already asked this: it's not a problem.
comment:46 in reply to: ↑ 45 Changed 6 years ago by
Replying to jdemeyer:
Replying to SimonKing:
Apparently only
spkg-install
andspkg-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 ifSAGE_CHECK
is set?I think you already asked this: it's not a problem.
Right, sorry for asking twice.
comment:47 Changed 6 years ago by
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.
comment:48 Changed 6 years ago by
- Branch set to u/SimonKing/various_meataxe_build_issues
comment:49 Changed 6 years ago by
- Commit set to b6e16978528851ffb380449d72f3a529f543a4c9
- Status changed from new to needs_review
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
|
comment:50 Changed 6 years ago by
- Report Upstream changed from Reported upstream. Developers acknowledge bug. to None of the above - read trac for reasoning.
comment:51 follow-up: ↓ 54 Changed 6 years ago by
- Milestone changed from sage-7.1 to sage-7.2
- Reviewers set to Jeroen Demeyer
- Summary changed from Various meataxe build issues to Upgrade meataxe and fix some build issues
Just looking at spkg-src
for now...
- I think it's better if it can be executed from
$SAGE_ROOT
, so I would addcd build/pkgs/meataxe
in the beginning.
- 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"'
- Remove the
# commented out code
.
- I don't see the point of
git commit -a -m "Add make target *dist*"
since the tarball shouldn't contain the git repo.
comment:52 follow-up: ↓ 53 Changed 6 years ago by
Did you submit your patches upstream? If yes, how, and was there any reaction?
comment:53 in reply to: ↑ 52 Changed 6 years ago by
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.
comment:54 in reply to: ↑ 51 ; follow-up: ↓ 55 Changed 6 years ago by
Replying to jdemeyer:
Just looking at
spkg-src
for now...
- I think it's better if it can be executed from
$SAGE_ROOT
, so I would addcd build/pkgs/meataxe
in the beginning.
As long as it is not to be executed in a sage shell...
- 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
- it didn't work
- it didn't exit with an error although it didn't work.
So, what am I doing wrong?
- Remove the
# commented out code
.
Sorry, I guess it was "WIP"...
- 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.
comment:55 in reply to: ↑ 54 Changed 6 years ago by
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.
comment:56 Changed 6 years ago by
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.
comment:57 Changed 6 years ago by
Just add export
on this line
COMMIT=$(cat package-version.txt | cut -d'.' -f4)
comment:58 follow-up: ↓ 59 Changed 6 years ago by
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!
comment:59 in reply to: ↑ 58 Changed 6 years ago by
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.
comment:60 Changed 6 years ago by
- Commit changed from b6e16978528851ffb380449d72f3a529f543a4c9 to 96fc74f2b7d5aa908d28376673e014828375e4c7
Branch pushed to git repo; I updated commit sha1. New commits:
96fc74f | Sanitise meataxe's spkg-src script
|
comment:61 Changed 6 years ago by
Better?
comment:62 Changed 6 years ago by
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
.
comment:63 Changed 6 years ago by
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?
comment:64 follow-up: ↓ 65 Changed 6 years ago by
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.
comment:65 in reply to: ↑ 64 Changed 6 years ago by
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
, whereMTX_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 haveMTX_ROOT=$SAGE_LOCAL
. Consequences:- 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.
- 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.
- 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 asMTX_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.
comment:66 Changed 6 years ago by
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.
comment:67 Changed 5 years ago by
- Status changed from needs_review to needs_work
Apparently the branch doesn't apply.
Hi Jeroen,
Thank you for locating a couple of issues and opening the ticket! I am about to contact upstream
Replying to jdemeyer:
OK, but that would also require to either fix the sources (namely the Makefile), either by using
-fsigned-char
or by passingCFLAGS
.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.