Bug 476346 - Review Request: python-polybori - Framework for Boolean Rings
Summary: Review Request: python-polybori - Framework for Boolean Rings
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 476329 491537
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-12-13 10:45 UTC by Conrad Meyer
Modified: 2009-04-04 01:04 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-04 01:04:25 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)
Patch to build libcuddobj.so (2.93 KB, patch)
2009-03-29 17:06 UTC, Mamoru TASAKA
no flags Details | Diff

Description Conrad Meyer 2008-12-13 10:45:40 UTC
Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/python-polybori.spec
SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/python-polybori-0.5-1.fc9.src.rpm
Description:
PolyBoRi is a special purpose computer algebra system for computations in
Boolean Rings. The core is a C++ library, which provides high-level data
types for Boolean polynomials and related structures. As a unique
approach, binary decision diagrams are used as internal storage type for
polynomial structures. On top of this, we provide a Python interface for
parsing of complex polynomial systems, as well as for sophisticated and
extendable strategies for Groebner base computation.

Comment 1 Conrad Meyer 2008-12-13 10:48:57 UTC
Koji scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=996510

Comment 2 Conrad Meyer 2008-12-13 10:50:31 UTC
Obviously (from scratch build) this depends on cudd. Adding blocker bug.

Comment 3 Jason Tibbitts 2009-03-09 04:54:07 UTC
This failed to build for me; it looks like it's missing a build dependency on scons:

+ scons install PREFIX=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr EPREFIX=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr/bin INSTALLDIR=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr/share/python-polybori DOCDIR=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr/share/doc/python-polybori MANDIR=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr/share/man PYINSTALLPREFIX=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr/lib64/python2.6/site-packages DEVEL_PREFIX=/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.x86_64/usr 'CFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/cudd -I/usr/include/m4ri' 'CCFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/cudd -I/usr/include/m4ri' 'CXXFLAGS=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -I/usr/include/cudd -I/usr/include/m4ri'
/var/tmp/rpm-tmp.TvllXs: line 30: scons: command not found

I added scons as a build dep and the build fails later:

In file included from /usr/include/boost/dynamic_bitset.hpp:16,
                 from groebner/src/groebner_alg.h:16,
                 from PyPolyBoRi/Poly_wrapper.cc:14:
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp: In member function 'size_t boost::dynamic_bitset<Block, Allocator>::count() const':
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp:1021: error: 'mode' cannot appear in a constant-expression
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp:1021: error: template argument 1 is invalid
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp:1021: error: expected '>' before '*' token
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp:1021: error: expected '(' before '*' token
/usr/include/boost/dynamic_bitset/dynamic_bitset.hpp:1021: error: expected primary-expression before '>' token

No idea what's up there.

Comment 4 Conrad Meyer 2009-03-19 04:07:06 UTC
I'm having trouble getting PolyBoRi to link against cudd for whatever reason. This will take some work.

