Opened 12 years ago

Closed 5 years ago

#5994 closed defect (fixed)

singular.version() has no doctest

Reported by: SimonKing Owned by: was
Priority: minor Milestone: sage-7.5
Component: interfaces Keywords: singular version help.cnf
Cc: kedlaya Merged in:
Authors: Jori Mäntysalo Reviewers: Jeroen Demeyer
Report Upstream: Completely fixed; Fix reported upstream Work issues:
Branch: 87c73ac (Commits, GitHub, GitLab) Commit: 87c73ac3a8a19d0779b9f31f21b23b629a0fe82e
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Neither singular.version nor singular_version have doc tests.

Attachments (1)

trac_5994.patch (1.6 KB) - added by was 11 years ago.

Download all attachments as: .zip

Change History (56)

comment:1 follow-up: Changed 12 years ago by SimonKing

Remark:

Apparently the problem is on the side of Singular.

In Singular, freshly started, when you do

> system("--version");

there is the error (it says "? cannot open help.cnf"). If you repeat, no error.

If people think that my suggestion would break code, I suggest to have a new method singular.version_number().

comment:2 in reply to: ↑ 1 ; follow-up: Changed 12 years ago by SimonKing

Replying to SimonKing:

Apparently the problem is on the side of Singular.

And meanwhile it is reported upstream.

I do not know, however, if the problem still occurs with Singular 3-1-0 (I only have a beta version, and this still shows the error).

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 12 years ago by SimonKing

Replying to SimonKing:

Replying to SimonKing: And meanwhile it is reported upstream.

I do not know, however, if the problem still occurs with Singular 3-1-0 (I only have a beta version, and this still shows the error).

"Upstream" answered, and it seems that the problem is fixed in the official release.

I could verify that on the server in Oberwolfach, the error does not occur in Singular 3-1-0, but it does occur in Oberwolfach in Singular 3-0-4 (as being part of Sage). Opinions?

But there still remains the question if singular.version() ought to return a tuple of integers, rather than a lengthy string with all build informations.

comment:4 in reply to: ↑ 3 Changed 12 years ago by AlexGhitza

Replying to SimonKing:

"Upstream" answered, and it seems that the problem is fixed in the official release.

I could verify that on the server in Oberwolfach, the error does not occur in Singular 3-1-0, but it does occur in Oberwolfach in Singular 3-0-4 (as being part of Sage). Opinions?

We can probably wait until after Sage Days 15 for this?

But there still remains the question if singular.version() ought to return a tuple of integers, rather than a lengthy string with all build informations.

My personal preference would be for it to return a tuple of integers, but we could give it an optional argument with_build_info=True (or verbatim=True or something similar) to make it return the lengthy string.

comment:5 in reply to: ↑ 3 Changed 12 years ago by SimonKing

Replying to SimonKing:

I could verify that on the server in Oberwolfach, the error does not occur in Singular 3-1-0, but it does occur in Oberwolfach in Singular 3-0-4 (as being part of Sage). Opinions?

The "Opinions?" bit should be two lines lower. Sorry.

It turned out that the error occurs in any Sage-built Singular version that I know: Singular 3-0-4 on sage.math, on two of my computers, in Oberwolfach, and Singular 3-1-0-Beta on sage.math and on one of my computers.

If Singular is not built by Sage, then the error apparently does not occur: With Singular 3-0-3 (very old) on one of my machines, Singular-3-1-0-Beta on my machine (same sources, modulo the usual patches, as the sage-built version!), and Singular-3-1-0 official release in Oberwolfach.

So, after all, it seems to me that it is all Sage's fault.

But there still remains the question if singular.version() ought to return a tuple of integers, rather than a lengthy string with all build informations.

Here is where I want to know some "Opinions"...

comment:6 follow-up: Changed 11 years ago by was

  • Report Upstream set to N/A
  • Status changed from new to needs_review

The attached patch fixes the output format for version to be more consistent with the other interfaces, e.g., gp.version(). It also programs around the issue with help files, which is *not* fixed in Singular-3-1-0... and I'm not at all clear *why* it is considered a bug in Singular by the people in the thread above.

Changed 11 years ago by was

comment:7 in reply to: ↑ 6 Changed 11 years ago by SimonKing

Replying to was:

It also programs around the issue with help files, which is *not* fixed in Singular-3-1-0... and I'm not at all clear *why* it is considered a bug in Singular by the people in the thread above.

Note that I originally thought that the issue with help files is a problem of Singular, and clearly a bug: I mean, you ask for the version number and get an error; you ask again, and it works! It had reported it upstream.

But then the impression came across (see my last post) that it only occurs in Singular if it is built by Sage. In this case, it could be a problem with the patched version in Sage, which might be worth another ticket.

Cheers, Simon

comment:8 follow-up: Changed 11 years ago by was

I think the first time you ask for the version, singular fires up its help system, and reports a bug about it not being properly configured by *us* for such use. Then it doesn't report that again, since it already did. I think it is very sensible behavior by Singular.

So can you review this?

comment:9 Changed 11 years ago by wjp

For the record, adding an empty $SAGE_ROOT/local/share/singular/help.cnf suppresses the error. There is actually a help.cnf file in the singular sources (, but it doesn't seem to get installed. Its contents don't look to be particularly relevant for sage at first glance, though.

comment:10 in reply to: ↑ 8 ; follow-ups: Changed 11 years ago by SimonKing

Replying to was:

So can you review this?

I'd like to, but the sage-4.3.1.alpha1 that I had built on sage-math seems broken. It used to work, but when I did ./sage -br main, it failed with

ImportError: /usr/lib/libstdc++.so.6: version `GLIBCXX_3.4.11' not found (required by /home/SimonKing/SAGE/sage-4.3.1.alpha1/local/lib/libgmpxx.so.3)

Might ./sage -ba work?

Anyway, I'd like to see one more doc test, that checks consistency with another way of getting the version number -- just for consistency:

sage: tuple([Integer(c) for c in singular.eval('system("version")')][:3] == singular.version()[0]
True

comment:11 in reply to: ↑ 10 Changed 11 years ago by SimonKing

Replying to SimonKing:

...

sage: tuple([Integer(c) for c in singular.eval('system("version")')][:3] == singular.version()[0]
True

Oops, apparently I forgot a closing bracket on the left hand side. Anyway, you know what this prospective doc-test is supposed to do...

comment:12 in reply to: ↑ 10 Changed 11 years ago by SimonKing

Replying to SimonKing:

Might ./sage -ba work?

It did not work. I fear that I have to start from scratch, so that it will take some hours before I will be able to review the patch.

comment:13 follow-up: Changed 11 years ago by SimonKing

  • Status changed from needs_review to needs_info

OK, meanwhile I built sage-4.3.1.rc1

The patch applies cleanly.

However, I don't like the doc tests, and I think the return value is wrong.

The tests check that the first version number is 3. OK, it will eventually change, but not in the near future.

Then they test that the version number is of length 3. Can we rely on it? There used to be two-digit versions.

In fact, the "official" version number seems to be four digits, not three:

sage: singular.eval('system("version")')
'3104'

Hence, the first return value of singular.version() should be (3,1,0,4) not (3,1,0). Note that according to the Singular homepage there is a version 3-1-0-6 available, so, the last digit does play a role.

So, my questions are:

  • How important is it to have doc tests that remain valid if the Singular version changes?
  • Do we take the patch level version number of Singular serious?

comment:14 in reply to: ↑ 13 Changed 11 years ago by SimonKing

Replying to SimonKing:

So, my questions are:

  • How important is it to have doc tests that remain valid if the Singular version changes?
  • Do we take the patch level version number of Singular serious?

Is there an answer, yet?

comment:15 follow-up: Changed 10 years ago by SimonKing

-- bump --

First question: Do we want that singular.version() works? Currently, it fails when first called.

Second question: Do we want that (at least by default) it returns a tuple of three or four numbers (three- resp four-digit vesion numbers), or do people like that the output of singular.version() (if it is called again after the initial error) returns a lengthy string with full information on the way Singular has been built?

comment:16 Changed 10 years ago by leif

  • Authors set to William Stein
  • Keywords help.cnf added

It shouldn't hurt to install the help.cnf file though, in which case we wouldn't need (and should remove) the try ... except RuntimeError ... and a second try.

comment:17 Changed 10 years ago by leif

  • Cc kedlaya added

Since he reported this again on a duplicate, #11519.

comment:18 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:19 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:20 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:21 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:22 in reply to: ↑ 15 Changed 6 years ago by jmantysalo

Replying to SimonKing:

Do we want that (at least by default) it returns a tuple of three or four numbers (three- resp four-digit vesion numbers), or do people like that the output of singular.version() (if it is called again after the initial error) returns a lengthy string with full information on the way Singular has been built?

As a default I would suggest output similar to pari.version(). But for now at least kash_version() and r_version() gives slightly different output...

Maybe right way could be to remove them, and have something like version('kash') for one package and version() for all packages?

comment:23 Changed 6 years ago by SimonKing

Gosh, is that still not fixed? Sorry that I spoiled it by asking questions in comment:13 that nobody bothers to answer...

comment:24 follow-up: Changed 6 years ago by SimonKing

To repeat my questions:

  • How important is it to have doc tests that remain valid if the Singular version changes?

My answer: We could at least have a test marked random. In that way, we at least make sure that there is no error raised.

  • Do we take the patch level version number of Singular serious? Respectively do we really want the current behaviour (singular.version() returns a lengthy string), or would we prefer to have it return a tuple of integers?

My answer: We have

sage: pari.version()
[2, 8, 0]
sage: r.version()
((3, 2, 1), 'R version 3.2.1 (2015-06-18)')
sage: gap.version()
'4.7.8'
sage: sage0.version()
'SageMath Version 6.9.beta3, Release Date: 2015-08-21'

Hence, the different interfaces have all kind of different output. Thus, the current behaviour should be fine, but the following output

sage: singular.eval('system("version")')
'3170'

would be equally fine.

My suggestion is: Use the SHORT version string by default, but provide singular_version and singular.version with an optional "verbose=False" argument, that allows to obtain the lengthy version string. And of course fix the error.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 6 years ago by jmantysalo

Replying to SimonKing:

To repeat my questions:

  • How important is it to have doc tests that remain valid if the Singular version changes?

My answer: We could at least have a test marked random. In that way, we at least make sure that there is no error raised.

+1

My suggestion is: Use the SHORT version string by default, but provide singular_version and singular.version with an optional "verbose=False" argument, that allows to obtain the lengthy version string. And of course fix the error.

Sounds good. Somehow the output of pari.version() seems best for me. Maybe this could also be documented in http://doc.sagemath.org/html/en/developer/packaging.html#package-versioning as the recommended way to have *.version() commands?

comment:26 in reply to: ↑ 25 ; follow-up: Changed 6 years ago by SimonKing

Replying to jmantysalo:

Sounds good. Somehow the output of pari.version() seems best for me.

Slight problem: The patching level usually is not indicated by a number but by "p" plus a number. So, providing the version as a list of integers is perhaps not what we want. And it is not good, I think, if the default output is a list of integers and the verbose output is a totally different type, namely a string.

From that perspective, the r-output is better.

Maybe this could also be documented in http://doc.sagemath.org/html/en/developer/packaging.html#package-versioning as the recommended way to have *.version() commands?

I didn't know about the existence of a package_version.txt. I wonder if we could access these data---after all, the package version of singular is "3.1.7p1.p0", but the version shown by singular.eval('system("version")') is "3170". Similarly for r, it should be 3.2.1.p0, not (3,2,1).

I guess it is time to do a bit bike shedding at sage-devel.

comment:27 in reply to: ↑ 26 Changed 6 years ago by jmantysalo

Replying to SimonKing:

Slight problem: The patching level usually is not indicated by a number but by "p" plus a number. So, providing the version as a list of integers is perhaps not what we want. And it is not good, I think, if the default output is a list of integers and the verbose output is a totally different type, namely a string.

From that perspective, the r-output is better.

True.

As a system admin I should be able to answer fast to questions "What is the version of ... in our server ...?". For that it is not so important what is the format. But it gives a little unprofessional feeling if I get (1,2,3) from foo.version() and [1,2,3] from bar.version().

comment:28 Changed 6 years ago by SimonKing

  • Branch set to u/SimonKing/singular_version_yielding_error

comment:29 Changed 6 years ago by SimonKing

  • Authors changed from William Stein to William Stein, Simon King
  • Commit set to 70f7b1c5d206e8627f3d124a28b7083e3a82313a
  • Status changed from needs_info to needs_review

I work around the bug that results in the error, and I think it is best to return both a version tuple on the one hand (so that one can easily test if the version is enough recent) and the complete version information as provided by Singular as a string.

Hopefully the reviewers agree...

comment:30 Changed 6 years ago by SimonKing

Why is it not possible to click on the attached branch an see the changes?

comment:31 Changed 6 years ago by SimonKing

Arrgh. I forgot to commit before pushing :-/

comment:32 Changed 6 years ago by git

  • Commit changed from 70f7b1c5d206e8627f3d124a28b7083e3a82313a to a2ae8c869553c8aff5404ef2cfcaf95d2f8eac52

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

a2ae8c8Fix error in singular_version()

comment:33 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The error should be reported upstream and the comment in the code should link to this ticket and the upstream report.

comment:34 follow-up: Changed 6 years ago by SimonKing

I am not so sure if it requires an uptream report. The error is about a missing file, and according to comment:9, adding an empty $SAGE_ROOT/local/share/singular/help.cnf suppresses the error.

comment:35 in reply to: ↑ 34 Changed 6 years ago by jdemeyer

Replying to SimonKing:

adding an empty $SAGE_ROOT/local/share/singular/help.cnf suppresses the error.

Then that should be done instead of wrapping the call in a try/except block (which is a really ugly "solution")

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

There is actually a help.cnf installed in

$SAGE_LOCAL/share/singular/LIB/help.cnf

so I guess it's a bug in Singular that the file is either installed or looked for in the wrong place.

In any case, copying that file to $SAGE_LOCAL/share/singular/help.cnf would be a good solution for this ticket.

comment:37 in reply to: ↑ 36 ; follow-up: Changed 6 years ago by SimonKing

Replying to jdemeyer:

There is actually a help.cnf installed in

$SAGE_LOCAL/share/singular/LIB/help.cnf

so I guess it's a bug in Singular that the file is either installed or looked for in the wrong place.

From reading this change log, I believe that .../share singular/LIB/ is the correct place.

I'll file a ticket in the singular trac server. But for now, we need a work-around in Sage.

comment:38 Changed 6 years ago by SimonKing

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

I reported it upstream.

Last edited 6 years ago by SimonKing (previous) (diff)

comment:39 follow-up: Changed 6 years ago by SimonKing

I stand corrected! There used to be $SAGE_LOCAL/share/singular/help.cnf, but it is gone. The only help.cnf found under $SAGE_ROOT is upstream/src/latest/Singular/LIB/help.cnf.

So, apparently we don't install it at all!

comment:40 in reply to: ↑ 39 Changed 6 years ago by jdemeyer

Replying to SimonKing:

So, apparently we don't install it at all!

You're right, this must have come from some earlier Singular version (note the date):

$ ls -l local/share/singular/LIB/help.cnf
-rw-r--r-- 1 jdemeyer jdemeyer 3054 Oct 29  2014 local/share/singular/LIB/help.cnf

comment:41 in reply to: ↑ 37 ; follow-up: Changed 6 years ago by jdemeyer

Replying to SimonKing:

But for now, we need a work-around in Sage.

I suggest you just change spkg-install to touch the empty file $SAGE_SHARE/singular/help.cnf

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:42 in reply to: ↑ 41 ; follow-up: Changed 6 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

But for now, we need a work-around in Sage.

I suggest you just change spkg-install to touch the empty file $SAGE_SHARE/singular/help.cnf

Why not modify spkg-install, so that it installs the missing file? Alternatively, we could patch Singular's Makefile (or whatever it is called) so that it installs the missing file. What is the preferred way?

Also, I'd like to know why Singular's Makefile does not install help.cnf. After all, if I understand correctly, it DOES install all the library files of Singular (which are in the same folder as help.cnf).

Hang on. I just notice that the singular spkg apparently screws completely. Or how would you explain that I find the complete UNPACKED singular source tree in SAGE_LOCAL/upstream/ ? Do we need a new Singular package?

Last edited 6 years ago by SimonKing (previous) (diff)

comment:43 Changed 6 years ago by SimonKing

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.

Upstream says that help.cnf should indeed be put into the same folder as all the other library files, i.e., local/share/singular/.

comment:44 in reply to: ↑ 42 ; follow-up: Changed 6 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

Replying to SimonKing:

But for now, we need a work-around in Sage.

I suggest you just change spkg-install to touch the empty file $SAGE_SHARE/singular/help.cnf

Why not modify spkg-install, so that it installs the missing file? Alternatively, we could patch Singular's Makefile (or whatever it is called) so that it installs the missing file. What is the preferred way?

I don't really care much.

Hang on. I just notice that the singular spkg apparently screws completely. Or how would you explain that I find the complete UNPACKED singular source tree in SAGE_LOCAL/upstream/ ?

Probably you once extracted the tarball in upstream? It's not the package which does that...

comment:45 in reply to: ↑ 44 Changed 6 years ago by SimonKing

Replying to jdemeyer:

Why not modify spkg-install, so that it installs the missing file? Alternatively, we could patch Singular's Makefile (or whatever it is called) so that it installs the missing file. What is the preferred way?

I don't really care much.

Then I guess we should do it in spkg-install.

Probably you once extracted the tarball in upstream? It's not the package which does that...

Could be. I just deleted it and did "sage -f singular".

comment:46 follow-up: Changed 6 years ago by leif

There's also #sagemath on freenode. Just saying...

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

Replying to leif:

There's also #sagemath on freenode. Just saying...

Meaning what?

comment:48 in reply to: ↑ 47 Changed 6 years ago by leif

Replying to SimonKing:

Replying to leif:

There's also #sagemath on freenode. Just saying...

Meaning what?

Overfull inbox, TeX would have said. ;-)

comment:49 follow-up: Changed 6 years ago by SimonKing

It seems the current agreement seems to be to modify spkg-install, so that it copies help.cnf from the singular sources to $SAGE_SHARE/singular/. What is the path to the singular sources while spkg-install is executed?

comment:50 in reply to: ↑ 49 Changed 6 years ago by jdemeyer

Replying to SimonKing:

What is the path to the singular sources while spkg-install is executed?

It's the current working directory.

comment:51 Changed 5 years ago by jmantysalo

  • Branch changed from u/SimonKing/singular_version_yielding_error to u/jmantysalo/singular_version_yielding_error

comment:52 Changed 5 years ago by jmantysalo

  • Authors changed from William Stein, Simon King to Jori Mäntysalo
  • Commit changed from a2ae8c869553c8aff5404ef2cfcaf95d2f8eac52 to 87c73ac3a8a19d0779b9f31f21b23b629a0fe82e
  • Description modified (diff)
  • Milestone changed from sage-6.4 to sage-7.5
  • Priority changed from major to minor
  • Report Upstream changed from Reported upstream. Developers deny it's a bug. to Completely fixed; Fix reported upstream
  • Status changed from needs_work to needs_review

Error has gone away with Singular 4.

I added doctest.

What goes to output format, I think this should be changed in all *_version() commands at once. (Assuming somebody would care, which is propably not true.)


New commits:

87c73acDoctest singular_version().

comment:53 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from singular.version() yields an error when first called, has no doctest, and has a strange output imo to singular.version() has no doctest

comment:54 Changed 5 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:55 Changed 5 years ago by vbraun

  • Branch changed from u/jmantysalo/singular_version_yielding_error to 87c73ac3a8a19d0779b9f31f21b23b629a0fe82e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.