Skip to content
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

spkg-configure.m4 for brial #29369

Closed
orlitzky opened this issue Mar 19, 2020 · 51 comments
Closed

spkg-configure.m4 for brial #29369

orlitzky opened this issue Mar 19, 2020 · 51 comments

Comments

@orlitzky
Copy link
Contributor

Nothing unusual here except that this is a C++ library with no pkg-config file, and that makes it a bit harder to search for. I've assumed that any version we find is acceptable, which probably is not quite true.

CC: @embray @kiwifb @mkoeppe @dimpase

Component: build: configure

Author: Michael Orlitzky

Branch: 40a59a4

Reviewer: Matthias Koeppe, Dima Pasechnik

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

@orlitzky orlitzky added this to the sage-9.1 milestone Mar 19, 2020
@orlitzky
Copy link
Contributor Author

Branch: u/mjo/ticket/29369

@orlitzky
Copy link
Contributor Author

Author: Michael Orlitzky

@orlitzky
Copy link
Contributor Author

comment:1

I left off the libpng dependency, because I think brial needs it if and only if mr4i does. I also left off the python dependency, but maybe I shouldn't have?


New commits:

8b45a14Trac #29369: clarify message from SAGE_SPKG_DEPCHECK.
b262c17Trac #29369: new spkg-configure.m4 for brial.

@orlitzky
Copy link
Contributor Author

Commit: b262c17

@kiwifb
Copy link
Member

kiwifb commented Mar 19, 2020

comment:2

This is a matter for another ticket but I think the python part should be merged into sage proper.
A bit of history.
PolyBoRi use to have its own python bindings. sage also had its own python bindings to PolyBoRi. The python part of PolyBoRI (now BRiAl) could work with either the PolyBoRI or sage bindings. In BRiAl we never bothered with porting the python bindings of PolyBoRi - they are not there anymore. I removed all bits that could have used the PolyBoRi bindings from what is now sage-brial in Gentoo. So at the moment

  • Brial is needed to build sage because sage builds binding to brial
  • sage-brial needs sage's binding at runtime to be usable
  • sage needs sage-brial at run time for some stuff

I think sage-brial should just be merged in sage.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 19, 2020

comment:3

build/pkgs/brial/distros/ needs filling

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 19, 2020

comment:4

At the moment we cannot use python packages in the system python -- such as presumably the one of brial.
See #29023

@orlitzky
Copy link
Contributor Author

comment:5

Replying to @kiwifb:

This is a matter for another ticket but I think the python part should be merged into sage proper.

Ah, I didn't realize that sage's brial is installing TWO things, both the C++ library and a python package. I've only detected the C++ library, so this will have to wait until something happens with python -- either adding support for system python modules, or just moving sage-brial into sage in this case.

