Opened 8 years ago

Closed 7 years ago

#13767 closed enhancement (fixed)

upgrade boost to version 1.52.0

Reported by: tkluck Owned by: tbd
Priority: minor Milestone: sage-5.8
Component: packages: standard Keywords:
Cc: jpflori Merged in: sage-5.8.beta3
Authors: Timo Kluck Reviewers: Volker Braun, François Bissey, Alexander Dreyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by tkluck)

There was a discussion on sage-devel a while ago about including the Boost headers [1]. As noted there, the latest version of the optional spkg polymake doesn't compile with the current boost package. The current boost package is called boost-cropped because it contains only a subset of the boost headers, because of package size.

This new package is contains all headers, which is still only a subset of the upstream tarball. The total packaged size is ~ 5M, which feels reasonable to me.

I've checked that the current version polybori-0.8.2 and the current upstream version polymake-2.12-rc3 both compile under Sage with these headers. I'm also submitting an spkg for polymake in another ticket.

[1] https://groups.google.com/d/topic/sage-devel/fcxNrQqVSz0/discussion

Attachments (2)

Change History (39)

comment:1 Changed 8 years ago by tkluck

Because of the file size limit on trac attachment, I've put it here:

http://www.infty.nl/misc/boost-cropped-1.52.0.spkg

comment:2 Changed 8 years ago by tkluck

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by vbraun

The deleted patch is still in the repo:

[vbraun@laptop boost-cropped-1.52.0]$ hg st
! patches/boost-1.34.1-gcc-4.4.0-fixes.patch

Also, I take it the following in SPKG.txt is no longer true?

Merged against policy a patch that makes boost build with gcc 4.4.0. Will be obsoleted by an updated boost shortly.

comment:4 Changed 7 years ago by tkluck

Thanks for reviewing. I've removed that line from SKPG.txt, and committed my changes into the hg repo. I also renamed the package to boost_cropped (underscore instead of dash) to conform to the name-version scheme.

The current version is here:

http://www.infty.nl/misc/boost_cropped-1.52.0.spkg

comment:5 Changed 7 years ago by tkluck

I just uploaded a slightly newer version (md5sum d897d8e152623fab9f9c866ce184b501) that fixes a bash escaping error in spkg-install that I happened to notice.

comment:6 Changed 7 years ago by vbraun

+1 for underscore but don't we have to patch those as well?

