New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade M4RI to newest upstream release #7375
Comments
comment:1
The SPKG is here http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20091101.spkg Dave, can you take a look? |
comment:2
I replaced the same SPKG with a new version which fixes two |
comment:3
Again, I replaced the SPKG with an updated one which fixes compiler warnings and errors from Microsoft Visual Studio C++. I should have all platforms accounted for now (I might try the Sun compiler later though) |
comment:4
For some reason I never got a message about this. I will check over this. |
comment:5
A new SPKG which is supposed to fix #7171 is available at http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20091118.spkg |
comment:6
Hi, Anyway, back to the trac ticket...There are still some problems with this, which means the configure script believes the Sun compiler is broken. I've also made some other points. Some are nit-picking, but while you resolve one issue, you might as well resolve the others. The main problem is that spkg-install sets CFLAGS to include -fPIC and -Wall, both of which are GNU specific flags. So when called with the Sun compiler, the compiler fails to build an executable, as its given erroneous flags. That can be seen in the config.log attached. It's essential that flags like those are only added on compilers that accept them. I've attached a small script called testcc that I was intending at some point including in a more general place in Sage, but you can use it if you wish. It tests the C compiler and will report one of:
The script works by checking what the C pre-processor defines, which is more reliable than looking for words like GNU or Sun in the output. You could use that script to decide what flag to add to the compiler - whether it is -fPIC for GNU compilers, or -Kpic for the Sun compiler. I would not worry about the others at the minute. Here is what the script gives as output on both Sun and GNU compilers on a Sun workstation. It's also been tested on AIX, tru64 and HP-UX too.
Out of completeness, I'll attach another for the C++ compiler (testcxx), but I do not think you will want the one for C++. Using that script, you could then add in spkg-install something like the following, which I have not tested.
The Sun compilers are a lot more fussy than the GNU ones, so enabling extra warnings will probably show too many. Whilst Sage does not aim to support HP-UX, it would be worth adding that +z flag, as compiling with other compilers tends to show problems in code. The library might build on HP-UX with those changes.) Others issues I see are:
That's far from perfect, but will work with the Sun and GNU compilers, as both accept -m64. I do not believe you need to set CPPFLAGS to -m64, as that is not an option for the C pre-processor. But it would be worth double-checking that.
A better way to add a flag for c99 support (if it is needed), is to call the autoconf macro AC_PROG_CC_C99 http://www.gnu.org/software/autoconf/manual/autoconf.html#C-Compiler That macro was designed for exactly this purpose. As such, you can delete all the code that checks for the Sun compiler in configure.in and just use the built-in macro. It would also be wise to exit with an error if the compiler is not C99 compliant, if that is needed. Test if $ac_cv_prog_cc_c99 = no, and if so exit with an error that the code needs to be able to find a C99 compliant compiler. If you can make those changes, this has a reasonable chance of building on Solaris with the Sun compiler. I'll also try it on HP-UX for you. The machine is not running at the minute, but there is no point in even trying, as I know the GNU flags will break the build. |
This comment has been minimized.
This comment has been minimized.
Checks the type of C compiler, much more thorougly than using grep on output |
Attachment: testcc.gz Attachment: testcxx.gz Teses the C++ compiler. Not really needed here, but done for completelness. |
config.log showing the -Wall and -fPIC flags are causing problems. |
Reviewer: drkirkby |
comment:7
Attachment: config.log |
comment:8
Replying to @sagetrac-drkirkby:
I had a few beers in London yesterday too but only in the pub down the road :)
I am using it in spkg-install now
Changed.
Changed.
Changed.
It's now at the top.
Added.
Great, using that now.
Thanks a lot, this is really really helpful! |
comment:9
The updated SPKG is at http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20091119.spkg |
comment:10
Hi,
I hope the results of the tests are useful. Sorry if some of these existed in your earlier spkg-install, and I forgot to mention them, but I did admit I was under the influence of alcohol! == General points ==
to
However, there was a consensus some time back that the default should be to build with debug information present (-g flag) and only if SAGE_DEBUG was set to "no" should -g not be there. However, adding your --enable-debug flag to the configure script might have a serious impact on performance, in which case you should not enable that by default. You can best judge how to handle that.
So -pedantic and -g are CFLAGS. This has two problems. First, -pedantic is a GNU specific flag. Secondly, there is the issue of whether the -g flag should be there or not. The '-g' flag does appear to be a portable flag, so should not be a problem on any compiler I know of. Now to the results. == Results on HP-UX 11.11 with gcc 4.4.0 == M4RI configured properly, without aborting when trying to find the cache size, as reported in #7171. The package went on to compile ok on HP-UX, and passed all tests. That is certainly better than many packages. There was however a few things that perhaps can be addressed. One is HP-UX specific, so I would not bother trying to fix that, but the other two I believe will happen on other platforms too.
this happened on numerous files. Since the cache size will not be determined on Solaris, I might assume the same would happen there. Is there a way you can work around that, so the warning is not generated? It does not look very impressive to see warnings, but your package is still a lot better than many others in this respect.
So overall, on HP-UX, this is pretty good, though perhaps you can sort out at least the first of these. == Results on Solaris 10 update 7 (SPARC) with Sun Studio 12.1 compiler == Here things were not as successful as on HP-UX with gcc. 4MRI will not build with Sun Studio.
Whilst the -Wall and -fPIC flags are no longer being sent to the compiler, the -pedantic flag is getting to the compiler from spkg-install. I think that is what is preventing the Sun compiler from building an executable. I'm attaching another config.log generated on Solaris with the Sun compiler. I think the reason for the failure is pretty simple to sort out. I'm not sure if you need both -Wall and -pedantic (I don't know exactly what they are each supposed to do), but removing the -pedantic flag near the top of spkg-install, and adding in like this:
will probably do the trick. Then it will only get added on GNU compilers, so should not prevent it building on Solaris 10. Dave Dave |
comment:11
Replying to @sagetrac-drkirkby:
Very helpful indeed!
Done.
Okay, why not.
Moved to the GNU specific part.
If you like you can run
See above.
Doesn't ring a bell, so I won't address this for now.
I don't do anything with /dev/null, so I gues it must be in the toolchain.
This should be fixed now. http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20091120.spkg |
comment:12
Hi, I did not recheck on HP-UX as I have powered the machine off, and it is the garage, but I suspect it will still be fine there. But I notice at the top of spkg-install you have
which means you override the value of the environment variable, so it makes any tests pointless. That should be very easy to fix. == Suspect source code found with Sun's lint == As you can probably see, I can be a bit picky, but I am keen the quality of software in Sage is improved as much as possible. Some, such as lcalc leaves a lot to be desired in my opinion. The code in there is pretty awful, which means I'd be suspicious myself of trusting any results that make use of lcalc. Anyway, with that in mind, I decided to have a quick look at the M4RI source code. I'm not a mathematician, so I do not understand what is going on, but I did notice a few things, with the aid of 'lint' that I believe need addressing. I'm not an expert using lint, so just did:
in the 'src' directory. Here are a few examples of things I found. I've attached a log of the lint results, so perhaps you can have a look though them, to see if there are things that you should fix. In grayflex.c
does not use the argument 'c' at all. In permutation.c, the following function
does not use the integer 'value' at any point. Line 1180 of strassen.c
takes a argument 'cutoff' but you never actually use 'cutoff' in that function. sqrt() is used in brilliantrussian.c, but the header file math.h is not included. |
Results of running Sun's lint command in the 'src' directory. |
comment:13
Attachment: lintlog.txt I have made your lint suggestions a new ticket at: http://bitbucket.org/malb/m4ri/issue/18/cleanup-solaris-lint-warnings I will address them in due course. I have replaced the SPKG with a new SPKG which doesn't overwrite SAGE_DEBUG in the spkg-install script. Is that a positive review then? |
comment:15
What's holding off the positive review? |
comment:16
Looks good to me, based on the above and my own snooping around.
Positive review subject to checking in testcc.sh somewhere. |
comment:18
Hi,
I thought it wise to open a Sage ticket for the issues reported by lint to be resolved. (lint should exist on any platform, or if not the source can be found easily). As such, I have opened: #7503 M4RI source code needs looking at, as lint finds some issues. Thank you for resolving the issues. I wish others would take as much care over fixing issues as you did. Dave |
comment:19
BTW, if testcc.sh is checked in outside of the .spkg file, then testcxx (or if you want to call it testcxx.sh), which is attached to this ticket, should be placed in the same place. Both have wider use outside of M4RI. But the testcxx is not needed in M4RI. Dave |
comment:20
New SPKG with testcc.sh checked in: http://sage.math.washington.edu/home/malb/spkgs/libm4ri-20091120.p0.spkg I only checked it into the local M4RI SPKG repository. Once it is available globally in Sage, it can be removed again. |
comment:21
Fine. I'll try to get the testcc and testcxx reviewed at some point in the future and incorporated a bit more widely. When your updated M4RI package is added, the release manager should close these two tickets, as this upgrade fixes both these issues.
Dave |
Merged: sage-4.3.alpha1 |
Changed reviewer from drkirkby to David Kirkby |
Improvements in the most recent versions:
CC: @sagetrac-drkirkby
Component: packages: standard
Keywords: M4RI
Author: Martin Albrecht
Reviewer: David Kirkby
Merged: sage-4.3.alpha1
Issue created by migration from https://trac.sagemath.org/ticket/7375
The text was updated successfully, but these errors were encountered: