Switch from PolyBoRi to BRiAl

[ohanar]

Right now PolyBoRi relies on scons (which is still Python 2 only), and has some Python 2 specific syntax in its bindings. These issues will need to be resolved before Sage can support Python 3.

On this ticket we update to the first version of BRiAl which includes a new (incomplete, but sufficient for our purposes) autotools based system.

To achieve this, we fork upstream polybori, see https://github.com/BRiAl/BRiAl

Tarball: https://github.com/BRiAl/BRiAl/releases/download/0.8.4.3/brial-0.8.4.3.tar.bz2

Depends on #19085

CC: @malb

Component: packages: standard

Author: R. Andrew Ohana

Branch/Commit: 7ff3b09

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/18437

[ohanar]

comment:1

Reposted from #15530:

fyi, I started working on an autotools based build system for polybori at https://github.com/ohanar/PolyBoRi/tree/autotools. I'll probably work on it a bit more next weekend during SD 64.25, but the important bits (the c++ libraries) are currently working with only a couple of hacks. What remains (for sage at least) is just a little cleanup and throwing in the bit of the python bindings we use.

---

I also don't know how we should handle this with regards to upstream, since upstream is dead. I certainly don't want to take over maintenance for polybori, but we'll probably need to make some sort of fork.

[kiwifb]

comment:2

I think there are two separate issues: 1) polybori is using a internal copy of a third party package that may not be dead upstream (I think it has updates once in a while): cudd. 2) the python bindings

We could package the libraries properly and then have a separate python binding package, unless polybori is altering its upstream significantly beyond the build system. The python binding could be moved in sage making it "a collective" responsibility.

[kiwifb]

comment:3

Current version of cudd in polybori is 2.5.0 but upstream at http://vlsi.colorado.edu/~fabio/ has a version 2.5.1 - it is somewhat alive.

[ohanar]

comment:4

Replying to @kiwifb:

I think there are two separate issues: 1) polybori is using a internal copy of a third party package that may not be dead upstream (I think it has updates once in a while): cudd. 2) the python bindings

We could package the libraries properly and then have a separate python binding package, unless polybori is altering its upstream significantly beyond the build system. The python binding could be moved in sage making it "a collective" responsibility.

Polybori has made some real modifications to cudd (I don't think that they are too significant though). From looking at the code it seems like they at least tried to support using an unmodified version, however I don't know if it actually works. Also, cudd's build system looks to be a joke (e.g. assumes sizeof(void *) == 4 unless you explicitly tell it otherwise), so I don't know how much I would trust it.

[kiwifb]

comment:5

Doesn't sound good. Autotooling something like that is definitely a good idea. But that puts the burden of maintenance on us. Mind you in Gentoo we have patches to autotool all the individual components of suitesparse independently of upstream, so it has been/is being done.

[ohanar]

comment:6

So I looked a bit more into how exactly polybori modifies cudd, and other than the build system changes, it appears to be just one of two things:

1. Prefixing the symbols to not collide with another installation of cudd
2. stripping out code that polybori doesn't use (there is only one case that polybori makes any meaningful changes to do this).

Given this, I don't think it would be hard to make polybori work with a vanilla version of cudd. Also, since upstream cudd is not completely dead, the author might be receptive to a better build system if we provide it.

[ohanar]

Author: R. Andrew Ohana

[ohanar]

comment:7

I contacted the author of Cudd on Tuesday and have yet to hear back from him. Given that, I think I would prefer not packaging Cudd separately, since maintaining one autotools based system (for polybori) is easier than two (for polybori and for cudd).

I've posted a branch, which uses the tarball at https://github.com/ohanar/PolyBoRi/releases/download/0.8.4/polybori-0.8.4.tar.gz. It doesn't include any of the python bindings, nor any of the tests, so beyond the build system, it is a bit of a fork of polybori. (Maybe we should rename it to something like cropped-polybori.)

---

New commits:

083fabf	cleanup settings in polybori in module_list.py
f5b2b78	move polybori python interface into sage
8ee9176	use new autotools based polybori

[ohanar]

Commit: 8ee9176

[ohanar]

Branch: u/ohanar/polybori

[kiwifb]

comment:8

I can easily test the new stuff in Gentoo. I can even test your stuff from git master if that helps.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

b6aad41

use non-broken version of polybori

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8ee9176 to b6aad41

[ohanar]

comment:10

Yes, I that would be good (at the very least to make sure that I haven't somehow made things work with old artifacts lying around). Point in fact, I just realized that I pushed a little prematurely, as I forgot to include a few headers in the dist. The new tarball is https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2.

I'm actually using the autotools branch, but at the moment the tip is the new release I just cut. Since this is new, I can squash the history at some point if desired.

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 Right now PolyBoRi relies on scons (which is still Python 2 only), and has some Python 2 specific syntax in its bindings. These issues will need to be resolved before Sage can support Python 3.
+
+**(Fake) upstream tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2)

[jdemeyer]

comment:12

Exactly what does this patch do? It looks like you're moving part of polybori into the Sage library, why that?

[jdemeyer]

comment:14

Extraordinary measures (forking polybori???) need extraordinary justification...

[kiwifb]

comment:15

Upstream being dead is ample justification. We had that conversation elsewhere and decided that it was the way to go.

[jdemeyer]

comment:16

Replying to @kiwifb:

We had that conversation elsewhere

Pointers please...

[kiwifb]

comment:17

We had some talk over at #15530 comment:32 but I think the confirmation that Alexander and Michael had completely abandoned ship were posted on a thread on sage-devel. I'll dig that up.

[kiwifb]

comment:18

From sage-devel "Python 3 focused Sage Days" thread: Hi, here's a reply from a PolyBoRi developer:

---

Subject: Re: [Polybori-discuss] Fwd: Re: [sage-devel] Re: Python 3 focused Sage Days Date: Saturday 10 Jan 2015, 22:19:16 From: Alexander Dreyer adreyer@t-online.de To: Martin Albrecht martinralbrecht@googlemail.com, Polybori Discuss polybori-discuss@lists.sourceforge.net

Hi Martin, Unfortunately, Andrew's right, PolyBoRi died when its developers left the scientific community. I'm not sure, what's the best way for SAGE to deal with it. Porting PolyBoRi's py files shouldn't be a big deal, and Sage already uses
it's own Cython-bindings for the data structures. What remains is scons. Perhaps, it's easier to set up a setup.py or another build system than removing PolyBoRi from all the crypto modules. (You probably know the dependencies better than me.) For working around lots of scons issues, most of the SConstruct file is plain python anyway.

Best regards, Alexander

[jdemeyer]

comment:19

Even if upstream polybori is dead and even if you fork it, that still doesn't mean it should be moved to the Sage library.

[kiwifb]

comment:20

Fair point. The question then is "does anyone care to have to install sage to get polybori instead of having it as stand alone package?". If it has users as a stand alone package then it should be kept split. If not, that's at the discretion of whoever actually maintains the code.

[jdemeyer]

comment:21

Replying to @kiwifb:

Fair point. The question then is "does anyone care to have to install sage to get polybori instead of having it as stand alone package?".

I don't think that's "the question".

All the packaging effort in Sage is really converging towards separating packages from the Sage library proper.

At the very least, moving polybori in the Sage library is a sufficiently big change that it should be discussed on sage-devel.

[jdemeyer]

comment:22

If you really decide to move polybori to the Sage library, then I think that

1. moving polybori to the Sage library and
2. making it Python 3 compatible

should be two different tickets.

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 Right now PolyBoRi relies on scons (which is still Python 2 only), and has some Python 2 specific syntax in its bindings. These issues will need to be resolved before Sage can support Python 3.

-**(Fake) upstream tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2)
+To achieve this, we fork upstream `polybori`, see [https://github.com/ohanar/PolyBoRi](https://github.com/ohanar/PolyBoRi) and we move the Python bindings to the Sage library.
+
+**Tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2)

[jdemeyer]

comment:24

And build/pkgs/polybori/SPKG.txt needs to be updated.

[ohanar]

comment:25

I haven't actually gone through the bindings and made sure they are python 3 compatible -- I just fixed their doctests to work in the sage environment. It is easy enough for me to break out the new package into a different ticket.

I think we should rename the fork to say cropped-polybori, to avoid confusion.

[ohanar]

Changed commit from b6aad41 to 33e900c

[ohanar]

Changed branch from u/ohanar/polybori to u/ohanar/polybori_autotools

[ohanar]

comment:26

I broke off the bindings portion to #18506.

---

New commits:

a05ce2d	move polybori python interface into sage
cdca588	cleanup settings in polybori in module_list.py
33e900c	use new autotools based polybori

[ohanar]

Dependencies: #18506

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

60725e3	Merge remote-tracking branch 'trac/develop' into polybori_autotools
da49082	update to a polybori which includes python bindings

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 33e900c to da49082

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

c30cf70

Revert "move polybori python interface into sage"

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from da49082 to c30cf70

[ohanar]

comment:29

I updated the polybori autotools to include the python bindings.

[ohanar]

Description changed:

--- 
+++ 
@@ -2,4 +2,4 @@

 To achieve this, we fork upstream `polybori`, see [https://github.com/ohanar/PolyBoRi](https://github.com/ohanar/PolyBoRi) and we move the Python bindings to the Sage library.

-**Tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.5/polybori-0.8.5.tar.bz2)
+**Tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2)

[ohanar]

Changed dependencies from #18506 to none

[jdemeyer]

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 Right now PolyBoRi relies on scons (which is still Python 2 only), and has some Python 2 specific syntax in its bindings. These issues will need to be resolved before Sage can support Python 3.

-To achieve this, we fork upstream `polybori`, see [https://github.com/ohanar/PolyBoRi](https://github.com/ohanar/PolyBoRi) and we move the Python bindings to the Sage library.
+To achieve this, we fork upstream `polybori`, see [https://github.com/ohanar/PolyBoRi](https://github.com/ohanar/PolyBoRi)

 **Tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2)

[jdemeyer]

comment:30

I think this needs more explanation/documentation. Surely build/pkgs/polybori/SPKG.txt will need to be updated. In particular explain how you obtained the tarball.

[jdemeyer]

comment:31

Is this commit https://github.com/ohanar/PolyBoRi/commit/a91362c5be91b2fb23f8acf609852381b78b15a4 part of the tarball on this commit? I would prefer to see it as a patch in build/pkgs/polybori/patches (and on a different ticket).

[ohanar]

comment:32

Replying to @jdemeyer:

Is this commit https://github.com/ohanar/PolyBoRi/commit/a91362c5be91b2fb23f8acf609852381b78b15a4 part of the tarball on this commit?

Yes.

I would prefer to see it as a patch in build/pkgs/polybori/patches (and on a different ticket).

I would not. As I see it there are three routes we could go with polybori:

1. We adopt the route that we took with Pynac (as a fork of Ginac). Our fork might in the future become tightly coupled with Sage, but we maintain it as a separate package outside of Sage.

2. We make a minimal fork, only include the minimal changes necessary to build polybori without scons. If more substantive changes are needed, we include those in the Sage library.

3. A hybrid approach: maintain a minimal fork, and a maintain a set of patches for more substantive changes.

I really dislike option 3 (which seems to be the one you are voting for). It seems like it just adds the unnecessary work of maintain patches, with no real benefit. Upon reflection, I would vote for 1, since it allows us make changes to the c++ source if we need to in the future.

If we go with option 1, I would rename this ticket to "Switch from polybori to XXXX", where XXXX is our fork of polybori (still needs a name, I haven't been 100% happy with anything I've come up with). In that case, I view it as perfectly acceptable for there to be additional changes to the fork, such as the Python 3 changes I made.

---

Also, @kiwifb. The author of cudd got back to me finally, he said he was already planning on making a new release sometime this summer which uses autotools. Until then, I think I'll continue bundling it with polybori.

[jdemeyer]

comment:33

There is also the issue of not doing too much on a single ticket. If you make a ticket which is only about compiling polybori with autotools, I'll be happy to review it. Anything more will only make the review process more difficult.

[jdemeyer]

comment:34

Replying to @ohanar:

1. We adopt the route that we took with Pynac (as a fork of Ginac). Our fork might in the future become tightly coupled with Sage, but we maintain it as a separate package outside of Sage.

One thing I don't like is that this will mean essentially unreviewed code in Sage.

From a practical point of view, there is not much difference between code in the Sage library and code in some upstream project which is only used in Sage.

But the code in the Sage library is supposed to be reviewed, while code in upstream projects is normally not reviewed (if people are using upstream outside of Sage, that means at least that the code is tested).

To be honest, I also don't have an optimal solution. Maybe that's why you should ask for feedback on sage-devel.

[kiwifb]

comment:35

Posted on sage-devel for feed back: https://groups.google.com/forum/#!topic/sage-devel/9Uhnvm3wef8

[ohanar]

Changed commit from c30cf70 to fdff733

[ohanar]

Changed branch from u/ohanar/polybori_autotools to u/ohanar/brial

[ohanar]

comment:37

I finally got back to this. This switches us to BRiAl (the successor/fork to polybori, as discussed in the sage-devel thread), and includes the new autotools based build system. I will repurpose #18506. for the second version of BRiAl (to be released), which will include some python 3 syntax fixes.

---

New commits:

72965f6	Merge branch 'polybori_autotools' into HEAD
fdff733	switch to BRiAl

[ohanar]

Description changed:

--- 
+++ 
@@ -1,5 +1,7 @@
 Right now PolyBoRi relies on scons (which is still Python 2 only), and has some Python 2 specific syntax in its bindings. These issues will need to be resolved before Sage can support Python 3.

-To achieve this, we fork upstream `polybori`, see [https://github.com/ohanar/PolyBoRi](https://github.com/ohanar/PolyBoRi)
+On this ticket we update to the first version of BRiAl which includes a new (incomplete, but sufficient for our purposes) autotools based system.

-**Tarball**: [https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2](https://github.com/ohanar/PolyBoRi/releases/download/0.8.6/polybori-0.8.6.tar.bz2)
+To achieve this, we fork upstream `polybori`, see [https://github.com/BRiAl/BRiAl](https://github.com/BRiAl/BRiAl)
+
+**Tarball**: [https://github.com/BRiAl/BRiAl/releases/download/0.8.4/brial-0.8.4.tar.bz2](https://github.com/BRiAl/BRiAl/releases/download/0.8.4/brial-0.8.4.tar.bz2)