[vbraun@laptop Sage]$ egrep boost-cropp sage/spkg/*
sage/spkg/install:BOOST_CROPPED=`newest_version boost-cropped`
sage/spkg/Makefile:BOOST_CROPPED=boost-cropped-1.34.1

comment:7 Changed 7 years ago by tkluck

I attached a patch doing just that. I don't know how to instruct the patchbot to apply this to sage_root, and it wouldn't work anyway since it needs the new spkg. Let me know if I can do anything more, though.

Note that the file spkg/Makefile is automatically generated, so I didn't patch it.

Last edited 7 years ago by tkluck (previous) (diff)

comment:8 Changed 7 years ago by vbraun

  • Description modified (diff)
  • Reviewers set to Volker Braun

comment:9 Changed 7 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks good to me

comment:10 follow-up: Changed 7 years ago by kcrisman

  • Cc jpflori added

Dumb question: Will the change in package name (though salutary) affect upgrading? (I.e., would we need a build patch for that?)

CC:ing JP in case this might help any Cygwin woes with not having certain function (along the lines of what cephes was supposed to accomplish, but wasn't accomplishing since it wasn't installed...), though looks like he's gotten most of them ironed out :)

comment:11 follow-up: Changed 7 years ago by jdemeyer

In SPKG.txt, replace

=== boost-cropped-1.52.0 (Timo Kluck, November 28, 2012) ===

by

=== boost_cropped-1.52.0 (Timo Kluck, November 28, 2012) ===

comment:12 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by jdemeyer

  • Description modified (diff)

Replying to kcrisman:

Dumb question: Will the change in package name (though salutary) affect upgrading?

Yes.

(I.e., would we need a build patch for that?)

Yes, but there is such a patch.

comment:13 in reply to: ↑ 11 Changed 7 years ago by tkluck

Replying to jdemeyer:

In SPKG.txt, replace

=== boost-cropped-1.52.0 (Timo Kluck, November 28, 2012) ===

by

=== boost_cropped-1.52.0 (Timo Kluck, November 28, 2012) ===

Done and set the date to today. I uploaded a new version (md5sum 2e12e082e9db4e908d0796246d0af351).

comment:14 in reply to: ↑ 12 Changed 7 years ago by kcrisman

(I.e., would we need a build patch for that?)

Yes, but there is such a patch.

Ok, I see that now. I will trust you that it will perform appropriately :)

comment:15 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Could this have caused

sage -t  --long -force_lib devel/sage/sage/rings/polynomial/pbori.pyx
**********************************************************************
File "/release/merger/sage-5.8.beta2/devel/sage-main/sage/rings/polynomial/pbori.pyx", line 7868:
    sage: random_set((a*b*c*d).lm(),2)
Expected:
    {{b,c}, {b}}
Got:
    {{b}, {c}}
**********************************************************************
[...more like this...]

comment:16 in reply to: ↑ 15 Changed 7 years ago by tkluck

Replying to jdemeyer:

Could this have caused

sage -t  --long -force_lib devel/sage/sage/rings/polynomial/pbori.pyx
**********************************************************************
File "/release/merger/sage-5.8.beta2/devel/sage-main/sage/rings/polynomial/pbori.pyx", line 7868:
    sage: random_set((a*b*c*d).lm(),2)
Expected:
    {{b,c}, {b}}
Got:
    {{b}, {c}}
**********************************************************************
[...more like this...]

Yes, I'm pretty sure it has: I can reproduce this issue on my stock 5.5 installation with upgraded boost and recompiled polybori.

The reason is possibly that polybori probably relies on Boost.Random for its randomness. Since we're doing a rather large update 1.34 - > 1.52, it's quite possible that the implementation has changed sufficiently to obtain different random values even with the same seed.

I would be inclined to just update the doctests, since these are inherently random results. But I would prefer to first hear the opinion of someone who actually uses polybori.

comment:17 Changed 7 years ago by tkluck

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:18 Changed 7 years ago by fbissey

I approve the patch for the polybori tests. I have seen exactly this output in sage-on-gentoo for the last few months. I have it documented at least as far back as the time of sage-5.4.

comment:19 Changed 7 years ago by AlexanderDreyer

These random ;-) changes in PolyBoRi's random functions won't cause any problems.

But maybe we should just ignore the doctest results and add additional tests for the following assumed properties result = random_set(monomial, n) should obey len(result) == n and result.diff(monomial.divisors()).empty() .

Last edited 7 years ago by AlexanderDreyer (previous) (diff)

comment:20 Changed 7 years ago by AlexanderDreyer

Edited: it should read len(result) == n.

comment:21 follow-up: Changed 7 years ago by AlexanderDreyer

Anyway, if we need a quick fix: I can also give a positive review. The more generic test would then be a new ticket.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by tkluck

Replying to AlexanderDreyer:

Anyway, if we need a quick fix: I can also give a positive review. The more generic test would then be a new ticket.

That would be great; the polymake upgrade also depends on this ticket, so it would be great to just get it in.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by AlexanderDreyer

  • Reviewers changed from Volker Braun to Volker Braun, fbissey, Alexander Dreyer
  • Status changed from needs_review to positive_review

Replying to tkluck:

That would be great; the polymake upgrade also depends on this ticket, so it would be great to just get it in.

Allright then. Please set up the other ticket when there is time and CC me there.

Just out of curiosity: how is the polymake ticket related to PolyBoRi?

comment:24 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Could you make SPKG.txt world-readable inside the spkg:

1435597707    4 -rw-------   1 jdemeyer jdemeyer     1406 Feb 26 22:22 ./SPKG.txt

comment:25 Changed 7 years ago by jdemeyer

Also: the two patches are not proper Mercurial changesets (they lack a header). You should generate patches using hg export tip (see http://sagemath.org/doc/developer/walk_through.html#creating-a-change or http://sagemath.org/doc/developer/walk_through.html#creating-your-own-patch-with-queues)

comment:26 in reply to: ↑ 23 ; follow-up: Changed 7 years ago by tkluck

Replying to AlexanderDreyer:

That would be great; the polymake upgrade also depends on this ticket, so it would be great to just get it in.

Just out of curiosity: how is the polymake ticket related to PolyBoRi?

This is not a polybori ticket, but a boost ticket :-)

comment:27 in reply to: ↑ 26 ; follow-up: Changed 7 years ago by AlexanderDreyer

Replying to tkluck:

This is not a polybori ticket, but a boost ticket :-)

Sorry, I misunderstood this. (Ticket vs. patch.)

comment:28 in reply to: ↑ 27 Changed 7 years ago by AlexanderDreyer

(You wrote it correctly, I just misread)

Last edited 7 years ago by AlexanderDreyer (previous) (diff)

comment:29 Changed 7 years ago by tkluck

  • Status changed from needs_work to needs_review

The patches are now hg changesets. Sorry about that, I've been doing development on an integrated git tree recently, and I still had to setup the conversion git -> hg.

comment:30 Changed 7 years ago by tkluck

I created #14205 for possibly making those doctests non-random, and put Alexander in the CC as requested.

comment:31 follow-up: Changed 7 years ago by kcrisman

  • Reviewers changed from Volker Braun, fbissey, Alexander Dreyer to Volker Braun, François Bissey, Alexander Dreyer

Could this be dependent on platform (the pbori tests, I mean)? I get all the original ones after installing this spkg and patches, doing ./sage -b and testing (hence failures).

----------------------------------------------------------------------
| Sage Version 5.8.beta1, Release Date: 2013-02-24                   |
| Type "notebook()" for the browser-based notebook interface.        |
| Type "help()" for help.                                            |
----------------------------------------------------------------------
**********************************************************************
*                                                                    *
* Warning: this is a prerelease version, and it may be unstable.     *
*                                                                    *
**********************************************************************
sage: from polybori import random_set, set_random_seed
sage: B.<a,b,c,d,e> = BooleanPolynomialRing()
sage: (a*b*c*d).lm()
a*b*c*d
sage: set_random_seed(1337)
sage: random_set((a*b*c*d).lm(),2)
{{b,c}, {b}}

Is there something else I should have done to make sure this change works? (I'm on an Intel Mac OSX 10.7.)

Last edited 7 years ago by kcrisman (previous) (diff)

comment:32 in reply to: ↑ 31 Changed 7 years ago by tkluck

Replying to kcrisman:

Could this be dependent on platform (the pbori tests, I mean)? I get all the original ones after installing this spkg and patches, doing ./sage -b and testing (hence failures). Is there something else I should have done to make sure this change works? (I'm on an Intel Mac OSX 10.7.)

Did you recompile the Polybori package?

sage -f polybori

comment:33 in reply to: ↑ 24 Changed 7 years ago by tkluck

Replying to jdemeyer:

Could you make SPKG.txt world-readable inside the spkg:

1435597707    4 -rw-------   1 jdemeyer jdemeyer     1406 Feb 26 22:22 ./SPKG.txt

This has been done, too. New package version with md5sum 2852b9e02ede0ab2785930d0f9268c6f.

comment:34 follow-up: Changed 7 years ago by kcrisman

Did you recompile the Polybori package?

No, because it didn't say to. Maybe there should be something that retriggers this on update? Anyway, works fine now, sorry for the noise (on the way to #13768...)

comment:35 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

All administrative issues are fixed.

comment:36 in reply to: ↑ 34 Changed 7 years ago by tkluck

Replying to kcrisman:

Did you recompile the Polybori package?

No, because it didn't say to. Maybe there should be something that retriggers this on update?

You're right. The makefile spkg/Makefile knows which packages depend on which others. Your comment reminded me to check whether it knows that polybori depends on boost_cropped: it does. I'm pretty sure that means that the upgrade path will work.

Thanks for testing!

comment:37 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.