Opened 5 years ago

Closed 5 years ago

#12655 closed enhancement (fixed)

Update PolyBoRi to release 0.8.1

Reported by: AlexanderDreyer Owned by: AlexanderDreyer
Priority: major Milestone: sage-5.0
Component: packages: standard Keywords:
Cc: PolyBoRi, malb, burcin Merged in: sage-5.0.beta13
Authors: Alexander Dreyer Reviewers: Martin Albrecht, Jeroen Demeyer, Leif Leonhardy
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: Commit:
Dependencies: #12656, #12750, #12799 Stopgaps:

Description (last modified by AlexanderDreyer)

PolyBoRi's next minor release is out now. There were no changes of the interface between PolyBoRi 0.8.0 and 0.8.1, so we just have to update the sources from here:

https://sourceforge.net/projects/polybori/files/polybori/0.8.1

Current spkg

This also fixes building with GCC-4.7.0, see #12751 for the GCC-4.7.0 metaticket.

Change History (74)

comment:1 Changed 5 years ago by AlexanderDreyer

  • Description modified (diff)

comment:2 Changed 5 years ago by AlexanderDreyer

  • Status changed from new to needs_review

comment:3 Changed 5 years ago by AlexanderDreyer

  • Dependencies set to #12656

Even though the spkg works without a patch to the sage library, I recommend to use the patch from #12656, also. It fixed inconsistencies between the original interface and Sage's Cython-bases reimplementation.

comment:4 Changed 5 years ago by AlexanderDreyer