(I'm still running a ptestlong on this branch but presumably it will fail.)

@dimpase
Copy link
Member

dimpase commented Mar 20, 2020

comment:6

a less invasive option that moving sage-brial into sage would be to split the package into two parts, python and non-python (they may use the same tarball). Then the work done here is perfectly useful for brial proper, and sage-brial becomes another story.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

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

0a2745bTrac #29369: factor out brial's python module into a sage_brial package.
4c3c18cTrac #29369: fix installed_packages() doctest with system brial.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

Changed commit from b262c17 to 4c3c18c

@orlitzky
Copy link
Contributor Author

comment:8

Replying to @dimpase:

a less invasive option that moving sage-brial into sage would be to split the package into two parts, python and non-python (they may use the same tarball). Then the work done here is perfectly useful for brial proper, and sage-brial becomes another story.

Yeah, for now this is the path of least resistance (although the whole thing can probably be copy/pasted into src/sage/libs/brial). I factored the python module out into a new standard sage_brial package and now everything looks OK.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 26, 2020

@orlitzky
Copy link
Contributor Author

comment:10

You can throw an upgrade on top of my commits if you want, I just didn't feel like adding more variables to this ticket (especially since my goal is to never again use the SPKG in the first place).

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 26, 2020

Changed branch from u/mjo/ticket/29369 to u/mkoeppe/ticket/29369

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

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

c4f45b4fix for debian

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

Changed commit from 4c3c18c to c4f45b4

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 26, 2020

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 26, 2020

Reviewer: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

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

94b22edbuild/pkgs/{brial,sage_brial}: Add upstream_url

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2020

Changed commit from c4f45b4 to 94b22ed

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 27, 2020

comment:24

Replying to @dimpase:

there is a non-standard location formula/installation availabe:

https://github.com/brewsci/homebrew-science/blob/master/Formula/m4ri.rb

https://github.com/brewsci/homebrew-science is unmaintained. Let's not use it.

@orlitzky
Copy link
Contributor Author

comment:25

Replying to @mkoeppe:

But it does not find brial on fedora-31-standard (https://github.com/mkoeppe/sage/runs/537774738). Please take a look

Is brial-devel installed there?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2020

Changed commit from 94b22ed to 4ad36bd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2020

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

4ad36bdbuild/pkgs/brial/distros/fedora.txt: Add brial-devel

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 27, 2020

comment:27

Another round of tests at https://github.com/mkoeppe/sage/actions/runs/64668026

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 27, 2020

comment:28

fedora-28-standard (https://github.com/mkoeppe/sage/runs/539636819):

  [sagelib-9.1.beta8]   build/cythonized/sage/rings/polynomial/pbori.cpp:60026:55: error: use of deleted function ‘polybori::BooleConstant& polybori::BooleConstant::operator=(polybori::BooleConstant&&)’
  [sagelib-9.1.beta8]      __pyx_v_self->_pbconst = BooleConstant(__pyx_v_value);
  [sagelib-9.1.beta8]                                                          ^
  [sagelib-9.1.beta8]   In file included from /usr/include/polybori/BoolePolynomial.h:42,
  [sagelib-9.1.beta8]                    from /sage/src/sage/libs/polybori/pb_wrap.h:1,
  [sagelib-9.1.beta8]                    from build/cythonized/sage/rings/polynomial/pbori.cpp:684:
  [sagelib-9.1.beta8]   /usr/include/polybori/BooleConstant.h:40:7: note: ‘polybori::BooleConstant& polybori::BooleConstant::operator=(polybori::BooleConstant&&)’ is implicitly deleted because the default definition would be ill-formed:
  [sagelib-9.1.beta8]    class BooleConstant:
  [sagelib-9.1.beta8]          ^~~~~~~~~~~~~
  [sagelib-9.1.beta8]   /usr/include/polybori/BooleConstant.h:40:7: error: non-static const member ‘const bool polybori::BooleConstant::m_value’, can’t use default assignment operator
  [sagelib-9.1.beta8]   error: command 'gcc' failed with exit status 1

@orlitzky
Copy link
Contributor Author

comment:29

Replying to @mkoeppe:

fedora-28-standard (https://github.com/mkoeppe/sage/runs/539636819):

  [sagelib-9.1.beta8]   build/cythonized/sage/rings/polynomial/pbori.cpp:60026:55: error: use of deleted function ‘polybori::BooleConstant& polybori::BooleConstant::operator=(polybori::BooleConstant&&)’
...
  [sagelib-9.1.beta8]   error: command 'gcc' failed with exit status 1

See, this is why I became a mathematician...

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 27, 2020

comment:30

Right, for the endless fun with other mathematicians' software...

@orlitzky
Copy link
Contributor Author

Changed branch from u/mkoeppe/ticket/29369 to u/mjo/ticket/29369

@orlitzky
Copy link
Contributor Author

comment:31

This one rejects brial-0.8.5.


New commits:

40a59a4Trac #29369: detect only newish versions of brial.

@orlitzky
Copy link
Contributor Author

Changed commit from 4ad36bd to 40a59a4

@kiwifb
Copy link
Member

kiwifb commented Mar 28, 2020

comment:32

And I am physicist, which would make me the practical one - except I do more theoretical physics, so some people would say I am a mathematician with the wrong clothes. So, would something upstream make your job easier? Like a .pc file :)

I am upstream's maintainer after all. Pull requests welcome.

@orlitzky
Copy link
Contributor Author

comment:33

Replying to @kiwifb:

And I am physicist, which would make me the practical one - except I do more theoretical physics, so some people would say I am a mathematician with the wrong clothes. So, would something upstream make your job easier? Like a .pc file :)

I am upstream's maintainer after all. Pull requests welcome.

A .pc file will be invaluable the next time around, but here we need to detect the versions that have already shipped. I probably should have asked for help writing the test program, but it didn't look that hard until it was too late =)

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2020

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2020

@vbraun
Copy link
Member

vbraun commented Apr 5, 2020

Changed branch from u/mjo/ticket/29369 to 40a59a4

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 9, 2020

comment:38

Follow-up: I see crashes in running doctests on fedora-30-standard (https://github.com/mkoeppe/sage/runs/572856797), which uses system brial 1.2.5-1

sage -t src/sage/rings/polynomial/multi_polynomial_sequence.py  # Killed due to abort
sage -t src/sage/rings/polynomial/pbori.pyx  # Killed due to abort
sage -t src/sage/rings/polynomial/polynomial_ring_constructor.py  # Killed due to abort
sage -t src/sage/sat/boolean_polynomials.py  # Killed due to abort
sage -t src/sage/sat/solvers/dimacs.py  # Killed due to abort

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 9, 2020

Changed commit from 40a59a4 to none

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 9, 2020

comment:39

This is now #29490

@dimpase
Copy link
Member

dimpase commented Apr 28, 2020

comment:41

In src/module_list.py there is

Extension('sage.rings.polynomial.pbori',
...
      depends = [SAGE_INC + "/polybori/" + hd + ".h" for hd in ["polybori", "config"]],
      extra_compile_args = m4ri_extra_compile_args),

which causes constant rebuilds with system package, as these headers are no longer in SAGE_INC. It's a minor annoyance, but still.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

comment:42

Replying to @kiwifb:

This is a matter for another ticket but I think the python part should be merged into sage proper.
[....] So at the moment

  • Brial is needed to build sage because sage builds binding to brial
  • sage-brial needs sage's binding at runtime to be usable
  • sage needs sage-brial at run time for some stuff

I think sage-brial should just be merged in sage.

This is now #30332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants