Opened 5 years ago

Closed 4 years ago

#15015 closed defect (fixed)

Update to MPIR 2.7.0-alpha12

Reported by: jpflori Owned by:
Priority: major Milestone: sage-6.5
Component: packages: standard Keywords: mpir spkg cygwin
Cc: kcrisman, dimpase, tscrim, was Merged in:
Authors: Jean-Pierre Flori, Jeroen Demeyer Reviewers: Jean-Pierre Flori
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: d97c066 (Commits) Commit: d97c06604293f765fd0c8de6189d05cedcebea3c
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

MPIR 2.6.0 does not build on Cygwin 64. The next version should fix this problem (and others). Patches from Sage will mostly be integrated.

upstream at: http://mpir.org/mpir-2.7.0-alpha12.tar.bz2

Attachments (1)

mpirxx.2.patch (834 bytes) - added by jpflori 4 years ago.

Download all attachments as: .zip

Change History (63)

comment:1 Changed 5 years ago by jpflori

This should surely be renamed to "Update to MPIR 2.6.1" when the official version get released (no idea when...).

comment:2 Changed 5 years ago by jpflori

  • Authors set to Jean-Pierre Flori
  • Description modified (diff)

comment:3 Changed 5 years ago by jdemeyer

  • Dependencies set to MPIR 2.6.1 upstream
  • Description modified (diff)
  • Milestone changed from sage-5.12 to sage-pending
  • Summary changed from Let MPIR >=2.6.0 build on Cygwin to Update to MPIR 2.6.1 when it's released

comment:4 Changed 5 years ago by tscrim

For the record, the current spkg built for me on cygwin64.

comment:5 Changed 5 years ago by jpflori

On top of that the current MPIR 2.6.0 fails to properly build on Cygwin when CXX support is not enabled. Therefore it is not possible/easy to build GCC on Cygwin. That is because of a dirty hack in the autotools stuff already present in GMP before MPIR was forked. This is also fixed in the current git version.

comment:6 Changed 5 years ago by leif

  • Summary changed from Update to MPIR 2.6.1 when it's released to Update to MPIR 2.7.0 when it's released

Otherwise we'd probably have to close it as won't fix. :-)

Current upstream tarball: http://mpir.org/mpir-2.7.0-alpha4.tar.bz2

Still needs to be tested on Darwin PowerPC I think. (moufang? Does it still live?)

And probably also Macs with more recent versions of Xcode.

comment:7 follow-ups: Changed 5 years ago by kcrisman

So are you saying that JP should make a tarball for this as an alpha? I don't have time to test this on PPC until that exists. Currently trying to built 6.2.beta7, I'll wait until I get a clean build for that platform before I try new things. (Hopefully rpy is the only remaining problem there.)

comment:8 in reply to: ↑ 7 Changed 5 years ago by leif

Replying to kcrisman:

So are you saying that JP should make a tarball for this as an alpha?

I think that would be convenient for testing, and there are still some issues on Cygwin* JP knows about.

By the way, note that the latest changes in the alpha4 tarball aren't yet committed on github (it's still at alpha3).

comment:9 in reply to: ↑ 7 Changed 5 years ago by jpflori

Replying to kcrisman:

So are you saying that JP should make a tarball for this as an alpha? I don't have time to test this on PPC until that exists. Currently trying to built 6.2.beta7, I'll wait until I get a clean build for that platform before I try new things. (Hopefully rpy is the only remaining problem there.)

I happens I did and almost have an almost completely working out of the box cygwin64 branch. I just have to cherry-pick commits and push them to different branches on trac.

comment:10 Changed 5 years ago by jpflori

  • Branch set to u/jpflori/ticket/15015
  • Commit set to 5d7a2e99278f657b5d17f527d3d8f1c38e125a02
  • Dependencies MPIR 2.6.1 upstream deleted
  • Description modified (diff)

There are still issues with PPL... See https://groups.google.com/forum/?fromgroups=#!topic/mpir-devel/JFDaMAc0YpY


New commits:

a564118Update MPIR to 2.7.0.alpha4
5d7a2e9Remove unnecessary fix for MPIR.

comment:11 follow-up: Changed 5 years ago by leif

A few comments/questions:

  • Do we lose the Clang / XCode 5 patch? (I did have other clang patches elsewhere, also just preventing configure to fail for no real reason.)
  • Sun compiler patch (probably less important)?
  • What is patch ... || continue supposed to achieve? (The if [ $? -ne 0 ] afterwards gets useless.)
  • In if uname -a | grep x86_64; then ... any output (stdout and stderr) should get redirected to /dev/null.


Should #16149 be added as a dependency of this ticket, or do you want to deal with that in MPIR's spkg-install? (I'd prefer the former, but am also undecided about the solution to choose on #16149, hence "needs info".)

comment:12 follow-up: Changed 5 years ago by leif

I recall we also had issues with AVX-enabled CPUs on Darwin and (not that) old Apple assemblers not supporting AVX instructions. IIRC Jeroen removed a work-around (with a bunch of other things dealing with proper CFLAGS) from spkg-install a while ago, so this is presumably no longer an issue (just because we meanwhile require some more recent version of Xcode I guess), but I might be wrong.

comment:13 in reply to: ↑ 11 ; follow-ups: Changed 5 years ago by jpflori

Replying to leif:

A few comments/questions:

All (Most?) should be upstream now if I did not screw up a long time ago.

  • Do we lose the Clang / XCode 5 patch? (I did have other clang patches elsewhere, also just preventing configure to fail for no real reason.)

Must admit I don't remember about that one.

  • Sun compiler patch (probably less important)?

This one is in (IIRC).

  • What is patch ... || continue supposed to achieve? (The if [ $? -ne 0 ] afterwards gets useless.)

When there is no patch at all I guess. Not sure naymore.

  • In if uname -a | grep x86_64; then ... any output (stdout and stderr) should get redirected to /dev/null.

Maybe!


Should #16149 be added as a dependency of this ticket, or do you want to deal with that in MPIR's spkg-install? (I'd prefer the former, but am also undecided about the solution to choose on #16149, hence "needs info".)

The CXXFLAGS issue should be fixed in MPIR (maybe not alpha4, but in git master).

comment:14 in reply to: ↑ 12 Changed 5 years ago by jpflori

Replying to leif:

I recall we also had issues with AVX-enabled CPUs on Darwin and (not that) old Apple assemblers not supporting AVX instructions. IIRC Jeroen removed a work-around (with a bunch of other things dealing with proper CFLAGS) from spkg-install a while ago, so this is presumably no longer an issue (just because we meanwhile require some more recent version of Xcode I guess), but I might be wrong.

I don't now. We surely added thing for apple ocmpiler/assembler/linker but couldn't promise it was AVX related.

comment:15 in reply to: ↑ 13 Changed 5 years ago by leif

Replying to jpflori:

Replying to leif:

Should #16149 be added as a dependency of this ticket, or do you want to deal with that in MPIR's spkg-install? (I'd prefer the former, but am also undecided about the solution to choose on #16149, hence "needs info".)

The CXXFLAGS issue should be fixed in MPIR (maybe not alpha4, but in git master).

No, a slightly different one (but with the same effect). AC_PROG_CXX in MPIR is now correctly called after CXXFLAGS have been saved, and MPIR now behaves correctly (I'd say). But Sage sets (and exports) CXXFLAGS to the empty string, so MPIR doesn't touch them (again assuming they were intentionally set), such that we end up with missing flags (unless the user had really set them, or CFLAGS, to something meaningful).

comment:16 Changed 5 years ago by jpflori

I see. Thanks for the explanation.

comment:17 Changed 5 years ago by leif

P.S.: In spkg-install, we could do (almost) the same with CXXFLAGS as we do with CFLAGS. In the first "dry" configure run, we already unset them, so we could grep MPIR's from the Makefile and at least prepend those settings to our effective CXXFLAGS for the second, real configurerun.

(So far, we didn't touch CXXFLAGS at all, which I'd consider a small deficiency or inconsistency anyway.)

Last edited 5 years ago by leif (previous) (diff)

comment:18 Changed 5 years ago by leif

[Slight correction to my previous post.]

comment:19 in reply to: ↑ 13 Changed 5 years ago by leif

Replying to jpflori:

  • What is patch ... || continue supposed to achieve? (The if [ $? -ne 0 ] afterwards gets useless.)

When there is no patch at all I guess.

Ah, thought of something like that. But it should be

for foo in bar*; do
    [ -r "$foo" ] || continue   # ignore non-readable / non-existent files
    process "$foo"
    if [ $? -ne 0 ]; then
        # processing $foo failed, err out
        ...
    fi
done

comment:20 Changed 5 years ago by jpflori

Sure, I messed up :(

comment:21 Changed 5 years ago by git

  • Commit changed from 5d7a2e99278f657b5d17f527d3d8f1c38e125a02 to da483e1e109f71c93d22fa1ab115b12ede4fcaca

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

113a8ddMerge remote-tracking branch 'trac/develop' into ticket/15015
da483e1Correct MPIR spkg-install scripts for non-exisiting patches.

comment:22 Changed 4 years ago by jdemeyer

All patches from Sage seem to be integrated upstream, good! The changes here look fine, the main part of reviewing will be checking that it actually works. I will upgrade this to the latest MPIR alpha and report back.

comment:23 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 4 years ago by jpflori

I'm not sure the problem with ppl is fixed in the latest alpha... Some hints to fix it must lurk on flint-devel.

comment:25 Changed 4 years ago by jpflori

See https://groups.google.com/d/msg/mpir-devel/JFDaMAc0YpY/h774pq9wN2EJ And IIRC it does not only happens on Cygwin(64) but also on more usual systems.

comment:26 Changed 4 years ago by jdemeyer

  • Branch changed from u/jpflori/ticket/15015 to u/jdemeyer/ticket/15015
  • Created changed from 08/06/13 21:44:07 to 08/06/13 21:44:07
  • Modified changed from 08/28/14 10:21:45 to 08/28/14 10:21:45

comment:27 Changed 4 years ago by jdemeyer

  • Commit changed from da483e1e109f71c93d22fa1ab115b12ede4fcaca to 77251224787d5140af17a32e7cb3059effe22670

In spkg-install, I also changed

echo "Checking what CFLAGS MPIR would use if they were empty..."
(unset CFLAGS CPPFLAGS CXXFLAGS && ./configure $MPIR_CONFIGURE) &>configure-empty.log

to

echo "Configuring MPIR with empty CFLAGS to determine the defaults:"
(unset CFLAGS CPPFLAGS CXXFLAGS && ./configure $MPIR_CONFIGURE)

I think it's better to display the first configure run, in case it needs to be debugged or just to show the user that Sage isn't stuck.


New commits:

1607b8fMerge remote-tracking branch 'origin/develop' into ticket/15015
7725122Cosmetic changes

comment:28 Changed 4 years ago by jdemeyer

On Linux x86_64, g++ (Gentoo 4.6.4 p1.2, pie-0.5.2) 4.6.4, while compiling the Sage library:

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -I/usr/local/src/sage-config/local/include -I/usr/local/src/sage-config/local/include/csage -I/usr/local/src/sage-config/src -I/usr/local/src/sage-config/src/sage/ext -I/usr/local/src/sage-config/local/include/python2.7 -c build/cythonized/sage/libs/ppl.cpp -o build/temp.linux-x86_64-2.7/build/cythonized/sage/libs/ppl.o -fno-strict-aliasing -w
In file included from build/cythonized/sage/libs/ppl.cpp:352:0:
../local/include/gmpxx.h:1548:3: error: ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>::__gmp_expr(intmax_t)’ cannot be overloaded
../local/include/gmpxx.h:1539:3: error: with ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>::__gmp_expr(long int)’
../local/include/gmpxx.h:1552:3: error: ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>::__gmp_expr(uintmax_t)’ cannot be overloaded
../local/include/gmpxx.h:1540:3: error: with ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>::__gmp_expr(long unsigned int)’
../local/include/gmpxx.h:1615:16: error: ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>& __gmp_expr<__mpz_struct [1], __mpz_struct [1]>::operator=(intmax_t)’ cannot be overloaded
../local/include/gmpxx.h:1604:16: error: with ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>& __gmp_expr<__mpz_struct [1], __mpz_struct [1]>::operator=(long int)’
../local/include/gmpxx.h:1619:16: error: ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>& __gmp_expr<__mpz_struct [1], __mpz_struct [1]>::operator=(uintmax_t)’ cannot be overloaded
../local/include/gmpxx.h:1606:16: error: with ‘__gmp_expr<__mpz_struct [1], __mpz_struct [1]>& __gmp_expr<__mpz_struct [1], __mpz_struct [1]>::operator=(long unsigned int)’
error: command 'gcc' failed with exit status 1

comment:29 Changed 4 years ago by jpflori

Yup that is the problem I was mentioning.

comment:30 Changed 4 years ago by jpflori

Could you try with the attached patch? If it works, I'll forward it to Bill.

Changed 4 years ago by jpflori

comment:31 Changed 4 years ago by jpflori

Groumpf, thepatch is reversed. Take the .2 one.

comment:32 Changed 4 years ago by jpflori

Strange I don't have problems, even without my latest patch, on a machine where I seem to remember I used to have problems.

comment:33 Changed 4 years ago by jdemeyer

Works for me...

comment:34 Changed 4 years ago by jpflori

In fact I also need the patch, but not to build PPL anymore, but some ppl related file within the Sage library.

comment:35 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:36 Changed 4 years ago by git

  • Commit changed from 77251224787d5140af17a32e7cb3059effe22670 to aca71cb72ef80895a6228efde9965367152dba4f

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

6957f17Merge remote-tracking branch 'origin/develop' into ticket/15015
aca71cbUpgrade to mpir-2.7.0-alpha12

comment:37 Changed 4 years ago by git

  • Commit changed from aca71cb72ef80895a6228efde9965367152dba4f to 67babebcf82ac594043c634b8e63d4036c7818c6

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

67babebFix doctests due to changed xgcd results

comment:38 Changed 4 years ago by jdemeyer

  • Authors changed from Jean-Pierre Flori to Jean-Pierre Flori, Jeroen Demeyer

This now builds and passes all doctests on 64-bit for me.

Test please!

comment:39 Changed 4 years ago by jdemeyer

  • Cc was added

William: this changes one doctest in src/sage/tests/book_stein_modform.py

comment:40 Changed 4 years ago by git

  • Commit changed from 67babebcf82ac594043c634b8e63d4036c7818c6 to 8c7fbbd95d470a15e3e57b4e2573685bd726a7bd

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

8c7fbbdRe-enable "not tested" test from #4357

comment:41 Changed 4 years ago by jdemeyer

Unrelated to this ticket, I have re-enabled a doctest in book_stein_modform.py which was fixed in #4357.

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:42 Changed 4 years ago by jpflori

For info, I agree that we should output the first configure run on screen. Some users already complained that MPIR compilation was stuck because of the long time it can take on some systems and nothing was getting printed on screen.

comment:43 Changed 4 years ago by jpflori

Works fine for me (except for numerical noise I also get with the previous MPIR, and that is surely ppc64 specific).

Would you consider adding a spkg-src script?

comment:44 Changed 4 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Summary changed from Update to MPIR 2.7.0 when it's released to Update to MPIR 2.7.0.alpha12

This works perfectly fine for me (though I did not test Cygwin, but that's a low level priority and can be fixed later in case it is needed).

The current tarball is the upstream one and includes windows stuff we used to delete. Apart from that, I would positively review the ticket, even without a proper spkg-src script.

comment:45 Changed 4 years ago by jdemeyer

  • Summary changed from Update to MPIR 2.7.0.alpha12 to Update to MPIR 2.7.0 when it's released

Upstream has indicated that they consider the changed output of mpz_gcdext() a bug.

Therefore, I think it is premature to upgrade now.

comment:46 Changed 4 years ago by jpflori

Oh yeah, I read about that one...

comment:47 Changed 4 years ago by jdemeyer

  • Description modified (diff)

Because of the precedent at #17169, I would simply use the upstream tarball and not add a spkg-src script.

comment:48 Changed 4 years ago by jpflori

I don't really agree... If we keep on like that Sage will grow huge. Hard disks are cheap but internet connections aren't always, e.g. I get a poor bandwith between my office and boxen. Let's discuss this on sage-devel.

comment:50 Changed 4 years ago by jpflori

The gcdext change in MPIR was to be expected (at least the gcdext(1,1) example Jeroen found) and I personally don't think it's a bug. I'm not sure what Bill will think though.

At the very least it makes MPIR consistent with GMP.

comment:51 Changed 4 years ago by jpflori

(And ok for not shipping any spkg-src script.)

comment:52 Changed 4 years ago by jpflori

  • Status changed from new to needs_info

comment:53 Changed 4 years ago by jpflori

The only thing really annoying me right now is the name of the tarball. When we update to another alpha or the final version it will be a pain, so we should still repackage the upstream tarball to change the dir name wihtin it (rather than renaming the tarball).

comment:54 Changed 4 years ago by jpflori

  • Status changed from needs_info to needs_work
  • Work issues set to fix tarball name issue

comment:55 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-pending to sage-6.5
  • Status changed from needs_work to needs_review
  • Work issues fix tarball name issue deleted

comment:56 Changed 4 years ago by git

  • Commit changed from 8c7fbbd95d470a15e3e57b4e2573685bd726a7bd to aefe0ebcbbeeae156ea4923eee35dd112dfa6d70

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

aefe0ebUpgrade to MPIR 2.7.0-alpha12

comment:57 Changed 4 years ago by jdemeyer

New commits:

aefe0ebUpgrade to MPIR 2.7.0-alpha12

comment:58 Changed 4 years ago by jdemeyer

  • Summary changed from Update to MPIR 2.7.0 when it's released to Update to MPIR 2.7.0-alpha12

comment:59 Changed 4 years ago by git

  • Commit changed from aefe0ebcbbeeae156ea4923eee35dd112dfa6d70 to d97c06604293f765fd0c8de6189d05cedcebea3c

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

3d583afMerge remote-tracking branch 'origin/develop' into ticket/15015
d97c066Rename source directory

comment:60 Changed 4 years ago by jpflori

  • Status changed from needs_review to positive_review

Looks fine.

I had a vague souvenir we had trouble with dashes in version numbers but it does not seem to be the case anymore. Maybe we could also switch back to verbatim version numbers for Singular 4-0-1.

comment:61 Changed 4 years ago by jpflori

Got two files with failing doctests but cannot reproduce these failures:

  • src/sage/modular/modform/ambient.py
  • src/sage/graphs/genus.pyx

comment:62 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/15015 to d97c06604293f765fd0c8de6189d05cedcebea3c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.