For sage-5.0.beta7 (with #12656) the spkg installs and tests well (make ptestlong) on a SuSE Enterprise 11 AMD64.

comment:5 Changed 5 years ago by AlexanderDreyer

Also works without #12656 (not recommended).

comment:6 Changed 5 years ago by malb

I can confirm that doctests pass on geom.math with 5.0.beta8.

comment:7 Changed 5 years ago by AlexanderDreyer

  • Description modified (diff)

Here's an updates spkg. http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1rc5.p0.spkg Note: upstream PolyBoRi can detect and use Sage (if available), so we do not have to patch PyPolyBoRi?.py every time again and again.

comment:8 follow-up: Changed 5 years ago by malb

Since it's still an RC, do we aim for 'positive review' already?

comment:9 in reply to: ↑ 8 Changed 5 years ago by AlexanderDreyer

Replying to malb:

Since it's still an RC, do we aim for 'positive review' already?

It should be final now, but I'd like to wait for response from non-Linux people. Maybe you can give a conditional review for the case, the spkg doesn't change any more. (There's no rc5 marker inside the spkg, so we just have to rename it, if we are lucky.)

comment:10 Changed 5 years ago by malb

Sounds like a plan. conditional positive review it is :)

comment:11 Changed 5 years ago by AlexanderDreyer

  • Description modified (diff)

Unfortunately, I had to rebundle the spkg, because the directory name contained rc5 (D'Oh!). Here is the updated spkg: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p0.spkg Also, there is one last-minute change in the source (not doctesting optional polybori.plot, if prerequsite dot is not available): https://bitbucket.org/brickenstein/polybori/changeset/b3a12752980c

The recent spkg installed on SuSE11 AMD64 and OS X 10.5 ppc. make ptestlong succeeded on SuSE (OS X still running.)

comment:12 Changed 5 years ago by malb

  • Reviewers set to Martin Albrecht
  • Status changed from needs_review to positive_review

Hey, I checked the SPKG for obvious problems and it looks clean. I didn't install it again and ran tests, but those the buildbot can figure out. So: positive review.

comment:13 Changed 5 years ago by AlexanderDreyer

Finally, the rc became release, indeed!

comment:14 follow-up: Changed 5 years ago by jdemeyer

It is recommended to use patch for patching SPKGs, see http://sagemath.org/doc/developer/patching_spkgs.html. If not, at least add a corresponding .patch file for custom.py.

I'm not setting this ticket back to needs_work for this but it SHOULD be fixed.

comment:15 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by AlexanderDreyer

Replying to jdemeyer:

It is recommended to use patch for patching SPKGs, see http://sagemath.org/doc/developer/patching_spkgs.html. If not, at least add a corresponding .patch file for custom.py.

A patch might become part of upstream sources eventually. But custom.py is a user configuration file, which will never be upstream itself (of course the mechanism already supported there). Of course, we can generate a patch against /dev/null. but does it really makes sense?

comment:16 in reply to: ↑ 15 Changed 5 years ago by AlexanderDreyer

Replying to AlexanderDreyer:

Of course, we can generate a patch against /dev/null. but does it really makes sense?

Ok, answering myself: it's part of the directory patch, so of course one should consider it as a patch to upstream. There's no config directory mention here http://sagemath.org/doc/developer/producing_spkgs.html , so patching is the only option here.

comment:17 Changed 5 years ago by AlexanderDreyer

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

The updated spkg is now here: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p0.spkg Also, I added two minor fixes from upstream. One fixes a preprocessor misconception (#if instead of #ifdef, and the other fixes a doctest prerequisites check, which prevented ipbori's self test to check polybori.plot.

So, unfortunately we need a review again.

comment:18 Changed 5 years ago by AlexanderDreyer

  • Status changed from needs_work to needs_review

comment:19 follow-up: Changed 5 years ago by fbissey

I agree that "custom.py" is not a patch to upstream in any way. It is a custom configuration and there is nothing in it that couldn't be actually be shifted in spkg-install. Putting it in custom.py probably keep things cleaner. If you don't want to pollute the patch folder could we consider putting it in the spkg root directory?

comment:20 in reply to: ↑ 19 ; follow-up: Changed 5 years ago by jdemeyer

Replying to fbissey:

If you don't want to pollute the patch folder could we consider putting it in the spkg root directory?

I agree with this. I saw the file in patches/ and assumed it was a patch to some existing PolyBoRi file. If it's not a patch, it doesn't belong in patches/.

comment:21 Changed 5 years ago by AlexanderDreyer

Ok, I moved custom.py to the spkg root. The spkg-install is now more generic: it takes any *.patches in patches and apply them to the sources. The updated spkg is at the ususal place.

comment:22 in reply to: ↑ 20 Changed 5 years ago by AlexanderDreyer

Replying to jdemeyer:

Replying to fbissey:

If you don't want to pollute the patch folder could we consider putting it in the spkg root directory?

I agree with this. I saw the file in patches/ and assumed it was a patch to some existing PolyBoRi file. If it's not a patch, it doesn't belong in patches/.

I fixed that, so the package needs a new review.

comment:23 follow-up: Changed 5 years ago by fbissey

Sorry to be a little bit annoying. Now that custom.py is in the root folder of the spkg could you add a "special files" section to SPKG.txt, list custom.py and explain what it is.

comment:24 in reply to: ↑ 23 Changed 5 years ago by AlexanderDreyer

Replying to fbissey:

Sorry to be a little bit annoying. Now that custom.py is in the root folder of the spkg could you add a "special files" section to SPKG.txt, list custom.py and explain what it is.

No problem! You'll find an updated spkg at the usual place.

comment:25 Changed 5 years ago by jdemeyer

  • Reviewers changed from Martin Albrecht to Martin Albrecht, Jeroen Demeyer
  • Status changed from needs_review to positive_review

Looks good to me.

comment:26 Changed 5 years ago by malb

  • Status changed from positive_review to needs_work

comment:27 Changed 5 years ago by AlexanderDreyer

  • Status changed from needs_work to needs_review

Hi, I updated the spkg. Please have a look at it.

comment:28 Changed 5 years ago by malb

  • Status changed from needs_review to positive_review

Back to positive review. Thanks!

comment:29 Changed 5 years ago by jhpalmieri

  • Status changed from positive_review to needs_work

At least in combination with #12369, this fails to build on OpenSolaris on x86 (David Kirkby's machine hawk): it gives me

scons: done reading SConscript files.
scons: Building targets ...
writing config.h...
gcc -o Cudd/cudd/cuddAPI.pic.o -c -std=c99 -O3 -Wno-long-long -Wreturn-type -g -fPIC -mmmx -msse -msse2 -msse3 -g -fPIC -Wall -pedantic -O2 -KPIC -DPBORI_NDEBUG -DSIZEOF_VOID_P=4 -DSIZEOF_INT=4 -DSIZEOF_LONG=4 -DPBORI_HAVE_GD -DPBORI_HAVE_TR1_UNORDERED_MAP -DPBORI_HAVE_M4RI -Ilibpolybori/include -Igroebner/include -I/export/home/palmieri/testing/GCC-no-check/sage-5.0.beta10-gcc/local/include -I/export/home/palmieri/testing/GCC-no-check/sage-5.0.beta10-gcc/local/include/python2.7 -ICudd Cudd/cudd/cuddAPI.c
gcc: error: unrecognized option ‘-KPIC’
scons: *** [Cudd/cudd/cuddAPI.pic.o] Error 1

In contrast, the 0.8.0.p1 spkg builds fine.

On Solaris on sparc (mark2), in combination with #12369, I get this:

Starting build...
Removing old PolyBoRi install...
Done removing old PolyBoRi install.
Running build_polybori...
scons: Reading SConscript files ...
Platform:  sunos5
Variable LIBSUFFIX not in default environment!
Platform:  sunos5
Detecting type sizes... no
Could not detect type sizes (maybe compile/link flags trouble)! Exiting.

comment:30 Changed 5 years ago by AlexanderDreyer

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

Thanks, I think I found the issue. Please try out the following: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p1.spkg

Note: I updated the spkg from #12754 which already fixed other things (for instance gcc-4.7 compatibility.)

comment:31 follow-up: Changed 5 years ago by jhpalmieri

With this package, on both Solaris and OpenSolaris, I get this:

Starting build...
Removing old PolyBoRi install...
Done removing old PolyBoRi install.
Running build_polybori...
scons: Reading SConscript files ...
NameError: name 'CCFLAGS' is not defined:
  File "/export/home/palmieri/testing/sage-5.0.beta7-gcc/spkg/build/polybori-0.8.1.p1/src/polybori-0.8.1/SConstruct", line 241:
    if is_gcc():
  File "/export/home/palmieri/testing/sage-5.0.beta7-gcc/spkg/build/polybori-0.8.1.p1/src/polybori-0.8.1/SConstruct", line 239:
    compilerenv = Environment(ENV = os.environ, options = opts)
  File "/export/home/palmieri/testing/sage-5.0.beta7-gcc/local/lib/scons-1.2.0/SCons/Environment.py", line 975:
    variables.Update(self)
  File "/export/home/palmieri/testing/sage-5.0.beta7-gcc/local/lib/scons-1.2.0/SCons/Variables/__init__.py", line 169:
    execfile(filename, {}, values)
  File "custom.py", line 6:
    CCFLAGS += ["-Wno-long-long", "-Wreturn-type"]
Error building PolyBoRi.

Should CCFLAGS be CFLAGS? If I fix this, I get a similar error for GD_LIBS. I ended up applying the following patch:

  • custom.py

    diff --git a/custom.py b/custom.py
    a b  
    11import os
    22import sys
    33
    4 # FIXME: Do we really want to *overwrite* the flags (e.g. if set by the user)?
    5 # Answer: Should be fixed now.
    6 CCFLAGS += ["-Wno-long-long", "-Wreturn-type"]
    7 
    84# Note: PolyBoRi still appends DEFAULT_*FLAGS (overwrite those, if necessary)
    95if 'CFLAGS' in os.environ:
    106  CFLAGS = os.environ['CFLAGS'].split(' ')
     7else:
     8  CFLAGS = []
     9
     10# FIXME: Do we really want to *overwrite* the flags (e.g. if set by the user)?
     11# Answer: Should be fixed now.
     12CFLAGS += ["-Wno-long-long", "-Wreturn-type"]
    1113
    1214if 'CXXFLAGS' in os.environ:
    1315  CXXFLAGS = os.environ['CXXFLAGS'].split(' ')
     
    1517if 'CPPFLAGS' in os.environ:
    1618  CCFLAGS = os.environ['CPPFLAGS'].split(' ')
    1719
     20if 'GD_LIBS' in os.environ:
     21  GD_LIBS = os.environ['GD_LIBS'].split(' ')
     22else:
     23  GD_LIBS = []
     24
    1825GD_LIBS+=["png12","z"]
    1926
    2027# FIXME: Should we include LDFLAGS here? (see above)

The spkg-install file is odd, though: some places it refers to CFLAGS, others to CCFLAGS, so I'm not sure my changes are correct. In any case, it worked on OpenSolaris, but on Solaris I got the same error as before ("Could not detect type sizes"). This may be related to #12369.

comment:32 in reply to: ↑ 31 ; follow-ups: Changed 5 years ago by AlexanderDreyer

Replying to jhpalmieri:

With this package, on both Solaris and OpenSolaris, I get this: Should CCFLAGS be CFLAGS? If I fix this, I get a similar error for GD_LIBS. I ended up applying the following patch:

Sorry, I made a mistake. I did not have a recent Sage at hand on Solaris, so I tried that out without custom.py. (I need to fix that upstream again. Your patch is a good work around.)

The spkg-install file is odd, though: some places it refers to CFLAGS, others to CCFLAGS, so I'm not sure my changes are correct.

CFLAGS are flags for pure C, CCFLAGS for C or C++.

In any case, it worked on OpenSolaris, but on Solaris I got the same error as before ("Could not detect type sizes"). This may be related to #12369.

There a file called config.log. Can you post it somewhere?

comment:33 in reply to: ↑ 32 Changed 5 years ago by AlexanderDreyer

Sorry, I made a mistake. I did not have a recent Sage at hand on Solaris, so I tried that out without custom.py. (I need to fix that upstream again. Your patch is a good work around.)

But the CCFLAGS should still be there, for instance:

CCFLAGS = ["-O3", "-Wno-long-long", "-Wreturn-type"]

comment:34 follow-up: Changed 5 years ago by jhpalmieri

If I add a print statement at the top of custom.py ("print CCFLAGS"), then on OS X or sage.math, I see ['-O3']. On OpenSolaris, I get a NameError saying that CCFLAGS is not defined.

With Solaris (skynet machine mark2), the entire contents of config.log:

file /home/palmieri/mark2/GCC/sage-5.0.beta10-gcc/spkg/build/polybori-0.8.1.p1/src/polybori-0.8.1/SConstruct,line 653:
        Configure(confdir = .sconf_temp)
scons: Configure: Detecting type sizes... 
.sconf_temp/conftest_0.c <-
  |
  |        #include <stdio.h>
  |        int main(int argc, char **argv) {
  |          printf("SIZEOF_VOID_P=%u SIZEOF_INT=%u SIZEOF_LONG=%u",
  |            (unsigned)sizeof(void*), (unsigned)sizeof(int), (unsigned)sizeof(long));
  |          return 0;
  |        }
  |        
gcc -o .sconf_temp/conftest_0.o -c -Wno-long-long -Wreturn-type -std=c99 -O3 -DPBORI_NDEBUG -I/home/palmieri/mark2/GCC/sage-5.0.beta10-gcc/local/include .sconf_temp/conftest_0.c
gcc -o .sconf_temp/conftest_0 -Wl,-rpath=\$ORIGIN/../build/usr/local/lib -z origin .sconf_temp/conftest_0.o -L/home/palmieri/mark2/GCC/sage-5.0.beta10-gcc/local/lib
ld: fatal: option -dn and -P are incompatible
ld: fatal: Flags processing errors
collect2: ld returned 1 exit status
scons: Configure: no

comment:35 in reply to: ↑ 32 ; follow-up: Changed 5 years ago by jdemeyer

Replying to AlexanderDreyer:

CFLAGS are flags for pure C, CCFLAGS for C or C++.

I don't think this is standard usage. Normally, one uses CFLAGS for C and CXXFLAGS for C++. Of course, you can additionally support CCFLAGS if you want.

comment:36 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:37 in reply to: ↑ 35 ; follow-up: Changed 5 years ago by AlexanderDreyer

Replying to jdemeyer:

Replying to AlexanderDreyer:

CFLAGS are flags for pure C, CCFLAGS for C or C++.

I don't think this is standard usage. Normally, one uses CFLAGS for C and CXXFLAGS for C++. Of course, you can additionally support CCFLAGS if you want.

Well, that's not my idea, that's scons' standard. But I use CCFLAGS only internally (for instance for storing CPPFLAGS from the environment.

comment:38 in reply to: ↑ 34 Changed 5 years ago by AlexanderDreyer

Replying to jhpalmieri:

If I add a print statement at the top of custom.py ("print CCFLAGS"), then on OS X or sage.math, I see ['-O3']. On OpenSolaris, I get a NameError saying that CCFLAGS is not defined.

With Solaris (skynet machine mark2), the entire contents of config.log:

ld: fatal: option -dn and -P are incompatible
ld: fatal: Flags processing errors
collect2: ld returned 1 exit status
scons: Configure: no

Ok, that's arising from -rpath, As I understand it must be -R for the Solaris-linker. I'll provide a patch for that.

comment:39 in reply to: ↑ 37 Changed 5 years ago by jdemeyer

Replying to AlexanderDreyer:

Well, that's not my idea, that's scons' standard.

Not the first strange idea from the SCons developers... (honestly, if I encounter a SCons developer on the street, I'd like to punch him in the face)

comment:40 Changed 5 years ago by AlexanderDreyer

There's an updated spkg at the usual place: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p1.spkg

Please have a look at it.

comment:41 follow-up: Changed 5 years ago by jhpalmieri

On OpenSolaris: it seems to build correctly. I haven't done any testing, though.

On Solaris:

Running build_polybori...
scons: Reading SConscript files ...
Platform:  sunos5
Platform:  sunos5
ValueError: list.remove(x): x not in list:
  File "/home/palmieri/mark2/GCC/sage-5.0.beta10-gcc/spkg/build/polybori-0.8.1.p1/src/polybori-0.8.1/SConstruct", line 472:
    tools.remove(arg)

comment:42 in reply to: ↑ 41 Changed 5 years ago by AlexanderDreyer

Replying to jhpalmieri:

On OpenSolaris: it seems to build correctly. I haven't done any testing, though.

On Solaris:

ValueError?: list.remove(x): x not in list:

File "/home/palmieri/mark2/GCC/sage-5.0.beta10-gcc/spkg/build/polybori-0.8.1.p1/src/polybori-0.8.1/SConstruct", line 472:

tools.remove(arg)

}}}

Ok, a plain typo. Please try our gain.

comment:43 Changed 5 years ago by jhpalmieri

Okay, this works now on Solaris.

comment:44 Changed 5 years ago by AlexanderDreyer

So back to positive review again? (I add a dependency on #12754, since I used the spkg from there, and ot formally still needs a positive review.)

comment:45 Changed 5 years ago by AlexanderDreyer

  • Dependencies changed from #12656 to #12656 #12754

comment:46 Changed 5 years ago by AlexanderDreyer

  • Description modified (diff)

comment:47 Changed 5 years ago by AlexanderDreyer

  • Component changed from algebra to packages

comment:48 Changed 5 years ago by AlexanderDreyer

  • Description modified (diff)

comment:49 Changed 5 years ago by jdemeyer

  • Dependencies changed from #12656 #12754 to #12656, #12750, #12754

comment:50 Changed 5 years ago by jdemeyer

  • Dependencies changed from #12656, #12750, #12754 to #12656, #12750

comment:51 follow-ups: Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think SPKG.txt needs some work:

  1. The Changelog messages don't tell much. If you add a patch, you should mention that fact in the Changelog. You should add a ticket number in the Changelog messages.
  2. The patches should be documented.

comment:52 in reply to: ↑ 51 Changed 5 years ago by AlexanderDreyer

Replying to jdemeyer:

I think SPKG.txt needs some work:

  1. The Changelog messages don't tell much. If you add a patch, you should mention that fact in the Changelog. You should add a ticket number in the Changelog messages.
  2. The patches should be documented.

No problem, I'll add this:

  • SPKG.txt

    diff -r f4822b70da5b SPKG.txt           
    a b  
    3636 * png/libpng12 (accomplished because Python and gd depend on it, too)
    3737 * libz         (accomplished because e.g. libpng depends on it)     
    3838                                                                     
     39== Patches ==                                                         
     40                                                                     
     41 * Sage Trac #12655 (Upgrade ticket, fixing known issues):           
     42   - gbcore.py.patch -- Fixing list access to ideal, which might be represented
     43     as a set here                                                             
     44   - ipbori.patch -- Fixing prerequisites check for testing plot.py           
     45   - nf.h.patch -- Fixing preprocessor misconception                           
     46 * Sage Trac #12750:                                                           
     47   - base_lookup.patch -- Fixing explict name lookup in template base (gcc 4.7)
     48 * Sage Trac #12655, comment:29:
     49   - SConstruct.patch -- Fixing gcc components on sun
     50
     51All patches were merged upstream.
     52
     53
    3954== Special Files ==
     55
    4056 * custom.py - configuration file for PolyBoRi's build system,
    4157               sets local prefix and install paths to Sage directories
    4258
     59
    4360== Releases ==
    4461
    4562=== polybori-0.8.1.p1 (Alexander Dreyer, March 26th, 2012) ===
    46  * Rebased spkg on polybori-0.8.0.p2, work around broken scons on sun
     63 * Rebased spkg on polybori-0.8.0.p2 (Sage Trac #12750)
     64 * Working around broken scons on sun (Sage Trac #12655, comment:29)
    4765
    4866=== polybori-0.8.1.p0 (Alexander Dreyer, March 7th, 2012) ===
    49  * Updating sources to PolyBoRi's release 0.8.1
     67 * Updating sources to PolyBoRi's release 0.8.1 (Sage Trac #12655)
    5068
    5169=== polybori-0.8.0.p2 (Alexander Dreyer, March 26th, 2012) ===
    5270 * Fix scoping/name look-up issue and support flags from the environment

comment:53 Changed 5 years ago by AlexanderDreyer

Now, the new spkg is uploaded at the usual place: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p1.spkg .

comment:54 follow-up: Changed 5 years ago by leif

  • Reviewers changed from Martin Albrecht, Jeroen Demeyer to Martin Albrecht, Jeroen Demeyer, Leif Leonhardy
  • Work issues set to Fix dependencies in `module_list.py`.

module_list.py certainly needs work w.r.t. dependencies.

After installing the new PolyBoRi spkg and running env SAGE_UPGRADING=yes make build (which reinstalls the Sage library as well as GAP and sagetex), I get dozens of import errors from Sphinx (since ptestlong depends on doc -- never understood why we have to build HTML documentation to run tests...).

Note that local/bin/sage-starts seemed to have succeeded, but in fact it was [ -f local/lib/sage-started.txt ] that did; i.e., of course also Sage doesn't start up.

That's because a couple of Sage library extension modules didn't get rebuilt, still referring to the old, deleted PolyBoRi shared library.

comment:55 follow-ups: Changed 5 years ago by leif

I also noticed that (e.g.) -O3 in C/CPP/CXXFLAGS gets overridden by -O2, but the latter apparently originates from M4RI's m4ri_config.h, i.e. __M4RI_CFLAGS.

Is it intentional to use all of them in all compile commands? (These are btw. also passed to g++ when compiling C++ sources.)


Furthermore, it's quite funny to use g++ -std=c++98 while using long long; I get hundreds of warnings for that, since -Wall is also there, but not -Wno-long-long. (-Wall comes from M4RI's CFLAGS as well.)

comment:56 Changed 5 years ago by leif

After running ./sage -ba-force, make ptestlong passes all tests (Sage 5.0.beta11, Ubuntu 12.04 beta x86_64, self-made GCC 4.6.3).

comment:57 in reply to: ↑ 54 ; follow-ups: Changed 5 years ago by leif

Replying to leif:

module_list.py certainly needs work w.r.t. dependencies.

[...]

That's because a couple of Sage library extension modules didn't get rebuilt, still referring to the old, deleted PolyBoRi shared library.

Actually only the pbori module needs to get rebuilt; while touching $SAGE_LOCAL/include/polybori.h doesn't help, touching $SAGE_LOCAL/include/polybori/* does (afterwards running ./sage -b).


Also builds with GCC 4.7.0 (Sage 5.0.beta10, Ubuntu 10.04.4 x86_64).

I noticed that different flags get passed to gcc/g++ depending on GCC's version, or probably the settings of CC and CXX. Seems rather a side-effect than intended behaviour...

W.r.t. M4RI's flags: It should be sufficient to only pass / add the contents of __M4RI_SIMD_CFLAGS, and probably only when compiling some of the modules.

comment:58 follow-up: Changed 5 years ago by leif

  • Work issues changed from Fix dependencies in `module_list.py`. to Fix dependencies in `module_list.py` and/or touch some headers from `spkg-install`.

comment:59 in reply to: ↑ 55 Changed 5 years ago by AlexanderDreyer

Replying to leif:

I also noticed that (e.g.) -O3 in C/CPP/CXXFLAGS gets overridden by -O2, but the latter apparently originates from M4RI's m4ri_config.h, i.e. __M4RI_CFLAGS.

Is it intentional to use all of them in all compile commands? (These are btw. also passed to g++ when compiling C++ sources.)

Otherwise you might get inconsistencies.


Furthermore, it's quite funny to use g++ -std=c++98 while using long long; I get hundreds of warnings for that, since -Wall is also there, but not -Wno-long-long. (-Wall comes from M4RI's CFLAGS as well.)

long long arises from M4RI, but M4RI assumes C99 which knows long long. But this is a known problem, I'll add -Wno-long-long back to the custom flags in the spkg. (I removed it, because PolyBoRi 0.8.1 was improved to be compatible wit c++98.)

comment:60 in reply to: ↑ 57 ; follow-up: Changed 5 years ago by AlexanderDreyer

Replying to leif:

Replying to leif:

module_list.py certainly needs work w.r.t. dependencies.

[...]

That's because a couple of Sage library extension modules didn't get rebuilt, still referring to the old, deleted PolyBoRi shared library.

Actually only the pbori module needs to get rebuilt; while touching $SAGE_LOCAL/include/polybori.h doesn't help, touching $SAGE_LOCAL/include/polybori/* does (afterwards running ./sage -b).

I'll add this to the spkg.

Also builds with GCC 4.7.0 (Sage 5.0.beta10, Ubuntu 10.04.4 x86_64).

I noticed that different flags get passed to gcc/g++ depending on GCC's version, or probably the settings of CC and CXX. Seems rather a side-effect than intended behaviour...

W.r.t. M4RI's flags: It should be sufficient to only pass / add the contents of __M4RI_SIMD_CFLAGS, and probably only when compiling some of the modules.

This is not related to the upgrade, so I'd prefer a new ticket on this.

comment:61 in reply to: ↑ 55 Changed 5 years ago by AlexanderDreyer

Replying to leif:

Furthermore, it's quite funny to use g++ -std=c++98 while using long long; I get hundreds of warnings for that, since -Wall is also there, but not -Wno-long-long. (-Wall comes from M4RI's CFLAGS as well.)

I double-checked that. Per default the spkg sets -Wno-long-long, It's probably overwritten by some C...FLAGS. Should I block -Wno-long-long such that it is never overwritten by the environment (like -std=c++98)?

comment:62 in reply to: ↑ 58 Changed 5 years ago by AlexanderDreyer

  • Status changed from needs_work to needs_info

Replying to leif: This issue is now #12799.

comment:63 in reply to: ↑ 60 ; follow-ups: Changed 5 years ago by jdemeyer

Replying to AlexanderDreyer:

Actually only the pbori module needs to get rebuilt; while touching $SAGE_LOCAL/include/polybori.h doesn't help, touching $SAGE_LOCAL/include/polybori/* does (afterwards running ./sage -b).

I'll add this to the spkg.

I think it's better to change module_list.py instead of the spkg.

comment:64 in reply to: ↑ 63 Changed 5 years ago by AlexanderDreyer

  • Dependencies changed from #12656, #12750 to #12656, #12750, #12799
  • Status changed from needs_info to needs_review
  • Work issues Fix dependencies in `module_list.py` and/or touch some headers from `spkg-install`. deleted

Replying to jdemeyer:

I think it's better to change module_list.py instead of the spkg.

This part is now #12799, so I add this as dependency and remove the work issue.

I also updated the spkg to use -Wno-long-long for C++ here: http://boxen.math.washington.edu/home/dreyer/spkg/polybori-0.8.1.p1.spkg

comment:65 in reply to: ↑ 57 Changed 5 years ago by AlexanderDreyer

Replying to leif:

W.r.t. M4RI's flags: It should be sufficient to only pass / add the contents of __M4RI_SIMD_CFLAGS, and probably only when compiling some of the modules.

@malb What do you think: Would this be enough?

comment:66 in reply to: ↑ 63 ; follow-up: Changed 5 years ago by leif

Replying to jdemeyer:

Replying to AlexanderDreyer:

Actually only the pbori module needs to get rebuilt; while touching $SAGE_LOCAL/include/polybori.h doesn't help, touching $SAGE_LOCAL/include/polybori/* does (afterwards running ./sage -b).

I'll add this to the spkg.

I think it's better to change module_list.py instead of the spkg.

IIRC the problem isn't only which files the pbori module depends on, but also that the headers have their original file date from upstream, i.e., aren't generated during build nor (currently) touched from spkg-install, such that they'll usually be older than a present pbori module.

If one doesn't want to change the timestamps, one could make the pbori module just (or also) depend on the .so (without a version suffix), since the (target of the) latter will always have a fresh modification time.

comment:67 in reply to: ↑ 66 ; follow-up: Changed 5 years ago by AlexanderDreyer

Replying to leif:

If one doesn't want to change the timestamps, one could make the pbori module just (or also) depend on the .so (without a version suffix), since the (target of the) latter will always have a fresh modification time.

The libraries are already listed as libraries=['polybori', 'polybori_groebner', 'gd', 'png12', 'm4ri']. Aren't they considered as dependencies anyway?

But the timestamp of include/polybori/config.h varies, so #12799 fixes the problem.

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

comment:68 in reply to: ↑ 67 ; follow-up: Changed 5 years ago by leif

Replying to AlexanderDreyer:

Replying to leif:

If one doesn't want to change the timestamps, one could make the pbori module just (or also) depend on the .so (without a version suffix), since the (target of the) latter will always have a fresh modification time.

The libraries are already listed as libraries=['polybori', 'polybori_groebner', 'gd', 'png12', 'm4ri']. Aren't they considered as dependencies anyway?

Of course the libraries are listed, since they need to get linked to the module, but Cython doesn't automatically consider their timestamps for dependency checking, i.e., which modules need to get rebuilt.


But the timestamp of include/polybori/config.h varies, so #12799 fixes the problem.

Then I'd add just that there.

comment:69 in reply to: ↑ 68 ; follow-up: Changed 5 years ago by AlexanderDreyer

Replying to leif:

Then I'd add just that there.

Done. Is this ticket positively reviewed again?

comment:70 in reply to: ↑ 69 ; follow-up: Changed 5 years ago by leif

Replying to AlexanderDreyer:

Replying to leif:

Then I'd add just that there.

Done. Is this ticket positively reviewed again?

Can't tell. Jeroen last set it from needs_review to needs_work, and the issues he mentioned seem to be fixed, as well as mine regarding the dependencies (now at #12799, which this ticket depends on).

If the rest of the spkg already had positive review, you could probably revert it to that, but the other reviewers can probably tell better.

comment:71 in reply to: ↑ 51 Changed 5 years ago by AlexanderDreyer

Replying to jdemeyer:

I think SPKG.txt needs some work:

  1. The Changelog messages don't tell much. If you add a patch, you should mention that fact in the Changelog. You should add a ticket number in the Changelog messages.
  2. The patches should be documented.

Also done (see above). @jdemeyer Back to positive?

comment:72 in reply to: ↑ 70 Changed 5 years ago by leif

Replying to leif:

Replying to AlexanderDreyer:

Is this ticket positively reviewed again?

[...] the issues he mentioned seem to be fixed, as well as mine regarding the dependencies (now at #12799, which this ticket depends on).

... which now has positive review.

comment:73 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Positive review then... provided it survives testing.

comment:74 Changed 5 years ago by jdemeyer

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