Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#19781 closed enhancement (fixed)

Move broken optional packages to experimental

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-7.0
Component: packages: experimental Keywords:
Cc: dimpase, ncohen, frederichan, vinceknight, kcrisman, mkoeppe, mmarco, malb, mmezzarobba Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: dd4e0cd (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

The following optional packages are broken and should either be fixed or moved to experimental:

  • cryptominisat: crashes Sage on 32-bit.
  • lie: a very arcane build process (copies over the complete build directory and requires bison).

Change History (74)

comment:1 Changed 4 years ago by jdemeyer

  • Cc ncohen added
  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Cc frederichan added

comment:3 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 4 years ago by ncohen

I did not know that bliss was broken. Could you say what the problem is?

comment:5 Changed 4 years ago by jdemeyer

bliss is probably an easy fix:

sage -t --long --warn-long 85.9 src/sage/graphs/bliss.pyx
**********************************************************************
File "src/sage/graphs/bliss.pyx", line 251, in sage.graphs.bliss.canonical_form
Failed example:
    canonical_form(G)                                             # optional - bliss
Expected:
    [(2, 0), (2, 1), (3, 0), (4, 1), (5, 3), (5, 4), (6, 0), (6, 4),
     (7, 1), (7, 3), (8, 2), (8, 5), (9, 6), (9, 7), (9, 8)]
Got:
    [(2L, 0L),
     (2L, 1L),
     (3L, 0L),
     (4L, 1L),
     (5L, 3L),
     (5L, 4L),
     (6L, 0L),
     (6L, 4L),
     (7L, 1L),
     (7L, 3L),
     (8L, 2L),
     (8L, 5L),
     (9L, 6L),
     (9L, 7L),
     (9L, 8L)]
**********************************************************************

comment:6 follow-up: Changed 4 years ago by jdemeyer

If you intent to fix it, please open a new ticket.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by ncohen

If you intent to fix it, please open a new ticket.

I will. Perhaps you will admit that it is a bit brutal to change a package's status because it returns longs instead of ints in a doctest.

Nathann

comment:8 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by jdemeyer

Replying to ncohen:

I will. Perhaps you will admit that it is a bit brutal to change a package's status because it returns longs instead of ints in a doctest.

Well, either the test needs to be fixed or the package's status changed.

comment:10 in reply to: ↑ 9 Changed 4 years ago by ncohen

Well, either the test needs to be fixed or the package's status changed.

True, that's a bug alright.

comment:11 follow-up: Changed 4 years ago by tscrim

  • Cc vinceknight kcrisman mkoeppe added

I would be happy to fix coxeter3 (if I knew what was failing since I don't have a 32-bit machine) and lie (although I don't think I have the technical prowess to do this on my own). I will open tickets for these.

comment:12 Changed 4 years ago by frederichan

Do you have some log files for the giacpy problems?

comment:13 Changed 4 years ago by dimpase

The only 4ti2-related test failures I know are in sandpiles, but they were there in 6.9 or 10 already, see #18618 (the latter took so long that I probably forgot to press for these being fixed). I have opened a ticket on this, #19783

Anything else?

Last edited 4 years ago by dimpase (previous) (diff)

comment:14 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Move broken optional packages to experimental to Fix optional packages

comment:15 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:17 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by jdemeyer

Replying to tscrim:

lie (although I don't think I have the technical prowess to do this on my own). I will open tickets for these.

lie just needs to be packaged properly using a real build/install system.

comment:18 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:20 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

lie (although I don't think I have the technical prowess to do this on my own). I will open tickets for these.

lie just needs to be packaged properly using a real build/install system.

That would be more of an upstream issue than changing around the spkg-install file, right?

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

Replying to tscrim:

That would be more of an upstream issue than changing around the spkg-install file, right?

Yes.

comment:22 in reply to: ↑ 21 Changed 4 years ago by dimpase

Replying to jdemeyer:

Replying to tscrim:

That would be more of an upstream issue than changing around the spkg-install file, right?

Yes.

However, note that the sources are in the author's literate programming system CWEBx, a clone of CWEB by Levy and Knuth. The fact that there is a tarball that does not need CWEBx is already great. I won't expect things magically changed to something even more Sage-compatible...

comment:23 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 4 years ago by frederichan

  • Description modified (diff)

I have open a ticket for the giacpy part. Could you post some details there?

comment:25 follow-up: Changed 4 years ago by vbraun

  • Cc mmarco added

Imho we should think about how important building on 32-bit is for optional packages; I expect that most normal users don't have access to 32-bit machines any more so its getting hard to test/fix.

comment:26 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:27 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:28 in reply to: ↑ 25 ; follow-up: Changed 4 years ago by mmarco

Replying to vbraun:

Imho we should think about how important building on 32-bit is for optional packages; I expect that most normal users don't have access to 32-bit machines any more so its getting hard to test/fix.

That is my case, i am trying to set up a 32 bits VM just for testing right now.

comment:29 Changed 4 years ago by vbraun

  • Description modified (diff)

comment:30 follow-up: Changed 4 years ago by mmarco

Btw, tides works fine in my 64 bits machine. Can you provide more details?

comment:31 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:32 in reply to: ↑ 30 Changed 4 years ago by jdemeyer

Replying to mmarco:

Btw, tides works fine in my 64 bits machine.

Did you check with Sage 7.0.beta1?

Can you provide more details?

sage -t --long src/sage/calculus/desolvers.py
**********************************************************************
File "src/sage/calculus/desolvers.py", line 1681, in sage.calculus.desolvers.desolve_tides_mpfr
Failed example:
    sol = desolve_tides_mpfr(f, [x0, y0, z0],0 , T, T, 1e-100, 1e-100, 100) # optional - tides
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.calculus.desolvers.desolve_tides_mpfr[9]>", line 1, in <module>
        sol = desolve_tides_mpfr(f, [x0, y0, z0],Integer(0) , T, T, RealNumber('1e-100'), RealNumber('1e-100'), Integer(100)) # optional - tides
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/calculus/desolvers.py", line 1731, in desolve_tides_mpfr
        subprocess.check_call(os.path.join(tempdir, 'runme'), shell=True,  stdout=subprocess.PIPE, stderr=subprocess.PIPE)
      File "/usr/local/src/sage-config/local/lib/python/subprocess.py", line 540, in check_call
        raise CalledProcessError(retcode, cmd)
    CalledProcessError: Command '/home/jdemeyer/.sage/temp/tamiyo/27921/dir_SMfRL3/runme' returned non-zero exit status 127
**********************************************************************

comment:33 Changed 4 years ago by jdemeyer

  • Cc malb mmezzarobba equaeghe added

comment:34 Changed 4 years ago by mmarco

I opened #19807 for the tides issue. I think i solved the problem on 64 bits (but please test it). Will try to look at the 32 bits problem in a VM.

comment:35 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:36 Changed 4 years ago by vbraun

  • Description modified (diff)

comment:37 in reply to: ↑ 28 Changed 4 years ago by tmonteil

Replying to mmarco:

That is my case, i am trying to set up a 32 bits VM just for testing right now.

You can automatically set up a series of 32bit VM and build/test Sage there at http://sagebuilder.metelu.net/ which i used when there was not enough binary builds for Debian and Ubuntu.

comment:38 follow-up: Changed 4 years ago by equaeghe

  • Cc equaeghe removed

comment:39 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:40 Changed 4 years ago by mmarco

I couldn't reproduce any problem to build tides on a 32 bits virtual machine. It is a fresh Ubuntu 15.10 install, with just build-essential, m4, dpkg-dev and git installed. Sage develop branch compiled from the beginning.

Which setup are you using?

comment:41 Changed 4 years ago by jdemeyer

  • Description modified (diff)

Yes, tides seems to build on 32-bit. I don't know why/how it failed before. Maybe I confused with a different package?

comment:42 Changed 4 years ago by dimpase

The 4ti2-dependent functionality of sandpiles (the one the causes the test failures) is deprecated. Fixing something that will be removed in half a year is a waste of time and effort.

I don't think we should demote 4ti2 to experimental just because of this.

comment:43 follow-up: Changed 4 years ago by jdemeyer

You could just mark the doctests as # known bug then.

comment:44 in reply to: ↑ 43 Changed 4 years ago by dimpase

Replying to jdemeyer:

You could just mark the doctests as # known bug then.

OK, done.

comment:45 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:46 follow-up: Changed 4 years ago by tscrim

So for lie, I know that you can reach an upstream contact. However, I don't think they are necessarily active, nor do I think they would care about improving the build process. So do you think we should rework the build process or fork the project and split it into 2, one for the library and one for the interface (this is the part that needs bison as I understand it)? Do you also think we should autotoolize the build as well?

I apologize in advance for the amount of hand-holding I will need for doing this.

comment:47 in reply to: ↑ 46 ; follow-up: Changed 4 years ago by jdemeyer

Replying to tscrim:

So for lie, I know that you can reach an upstream contact. However, I don't think they are necessarily active, nor do I think they would care about improving the build process.

I think we can look at the planarity package for what will hopefully happen. That package had the same problem as lie (but maybe slightly less bad). In that case, upstream didn't know about autotools but they accepted the autotoolization done by Nathann Cohen and now their "official release" is made with autotools.

So the first thing to do is to ask upstream what they think about this.

So do you think we should rework the build process or fork the project and split it into 2, one for the library and one for the interface (this is the part that needs bison as I understand it)?

I see no reason to split it up. Are you even sure that there is a "library"? At least I know that Sage does not use any library.

Do you also think we should autotoolize the build as well?

Obviously, see above.

comment:48 in reply to: ↑ 38 ; follow-up: Changed 4 years ago by equaeghe

Replying to equaeghe:

I removed myself from the Cc list, but I still get mails for this ticket. Please advise on how to stop receiving these mails.

comment:49 in reply to: ↑ 47 ; follow-up: Changed 4 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

So for lie, I know that you can reach an upstream contact. However, I don't think they are necessarily active, nor do I think they would care about improving the build process.

I think we can look at the planarity package for what will hopefully happen. That package had the same problem as lie (but maybe slightly less bad). In that case, upstream didn't know about autotools but they accepted the autotoolization done by Nathann Cohen and now their "official release" is made with autotools.

So the first thing to do is to ask upstream what they think about this.

I will send them a message and see what they say. However, I'm not sure we will necessarily receive a quick response, so I'm thinking we should start working on converting it to autotools.

What do you think about the bison dependency? Keep with the current concept of failing with an error message recommending the user install a system-wide bison?

So do you think we should rework the build process or fork the project and split it into 2, one for the library and one for the interface (this is the part that needs bison as I understand it)?

I see no reason to split it up. Are you even sure that there is a "library"? At least I know that Sage does not use any library.

At present, there isn't really a reason because we only have the interface in Sage. I'm somewhat planning on interacting with the algorithms in lie once more of #14901 is done, but I don't think that will happen until next year.

comment:50 in reply to: ↑ 48 ; follow-up: Changed 4 years ago by dimpase

Replying to equaeghe:

Replying to equaeghe:

I removed myself from the Cc list, but I still get mails for this ticket. Please advise on how to stop receiving these mails.

Sorry, this is a known bug/feature of trac (and/or our setup of it). Either fix it, or do mail filtering. In fact once you commented on a ticket, you will be mailed the comments, whether you are not in the CC list, or not.

comment:51 in reply to: ↑ 49 Changed 4 years ago by jdemeyer

Replying to tscrim:

What do you think about the bison dependency? Keep with the current concept of failing with an error message recommending the user install a system-wide bison?

None of the above: it shouldn't be a build-time dependency.

Normally, bison is a packaging-time dependency, but not a build-time or run-time dependency (unless there are very specific reasons why this cannot work for lie).

comment:52 in reply to: ↑ 50 ; follow-ups: Changed 4 years ago by equaeghe

Replying to dimpase:

Replying to equaeghe:

I removed myself from the Cc list, but I still get mails for this ticket. Please advise on how to stop receiving these mails.

Sorry, this is a known bug/feature of trac (and/or our setup of it). Either fix it, or do mail filtering. In fact once you commented on a ticket, you will be mailed the comments, whether you are not in the CC list, or not.

Well, given that I was added to the Cc list unasked, I am somewhat irritated.

It seems not to be a general trac bug, so it is probably a setup issue. Is there a ticket for this issue, so that I don't have to hijack this one? (Of course, I cannot fix it, given that I'm not an administrator of the trac instance.)

BTW: I shouldn't have to create filters; this should be fixed by the tract instance admin.

comment:53 in reply to: ↑ 52 Changed 4 years ago by jdemeyer

Replying to equaeghe:

Well, given that I was added to the Cc list unasked, I am somewhat irritated.

For the record: I added you in CC because you reported a cryptominisat issue in the past, so I assumed that you were interested in cryptominisat.

comment:54 in reply to: ↑ 52 ; follow-ups: Changed 4 years ago by dimpase

Replying to equaeghe:

Replying to dimpase:

Replying to equaeghe:

I removed myself from the Cc list, but I still get mails for this ticket. Please advise on how to stop receiving these mails.

Sorry, this is a known bug/feature of trac (and/or our setup of it). Either fix it, or do mail filtering. In fact once you commented on a ticket, you will be mailed the comments, whether you are not in the CC list, or not.

Well, given that I was added to the Cc list unasked, I am somewhat irritated.

It is perfectly acceptable among Sage developers to be CC'd on tickets without first asking. As you appear to be knowledgeable about cryptominisat, it was certainly done in good faith.

It seems not to be a general trac bug, so it is probably a setup issue.

Maybe. Do you know how to fix it? I certainly don't.

Is there a ticket for this issue, so that I don't have to hijack this one? (Of course, I cannot fix it, given that I'm not an administrator of the trac instance.)

I don't know. It's not hard to search Sage trac for specific tickets; please feel free to do so, and open a new ticket.

BTW: I shouldn't have to create filters; this should be fixed by the tract instance admin.

comment:55 in reply to: ↑ 54 Changed 4 years ago by kcrisman

It is perfectly acceptable among Sage developers to be CC'd on tickets without first asking. As you appear to be knowledgeable about cryptominisat, it was certainly done in good faith.

It seems not to be a general trac bug, so it is probably a setup issue.

Maybe. Do you know how to fix it? I certainly don't.

I think that if you can 'track' this down it would be very useful - this isn't the only time someone's wanted to get off a cc: list after having been added in good faith. (Sometimes added accidentally to a wrong ticket, too, and then impossible to get off.) So if you can, thank you for finding it!

comment:57 in reply to: ↑ 54 ; follow-up: Changed 4 years ago by equaeghe

Replying to dimpase:

I don't know. It's not hard to search Sage trac for specific tickets; please feel free to do so, and open a new ticket.

Please see #19832. Do not hesitate to add yourself to the CC list there, I'll refrain from doing it for yinz.

@vbraun: thanks for that link; it seems like there is a trade-off here. I'll add it to the other bug and investigate whether I can deal with this ticket in my Notifications preferences. In any case, my suggestion in the new ticket would be a usable stopgap measure.

comment:58 in reply to: ↑ 57 Changed 4 years ago by kcrisman

Please see #19832. Do not hesitate to add yourself to the CC list there, I'll refrain from doing it for yinz.

(Pittsburgh?)

comment:59 Changed 4 years ago by vinceknight

Sorry for getting to this late: I believe I was CC'd in because of gambit but noticed that this is no longer in the description. Has this been addressed?

comment:60 Changed 4 years ago by jdemeyer

There was no issue. It seems that there was something wrong with my original testing setup, causing some false positive failures. For cryptominisat, the crash seems genuine.

comment:61 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_optional_packages

comment:62 Changed 4 years ago by jdemeyer

  • Commit set to dd4e0cde24499ea5d58dfc4b57104a109053b1c6
  • Status changed from new to needs_review
  • Summary changed from Fix optional packages to Move broken optional packages to experimental

New commits:

dd4e0cdMove cryptominisat and lie to experimental

comment:63 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

I've heard back from upstream and they seem open to the idea. However I don't have final word yet.

Given that Volker wants to release 7.0 since we are already at 7.0.rc0, I don't think we would be able to autotoolize LiE in time. I really thought we would have more time...

In the interests of the 7.0 release, I'm going to set this to a positive review.

comment:64 Changed 4 years ago by vbraun

FWIW lie works. Depends on bison and the build system is definitely fugly, sure.

comment:65 Changed 4 years ago by dimpase

I really don't see why lie should be demoted to experimental.

comment:66 Changed 4 years ago by tscrim

Personally, I don't either, but I'm not an expert on build systems, trying to be a team player, and subsequently don't have any strong objections or reasons for not doing it. (Although I very slightly object to calling it broken.)

I would like to learn how to use autotools (and subsequently autotoolize coxeter3) and autotoolizing LiE is definitely something I want to do and I agree should be done. Moving it to experimental does give me more motivation to do it.

comment:67 follow-up: Changed 4 years ago by jdemeyer

It all depends on how high the "bar" should be for a package to become optional. Lie is borderline: on the one hand, it seems to work. On the other hand, both the upstream part and the Sage part of the build system are very strange.

comment:68 in reply to: ↑ 67 Changed 4 years ago by dimpase

Replying to jdemeyer:

It all depends on how high the "bar" should be for a package to become optional. Lie is borderline: on the one hand, it seems to work. On the other hand, both the upstream part and the Sage part of the build system are very strange.

Imagine HR firing perfectly functioning employees for being very strange otherwise :-)