Comment 5 Conrad Meyer 2009-03-20 07:05:54 UTC
So in mock on F-10 it errors because it can't link against cudd (probably because we haven't built cudd for F-10 (or at least haven't pushed the update)). In F-11 on koji it errors because ... boost is broken?

Comment 6 Jason Tibbitts 2009-03-20 18:15:33 UTC
Please clear the whiteboard when someone gets this building.

Comment 7 Mamoru TASAKA 2009-03-23 20:13:32 UTC
Comments just for build issue:
( I have only checked if this package builds or not on dist-f11
  and have not tried to review other things on this package)

! boost issue is now fixed (workaround applied) (bug 491537)

For this package:
- BR: scons is needed
- Still does not build because this package does not pass
  check-buildroot:
-----------------------------------------------------------------
+ /usr/lib/rpm/check-buildroot
/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.1.x86_64/usr/share/python-polybori/ipbori/ipythonrc-polybori:execute pyroot = '/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.1.x86_64/usr/lib64/python2.6/site-packages'
Found '/builddir/build/BUILDROOT/python-polybori-0.5-1.fc11.1.x86_64' in installed files; aborting
error: Bad exit status from /var/tmp/rpm-tmp.yxVT94 (%install)
------------------------------------------------------------------
   At least
------------------------------------------------------------------
sed -i -e 's|%{buildroot}||' %{buildroot}%{_datadir}/%{name}/ipbori/ipythonrc-polybori
------------------------------------------------------------------
   is needed to fix this issue
- After fixing these two, this package builds on dist-f11:
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1255206

Again I just checked if this package builds or not.

Comment 8 Conrad Meyer 2009-03-23 20:27:06 UTC
Thanks! I will grab the .spec from the Koji task.

Comment 9 Conrad Meyer 2009-03-23 20:28:27 UTC
Except, I'm not sure how to do that -- can you make a patch?

Comment 10 Mamoru TASAKA 2009-03-24 03:06:01 UTC
(In reply to comment #9)
> Except, I'm not sure how to do that -- can you make a patch?  

My srpm can be downloaded from:
http://koji.fedoraproject.org/scratch/mtasaka/task_1255206/

Comment 12 Mamoru TASAKA 2009-03-24 18:04:22 UTC
Assigning.

Comment 13 Mamoru TASAKA 2009-03-24 18:37:22 UTC
Some notes:

* %define -> %global
  - Current packaging guidelines suggest to use %global instead
    of %define, especially when defining nested macros, see:
    https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
    https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
    (The latter one is still under "Drafts" category, but this is already
    accepted by FESCo)

* %debug_package
-----------------------------------------------------------------
# and apparently
# also stops checking other libs once it hits the first static lib
-----------------------------------------------------------------
  - The problem is not there. The real issue is that the installed
    shared libraries are already stripped:
-----------------------------------------------------------------
Symlinking from libgroebner-0.5.0.so to /builddir/build/BUILDROOT/python-polybori-0.5-3.fc11.1.x86_64/usr/lib64/libgroebner.so
g++ -o polybori/libpolybori-0.5.0.so.0.0.0 -s -shared -Wl,-soname,libpolybori-0.5.0.so.0 ....
-----------------------------------------------------------------
    note that "-s" option is passed to g++.
    Perhaps modifying "LDFLAGS_LINUX" in polybori/Makefile.in will
    solve this issue.

* License
  - Seems GPLv2+. Would you check this?

* Requires
  - Would you check the dependency "i.e. Requires" for -devel subpackage?
     ! For example, polybori/groebner/groebner_alg.h contains:
-----------------------------------------------------------------
    12  #include <polybori.h>
    13  #include "groebner_defs.h"
    14  #include "pairs.h"
    15  #include <boost/dynamic_bitset.hpp>
-----------------------------------------------------------------
      This means -devel subpackage should have "Requires: boost-devel"
      (here I am not saying about BuildRequires).
-----------------------------------------------------------------
# rpm -ql python-polybori-devel | xargs grep -h 'include ' | sort | uniq
-----------------------------------------------------------------
      will show some useful information.
    ! And this output shows that polybori/CDDManager.h contains:
-----------------------------------------------------------------
   102  #define CDDManager_h_
   103  #include "cacheopts.h"
   104  // load basic definitions
   105  #include "pbori_defs.h"
-----------------------------------------------------------------
      Would you check if cacheopts.h can be really removed?

  - Also please check the dependency for main package.
    ! For example, polybori/nf.py contains:
-----------------------------------------------------------------
    81      global mat_counter
    82      mat_counter=mat_counter+1
    83      import Image, ImageDraw
-----------------------------------------------------------------
      This may mean that python-polybori should have "Requires: python-imaging".

-----------------------------------------------------------------
# rpm -ql python-polybori | grep -v /usr/share/doc | LANG=C xargs grep -h 'import ' | grep -v Binary | sort | uniq
-----------------------------------------------------------------
      will show some useful information.

    ! Also
-----------------------------------------------------------------
$ LANG=C ipbori
/usr/bin/ipbori: line 66: ipython: command not found
-----------------------------------------------------------------
      Perhaps "Requires: ipython" is needed.

* SourceURL
  - Please follow:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

* Linkage on installed system libraries
------------------------------------------------------------------
# rpmlint python-polybori
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libpolybori-0.5.0.so.0.0.0 Cudd_zddUnion
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libpolybori-0.5.0.so.0.0.0 _Z12defaultErrorSs
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libpolybori-0.5.0.so.0.0.0 Cudd_ReadZddSize
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libpolybori-0.5.0.so.0.0.0 cuddCacheInsert1
.....
.....
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libgroebner-0.5.0.so.0.0.0 Cudd_zddUnion
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libgroebner-0.5.0.so.0.0.0 _Z12defaultErrorSs
python-polybori.i586: W: undefined-non-weak-symbol /usr/lib/libgroebner-0.5.0.so.0.0.0 Cudd_ReadZddSize
.....
.....
------------------------------------------------------------------
  - These two libraries undefined non-weak symbols. 
    This cannot be allowed when these libraries also have in
    -devel subpackage named "libfoo.so" used for linkage against 
    these libraries, because these symbols will cause linkage error.

* Documents
  - I prefer to include README as %doc of main package for this case.
  - ChangeLog seems useful for -devel subpackage.

Comment 14 Conrad Meyer 2009-03-24 20:07:29 UTC
(In reply to comment #13)
> * %define -> %global

Done.

> * %debug_package
>     Perhaps modifying "LDFLAGS_LINUX" in polybori/Makefile.in will
>     solve this issue.

This doesn't fix it --- I also removed -s as a link flag from SConstruct, and now it builds without stripping at link time.

> * License
>   - Seems GPLv2+. Would you check this?

Ah. The only source I could find (sourceforge.net) just said GPL, so I assumed GPLv3. I guess LICENSE says GPLv2+, good (and at least one source file says the license information is in LICENSE).

> * Requires
>       This means -devel subpackage should have "Requires: boost-devel"
>       (here I am not saying about BuildRequires).

Good catch; fixed.

> -----------------------------------------------------------------
> # rpm -ql python-polybori-devel | xargs grep -h 'include ' | sort | uniq
> -----------------------------------------------------------------
>       will show some useful information.
>     ! And this output shows that polybori/CDDManager.h contains:
> -----------------------------------------------------------------
>    102  #define CDDManager_h_
>    103  #include "cacheopts.h"
>    104  // load basic definitions
>    105  #include "pbori_defs.h"
> -----------------------------------------------------------------
>       Would you check if cacheopts.h can be really removed?

Well, it's an empty file and rpm complains about that. I will not remove it, for now.
 
>   - Also please check the dependency for main package.
>       This may mean that python-polybori should have "Requires:
> python-imaging".

Done, thanks for spotting that.

> $ LANG=C ipbori
> /usr/bin/ipbori: line 66: ipython: command not found
> -----------------------------------------------------------------
>       Perhaps "Requires: ipython" is needed.

Yep.
 
> * SourceURL
>   - Please follow:
>     https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

Thanks, I couldn't find that when I was originally packaging this. Fixed.

> * Linkage on installed system libraries
>   - These two libraries undefined non-weak symbols. 
>     This cannot be allowed when these libraries also have in
>     -devel subpackage named "libfoo.so" used for linkage against 
>     these libraries, because these symbols will cause linkage error.

What do I need to fix for this?

> * Documents
>   - I prefer to include README as %doc of main package for this case.
>   - ChangeLog seems useful for -devel subpackage.

Ok, added.

Update URLs:
http://konradm.fedorapeople.org/fedora/SPECS/python-polybori.spec
http://konradm.fedorapeople.org/fedora/SRPMS/python-polybori-0.5-4.fc10.src.rpm

Comment 15 Mamoru TASAKA 2009-03-25 22:20:27 UTC
Well, today I had no time to review your newest srpm, however:

(In reply to comment #14)
> > * %debug_package
> >     Perhaps modifying "LDFLAGS_LINUX" in polybori/Makefile.in will
> >     solve this issue.
> 
> This doesn't fix it --- I also removed -s as a link flag from SConstruct, and
> now it builds without stripping at link time.

- What do you mean by "This doesn't fix it"? Your latest srpm
  now correctly generates debuginfo rpm (note that of course
  static archives are ignored)

> > * Requires
> >       This means -devel subpackage should have "Requires: boost-devel"
> >       (here I am not saying about BuildRequires).
> 
> Good catch; fixed.

- Please check if only "boost-devel" is needed as Requires.
  For example it seems that "Requires: cudd-devel" is also needed
  (as polybori/pbori_algo.h contains
-------------------------------------------------------
   162  // temporarily
   163  #include "cudd.h"
   164  #include "cuddInt.h"
   165  #include "CCuddInterface.h"
------------------------------------------------------- )
  (However m4ri-devel doesn't seem to be needed for Requires)

> > * Linkage on installed system libraries
> >   - These two libraries undefined non-weak symbols. 
> >     This cannot be allowed when these libraries also have in
> >     -devel subpackage named "libfoo.so" used for linkage against 
> >     these libraries, because these symbols will cause linkage error.
> 
> What do I need to fix for this?
- Please remove these undefined non-weak symbols by making
  those two libraies linked against proper libraries.

  Most of these undefined symbols on these 2 libraies are actually
  in libcudd.so.2, for making libpolybori.so and libgroebner.so
  linked against libcudd.so.2 (by adding "-lcudd" to LDFLAGS or so,
  for example) will solve _most_ of the problems, but as far as
  I checked not all.

  The trouble is that the symbol "_Z12defaultErrorSs" (after demangled
  it is "defaultError(std::string)") is not defined anywhere.
  Actually obj/cuddObj.cc in cudd-2.4.1 seems to define this funtion,
  however build.log for cudd shows that this source is not compiled
  and so is not included in the libraries in cudd binary rpms.
  So currently no libraries contain "Z12defaultErrorSs" symbol
  and this will cause linkage error

  I guess the code (or Makefiles) in cudd must need fixing so that
  defaultError(std::string) symbols is actually included in
  the libraries in cudd.

Comment 16 Conrad Meyer 2009-03-25 23:16:22 UTC
(In reply to comment #15)
> - What do you mean by "This doesn't fix it"? Your latest srpm
>   now correctly generates debuginfo rpm (note that of course
>   static archives are ignored)

Sorry, that alone didn't fix it. I also had to make changes via sed to the SConstruct file.

> - Please check if only "boost-devel" is needed as Requires.
>   For example it seems that "Requires: cudd-devel" is also needed
>   (as polybori/pbori_algo.h contains
>   (However m4ri-devel doesn't seem to be needed for Requires)

Ah, yes, this seems correct.

> - Please remove these undefined non-weak symbols by making
>   those two libraies linked against proper libraries.
> 
>   Most of these undefined symbols on these 2 libraies are actually
>   in libcudd.so.2, for making libpolybori.so and libgroebner.so
>   linked against libcudd.so.2 (by adding "-lcudd" to LDFLAGS or so,
>   for example) will solve _most_ of the problems, but as far as
>   I checked not all.
> 
>   The trouble is that the symbol "_Z12defaultErrorSs" (after demangled
>   it is "defaultError(std::string)") is not defined anywhere.
>   Actually obj/cuddObj.cc in cudd-2.4.1 seems to define this funtion,
>   however build.log for cudd shows that this source is not compiled
>   and so is not included in the libraries in cudd binary rpms.
>   So currently no libraries contain "Z12defaultErrorSs" symbol
>   and this will cause linkage error
> 
>   I guess the code (or Makefiles) in cudd must need fixing so that
>   defaultError(std::string) symbols is actually included in
>   the libraries in cudd.

Ok, I will fix this to link against cudd at build time, and fix cudd to include that symbol. I wasn't sure if libraries could link to other libraries (I am fairly new to doing anything beyond basic linking).

Comment 17 Conrad Meyer 2009-03-29 02:14:39 UTC
I don't get warnings from rpmlint about undefined non-weak symbols other than  _Z12defaultErrorSs so I think I did it right:

http://konradm.fedorapeople.org/fedora/SPECS/python-polybori.spec
http://konradm.fedorapeople.org/fedora/SRPMS/python-polybori-0.5-5.fc10.src.rpm

Now to fix cudd, I guess.

Comment 18 Conrad Meyer 2009-03-29 02:57:13 UTC
I think I fixed it with the new patch here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1263843

Though I did not test it yet.

Comment 19 Mamoru TASAKA 2009-03-29 17:06:39 UTC
Created attachment 337176 [details]
Patch to build libcuddobj.so

Well,
- python-polybori side now seems good (I have not retried full review
  yet)
- however cudd side is not yet good,

as libpolybori.so cannot resolve defaultError(std::string) symbol:
-----------------------------------------------------
# ldd -r /usr/lib/libpolybori-0.5.0.so
undefined symbol: _Z12defaultErrorSs    (/usr/lib/libpolybori-0.5.0.so)
        linux-gate.so.1 =>  (0x00e7d000)
        libcudd.so.2 => /usr/lib/libcudd.so.2 (0x00f24000)
        libm4ri-0.0.20081029.so => /usr/lib/libm4ri-0.0.20081029.so (0x0050c000)
        libstdc++.so.6 => /usr/lib/libstdc++.so.6 (0x006b8000)
        libm.so.6 => /lib/libm.so.6 (0x0057e000)
        libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x005f0000)
        libc.so.6 => /lib/libc.so.6 (0x00110000)
        libmtr.so.2 => /usr/lib/libmtr.so.2 (0x00281000)
        libcuddst.so.2 => /usr/lib/libcuddst.so.2 (0x00abf000)
        libcuddutil.so.2 => /usr/lib/libcuddutil.so.2 (0x00b9a000)
        libepd.so.2 => /usr/lib/libepd.so.2 (0x00c75000)
        /lib/ld-linux.so.2 (0x00b76000)
-------------------------------------------------------

As I said in the previous comment defaultError(std::string) is
defined in obj/cuddObj.cc
- A extra library under obj needs building. This library is not built
  by default so a patch is needed. Your patch builds a static archive
  under obj/ but does not seem to build shared library under obj/ .

  ! By the way the library under obj/ directory is named as libobj.so,
    however I think this naming is very bad and this library should be
    renamed as libcuddobj.so .
  ! Also without a path the rebuilt libcuddobj.so (after renamed)
    library is broken as this contains undefined non-weak symbols :(,
    because
    - although this is C++ library, "cc" is used for linkage.
      Note that libcudd.so is C library
    - this library is not properly linkaged against libcudd.so

  For me libcuddobj.so can be properly built by the attached patch.

- After this, libpolybori.so and libgroebner.so should be
  linkaged against both libcudd.so and libcuddobj.so.

  ! Note
    First I tried to merge libcuddobj.so into libcudd.so, however
    as said before libcudd.so is C library but libcuddobj.so contains
    C++ function, so this should not be done.

Comment 20 Conrad Meyer 2009-03-29 21:41:43 UTC
Ok, built:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1264624

Time to fix polybori to link against libcuddobj as well :).

Thank you very much for the patch.

Comment 21 Mamoru TASAKA 2009-03-30 16:43:22 UTC
Okay, now it seems okay with replacing like below:
-----------------------------------------------------------
sed -i -e 's|^LDFLAGS_LINUX.*-s|LDFLAGS_LINUX = -lcudd -lcuddobj|' \
  polybori/Makefile.in
sed -i -e "s|\['-s'\]|['-lcudd -lcuddobj']|" SConstruct
-----------------------------------------------------------
http://koji.fedoraproject.org/koji/taskinfo?taskID=1265939

-----------------------------------------------------------
  This package (python-polybori) is APPROVED by mtasaka
-----------------------------------------------------------

Comment 22 Conrad Meyer 2009-03-31 21:54:01 UTC
New Package CVS Request
=======================
Package Name: python-polybori
Short Description: Framework for Boolean Rings
Owners: konradm
Branches: F-10
InitialCC:

Comment 23 Dennis Gilmore 2009-04-01 16:38:43 UTC
CVS Done

Comment 24 Conrad Meyer 2009-04-04 01:04:25 UTC
Imported and built in rawhide; closing.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1276530


Note You need to log in before you can comment on or make changes to this bug.