Opened 14 months ago

Closed 13 months ago

Last modified 9 months ago

#29369 closed enhancement (fixed)

spkg-configure.m4 for brial

Reported by: mjo Owned by: mjo
Priority: major Milestone: sage-9.1
Component: build: configure Keywords:
Cc: embray, fbissey, mkoeppe, dimpase Merged in:
Authors: Michael Orlitzky Reviewers: Matthias Koeppe, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 40a59a4 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (42)

comment:1 Changed 14 months ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/29369
  • Cc embray fbissey mkoeppe dimpase added
  • Commit set to b262c17edb2ee783141662503a24bf641d8a8abf
  • Status changed from new to needs_review

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.

comment:2 follow-ups: Changed 14 months ago by fbissey

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.

comment:3 Changed 14 months ago by mkoeppe

build/pkgs/brial/distros/ needs filling

comment:4 Changed 14 months ago by mkoeppe

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

comment:5 in reply to: ↑ 2 Changed 14 months ago by mjo

  • Status changed from needs_review to needs_work

Replying to fbissey:

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.)

comment:6 follow-up: Changed 14 months ago by 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.

comment:7 Changed 14 months ago by git

  • Commit changed from b262c17edb2ee783141662503a24bf641d8a8abf to 4c3c18c666929aacd4417babb45a7c8a5df04967

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.

comment:8 in reply to: ↑ 6 Changed 14 months ago by mjo

  • Status changed from needs_work to needs_review

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.

comment:10 Changed 14 months ago by mjo

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).

comment:11 Changed 14 months ago by mkoeppe

  • Branch changed from u/mjo/ticket/29369 to u/mkoeppe/ticket/29369

comment:12 Changed 14 months ago by git

  • Commit changed from 4c3c18c666929aacd4417babb45a7c8a5df04967 to c4f45b4b3cfc04e722d3c12c71fb639ece81c4c2

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

c4f45b4fix for debian

comment:14 Changed 14 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe

comment:15 Changed 14 months ago by git

  • Commit changed from c4f45b4b3cfc04e722d3c12c71fb639ece81c4c2 to 94b22ed480852ca34a2a02edade8d00d160a9f00

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

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

comment:17 Changed 14 months ago by dimpase

OK, looks good. I'll wait for MacOS tests to finish.

comment:18 Changed 14 months ago by mkoeppe

better macOS tests scheduled here: https://github.com/mkoeppe/sage/actions/runs/64318155 (hopefully I fixed the broken parallelization of the build)

comment:19 Changed 14 months ago by fbissey

brial's build or sage's one?

comment:20 Changed 14 months ago by dimpase

I gather it's Sage's builds using brial from Homebrew

comment:21 follow-up: Changed 14 months ago by dimpase

system brial needs m4ri and boost, yet

2020-03-27T06:01:37.8676320Z Checking whether SageMath should install SPKG brial...
2020-03-27T06:01:37.8715050Z checking whether any of boost m4ri is installed as or will be installed as SPKG... yes; install brial as well
2020-03-27T06:01:37.8715730Z configure: no suitable system package found for SPKG brial

and then the build dies on pynac (not surprisingly)

The problem is in m4ri

2020-03-27T06:01:37.8106410Z Checking whether SageMath should install SPKG m4ri...
2020-03-27T06:01:37.8144280Z checking whether any of libpng is installed as or will be installed as SPKG... no
2020-03-27T06:01:37.8422490Z checking for m4ri >= 20140914... no
2020-03-27T06:01:37.8673980Z configure: no suitable system package found for SPKG m4ri

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

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

Last edited 14 months ago by dimpase (previous) (diff)

comment:22 Changed 14 months ago by dimpase

  • Reviewers changed from Matthias Koeppe to Matthias Koeppe, Dima Pasechnik
  • Status changed from needs_review to positive_review

let's merge this, and fix for MacOS/Cygwin, if/as needed.

comment:23 follow-up: Changed 14 months ago by mkoeppe

  • Status changed from positive_review to needs_work

Because it depends on m4ri and we only accept very new versions of that, this only uses brial on very few system configurations. ubuntu-eoan-python2 (https://github.com/mkoeppe/sage/runs/537774470), ubuntu-focal (https://github.com/mkoeppe/sage/runs/537774488) and archlinux-latest-standard (https://github.com/mkoeppe/sage/runs/537774841) work well. The test failures on debian-bullseye-standard (https://github.com/mkoeppe/sage/runs/537774574) are probably unrelated.

There's no brial package for homebrew, so there's nothing to test.

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

comment:24 in reply to: ↑ 21 Changed 14 months ago by mkoeppe

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.

comment:25 in reply to: ↑ 23 Changed 14 months ago by mjo

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?

comment:26 Changed 14 months ago by git

  • Commit changed from 94b22ed480852ca34a2a02edade8d00d160a9f00 to 4ad36bdea8a5a81df6324d16556984a4119d43ee

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

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

comment:28 follow-up: Changed 14 months ago by 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]      __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

comment:29 in reply to: ↑ 28 Changed 14 months ago by mjo

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...

comment:30 Changed 14 months ago by mkoeppe

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

comment:31 Changed 14 months ago by mjo

  • Branch changed from u/mkoeppe/ticket/29369 to u/mjo/ticket/29369
  • Commit changed from 4ad36bdea8a5a81df6324d16556984a4119d43ee to 40a59a410b7be086264f09cbefe9c03cb94e5c77
  • Status changed from needs_work to needs_review

This one rejects brial-0.8.5.


New commits:

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

comment:32 follow-up: Changed 14 months ago by fbissey

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.

comment:33 in reply to: ↑ 32 Changed 14 months ago by mjo

Replying to fbissey:

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 =)

comment:36 Changed 13 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:37 Changed 13 months ago by vbraun

  • Branch changed from u/mjo/ticket/29369 to 40a59a410b7be086264f09cbefe9c03cb94e5c77
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 13 months ago by mkoeppe

  • Commit 40a59a410b7be086264f09cbefe9c03cb94e5c77 deleted

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

comment:39 Changed 13 months ago by mkoeppe

This is now #29490

comment:40 Changed 13 months ago by dimpase

  • Owner changed from (none) to mjo

comment:41 Changed 13 months ago by dimpase

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.

comment:42 in reply to: ↑ 2 Changed 9 months ago by mkoeppe

Replying to fbissey:

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

Note: See TracTickets for help on using tickets.