#11616: Upgrade MPIR to a more recent upstream release
----------------------------------------------------------------------------------------------------------------+
Reporter: leif
| Owner: leif
Type: enhancement
| Status: needs_review
Priority: blocker
| Milestone: sage-5.0
Component: packages
| Resolution:
Keywords: sd32, GMP, SandyBridge, Westmere, yasm re2c race condition,
Linux ia64 Itanium GCC 4.7.0 bug | Work issues:
Report Upstream: N/A
| Reviewers: Jeroen Demeyer, Leif Leonhardy
Authors: Leif Leonhardy, Jeroen Demeyer
| Merged in:
Dependencies:
| Stopgaps:
----------------------------------------------------------------------------------------------------------------+
Comment (by leif):
Replying to [comment:51 jdemeyer]:
> Replying to [comment:48 leif]:
> > It would have been better if you made a p3, and not deleted my diff
with the changes of my p2.
> Since your changes weren't committed, I thought it was open to changes.
Anyway, this would probably "blow up" the Mercurial repository more than
my .asm patch ;-)
The raw patch alone is 32 KB. If one makes changes to other people's
code, one should always make a diff; that's what reviewer patches are for.
I also wanted to make further changes. Apart from that, it's confusing to
have different spkgs with the same patch level at different locations.
> > There's no need to remove the conditional unsetting of `PYTHON`, as I
already argued above.
> I really don't see the need for the `PYTHON` stuff. If somebody just
installs this spkg with "`./sage -i`", then `PYTHON` will not be set.
`PYTHON` will only be set if this spkg is installed through
`spkg/install`. I cannot think of a plausable scenario where this would
happen.
XD Just follow the instructions given in this ticket's description (and
elsewhere):
{{{
#!sh
$ cp mpir-x,y,z.pN.spkg "$SAGE_ROOT"/spkg/standard/
$ env SAGE_UPGRADING=yes make build
}}}
> > I wouldn't add a patch for the `.asm` files, but instead "patch" them
(with `sed`) on the fly. Otherwise this just blows up the Mercurial
repository
> The patch isn't that large, so it's not going to "blow up" the Mercurial
repository. Besides, I don't want to worry about the portability of the
find/sed commands that I'm using.
:-) Elsewhere we do...
> > Reading `CC` and `CFLAGS` ''from system-wide `gmp.h` files'' is also
for informational purposes.
> Who reads log files anyway?
Then why cat `config.log` in case of an error, and give all the other
messages? ;-)
> > Using the strings from that header (and not MPIR's `Makefile`, which
btw. could contain tabs as well) is the safer way, as these definitions
are documented and '''supposed''' to be (easily read out and) used by
other programs. (The [structure of the] `Makefile` is subject to change
without notice.)
> Agreed that it's subject to change without notice, but at least we'll
notice if it changes.
Not necessarily.
> Reading them from Makefile is absolutely the easiest way.
Is it? You use almost the same `sed` commands. And it's more error-
prone; just imagine we have
{{{
#!make
CFLAGS = -foo -bar # This is a comment.
}}}
Then `mpir_cflags` would certainly not be empty, but ... (Likewise for
`CC`.) Even the usage of one or more tabs can break your code.
> > You should also check whether reading the `CFLAGS` from the `Makefile`
apparently succeeded.
> This is harder to do. In theory, `CFLAGS` could be empty.
In that case we might (try to) add `-march=native`, using the functions
that have already been there... ;-)
> Moreover, it looks quite unlikely to me that reading `CC` would succeed
but reading `CFLAGS` would fail.
See above. A tab character is sufficient, a comment leads to weird errors
later.
> > I don't know why you removed the `-march=native` stuff;
> I removed it because it wasn't used! Reading the CC and CFLAGS from
MPIR should always succeed now (as far as I can tell), so the code using
`-march=native` would never be reached.
That was true for my p2 (provided `upstream_cflags` weren't empty), and
was subject to change, as I didn't notice that in the first place. And as
you mentioned, MPIR's `CFLAGS` may not contain processor-specific flags.
Different compilers might use / need different flags, and the code was
open for extensions regarding that.
> > I does in general make sense, especially since (at least the version
in Sage -- also regarding how long it takes to get a "new" version in)
will not always properly detect the CPU, i.e., will choose a more generic
target CPU [family] than `-march=native` does.
> Perhaps always using `-march=native` does make sense, but that's outside
the scope of this ticket.
Is it? But removing the code ''is'' in its scope? Doesn't make sense to
first remove it here and later again add slightly changed code on another
ticket.
> > It's not necessary to move the work-around for GCC 4.7.x on ia64, as
we don't support Itanium on anything but Linux.
> So what? Why ''a priori'' restrict it to Linux?
I didn't know you were porting MacOS to Itanium (a dead architecture
btw.)...
> > Moreover, using the odd `testcc.sh` script fails if `$CC` contains
more than one word
> Agreed, `$CC` should be quoted.
Perhaps rather change `testcc.sh`, although
{{{
#!sh
if $CC -E -x c /dev/null -dM 2>&1 | grep __GNUC__ >/dev/null; then
# It's GCC.
...
fi
}}}
does it, and is much simpler.
> > using shell pattern matching is not only more efficient
> Seriously? You're worrying about efficientcy of shell scripts?
It's also more reliable. (And in general, we have packages where
`configure` takes longer than the whole build.)
> > but also more readable.
> I find my code quite readable.
You mean like e.g.
{{{
#!sh
if [ -n "$AS" -a "$AS" != "as" ]; then ...
}}}
instead of
{{{
#!sh
if [[ $AS != as ]]; then ...
}}}
or
{{{
#!sh
arg=`echo "$arg" | sed \
-e 's/^-\(compatibility_version\)/-dylib_\1/' \
-e 's/^-\(current_version\)/-dylib_\1/' \
-e 's/^-\(install_name\)/-dylib_\1/' \
}}}
instead of
{{{
#!sh
case "$arg" in
-compatibility_version) arg=-dylib_compatibility_version;;
-current_version) arg=-dylib_current_version;;
-install_name) arg=-dylib_install_name
esac
}}}
?
> > I'm pretty sure we still need a work-around for newer CPUs (and GCCs)
on MacOS X, slightly different to what I previously added to the p2, at
least to avoid strange error messages.
> Let's see what users report... It doesn't make sense to me to add a
complicated workaround for a corner case which doesn't occur in practice.
Well, it's certainly a corner case (btw. like most of the regularly
occuring MacOS trouble ;-) ), but it did happen, so I added a test to
catch it, give an appropriate message, and work around it.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11616#comment:53>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