comment:69 follow-up: Changed 4 years ago by frederichan

Does a naive patch to remove bison such as the following one: public/testlie-nobison http://git.sagemath.org/sage.git/log/?h=public/testlie-nobison works also for you (I have linux 64) and would change things for the lie spkg status?

comment:70 in reply to: ↑ 69 Changed 4 years ago by jdemeyer

Replying to frederichan:

Does a naive patch to remove bison such as the following one: public/testlie-nobison http://git.sagemath.org/sage.git/log/?h=public/testlie-nobison works also for you (I have linux 64) and would change things for the lie spkg status?

Please no! That would only make it worse.

The problem to be fixed is not that lie depends on bison, that's only a symptom. The real problem is the crazy build system.

comment:71 follow-up: Changed 4 years ago by dimpase

Jeroen, I really do not understand why you call the build system crazy. In paticular, the need for bison is quite easy to understand, and is quite normal - there is a parser for the "Lie language" to build, then it's quite usual to use a tool like bison/yacc to simplify this task.

comment:72 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_optional_packages to dd4e0cde24499ea5d58dfc4b57104a109053b1c6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:73 in reply to: ↑ 71 Changed 4 years ago by jdemeyer

  • Commit dd4e0cde24499ea5d58dfc4b57104a109053b1c6 deleted

Replying to dimpase:

Jeroen, I really do not understand why you call the build system crazy. In paticular, the need for bison is quite easy to understand, and is quite normal - there is a parser for the "Lie language" to build, then it's quite usual to use a tool like bison/yacc to simplify this task.

See 51: usually, bison is a packaging-time dependency, not a build-time dependency.

comment:74 Changed 4 years ago by dimpase

bison is a small stable program, available for all platforms you can imagine. In this sense it's much easier than autotools.

Note: See TracTickets for help on using tickets.